On Mon, Oct 09, 2023 at 11:24:19PM +0200, Theo Buehler wrote:
> On Mon, Oct 09, 2023 at 10:49:53PM +0200, Tobias Heider wrote:
> > ipsecctl wrongly assumes that strings like the pf tag or
> > the identities are always null terminated.
> > The diff below fixes the cases that always kill my
> > ipsecctl -m when running a fuzzer.
> > 
> > ok?
> > 
> > Index: pfkdump.c
> > ===================================================================
> > RCS file: /mount/openbsd/cvs/src/sbin/ipsecctl/pfkdump.c,v
> > retrieving revision 1.57
> > diff -u -p -r1.57 pfkdump.c
> > --- pfkdump.c       7 Aug 2023 04:10:08 -0000       1.57
> > +++ pfkdump.c       9 Oct 2023 20:41:54 -0000
> > @@ -406,9 +406,11 @@ print_tag(struct sadb_ext *ext, struct s
> >  {
> >     struct sadb_x_tag *stag = (struct sadb_x_tag *)ext;
> >     char *p;
> > +   int plen;
> >  
> >     p = (char *)(stag + 1);
> > -   printf("%s", p);
> > +   plen = (stag->sadb_x_tag_len * 8) - sizeof(*stag);
> 
> If I understand correctly this attempts to undo the computation in
> export_tag(). So this computes PADUP(stag->sadb_x_tag_taglen) where
> PADUP() rounds up to the next multiple of 8. Should this not use
> stag->sadb_x_tag_taglen - 1 instead, which is the strlen of the tag?

Thanks, I don't think this version can read out-of-bounds,
see explanation below, but sadb_x_tag_taglen is certainly even better.

> 
> > +   printf("%.*s", plen, p);
> >  }
> >  
> >  static void
> > @@ -590,10 +592,12 @@ static void
> >  print_ident(struct sadb_ext *ext, struct sadb_msg *msg, int opts)
> >  {
> >     struct sadb_ident *ident = (struct sadb_ident *)ext;
> > +   int ilen;
> >  
> > -   printf("type %s id %llu: %s",
> > +   ilen = (ident->sadb_ident_len * 8) - sizeof(*ident);
> 
> Similar problem here, but there doesn't seem to be an easy way to get
> the actual length of the identity. Doesn't this still overread?

Not as far as I understand it. The whole payload is sadb_ident + string + 
eventual
zero padding. We read the string until we hit the first null byte or the end of 
the
whole payload which should be safe.

> 
> > +   printf("type %s id %llu: %.*s",
> >         lookup_name(identity_types, ident->sadb_ident_type),
> > -       ident->sadb_ident_id, (char *)(ident + 1));
> > +       ident->sadb_ident_id, ilen, (char *)(ident + 1));
> >  }
> >  
> >  static void
> > 

Reply via email to