Hi Stefano,

On Wed, 29 Aug 2018, Stefano Brivio wrote:

> We might overrun the buffer used to save it otherwise.
> 
> Signed-off-by: Stefano Brivio <[email protected]>
> ---
> v2: As requested by Jozsef, move validation of setname length to
>     attr2data() for data received via netlink, instead of doing
>     it in callback_list().
> 
>  lib/session.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/session.c b/lib/session.c
> index ca96aaa57ea6..74310a19850e 100644
> --- a/lib/session.c
> +++ b/lib/session.c
> @@ -691,6 +691,13 @@ attr2data(struct ipset_session *session, struct nlattr 
> *nla[],
>               D("nla typename %s",
>                 (const char *) ipset_data_get(data, IPSET_OPT_TYPENAME));
>  #endif
> +     if (type == IPSET_ATTR_SETNAME && nla[IPSET_ATTR_DATA]) {
> +             const char *setname = ipset_data_setname(data);
> +             if (!setname || strlen(setname) >= IPSET_MAXNAMELEN)
> +                     FAILURE("Broken kernel message: "
> +                             "setname missing or too long!");
> +     }
> +
>       return ret;
>  }

Actually, I was thinking something like this:

        if (attr->type == MNL_TYPE_UNSPEC)
                return 0;
        else if (attr->type == MNL_TYPE_NUL_STRING) {
                const char *str = d;
                if (!d || strlen(d) >= attr->len)
                        FAILURE(...)
        } ...
                
That way all typename is also verified.

> @@ -2014,7 +2021,11 @@ ipset_cmd(struct ipset_session *session, enum 
> ipset_cmd cmd, uint32_t lineno)
>       if (session->lineno != 0 &&
>           (cmd == IPSET_CMD_ADD || cmd == IPSET_CMD_DEL)) {
>               /* Save setname for the next possible aggregated restore line */
> -             strcpy(session->saved_setname, ipset_data_setname(data));
> +             const char *setname = ipset_data_setname(data);
> +             if (!setname || strlen(setname) >= IPSET_MAXNAMELEN)
> +                     return ipset_err(session,
> +                             "Invalid command: setname missing or too long");
> +             strcpy(session->saved_setname, setname);

I don't understand this part: it is from userspace to kernel and ADD/DEL 
commands already verified that setname is filled out, in build_msg().

>               ipset_data_reset(data);
>               /* Don't commit: we may aggregate next command */
>               ret = 0;
> -- 

Best regards,
Jozsef
-
E-mail  : [email protected], [email protected]
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Reply via email to