Hi Jozsef,
On Thu, 30 Aug 2018 11:10:02 +0200 (CEST)
Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxxxxxx> 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 <sbrivio@xxxxxxxxxx>
> > ---
> > 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
