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?
> + 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?
> + 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
>
