On Mon, Oct 09, 2023 at 11:55:36PM +0200, Theo Buehler wrote: > On Mon, Oct 09, 2023 at 11:50:14PM +0200, Tobias Heider wrote: > > 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.
Actually looking at net/pfkeyv2_parsemessage.c there is a good reason to use sadb_x_tag_len here too. The generic header length field gets validated in pfkeyv2_parsemessage() before being forwarded to userland, sadb_x_tag_taglen doesn't. But we might want to harden that too. > > > > > > > > > + 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. > > Ok, if this is guaranteed to have zero padding if the length isn't a > multiple of 8 the remainder then I have no concern apart from the extra > parentheses. fair enough