Hi Jozsef, On Thu, 30 Aug 2018 11:10:02 +0200 (CEST) Jozsef Kadlecsik <[email protected]> wrote:
> 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. Oh, right, I didn't think of that. I'll send a v3. > > @@ -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(). Right, I don't need to check that setname is there. Still, its length is not checked as far as I can see. Would you suggest that I move the length check to build_msg()? -- Stefano
