On Tue, Jul 19, 2022 at 11:43:25AM +0200, Claudio Jeker wrote:
> aspath_extract() should do at least a minimal overflow check and not
> access memory after the segment. Can't use fatalx here because bgpctl
> also uses this function. Instead return 0, that is an invalid ASN.
> No code will check the return value but that is fine since all callers
> ensure that pos does not overflow.
There are a few calls of the form
as = aspath_extract(seg, seg_len - 1)
where seg_len = ptr[1], so a priori pos == -1 seems possible. Should the
check not be
if (pos < 0 || pos >= ptr[1])
return 0;
to exclude an access at ptr[-2]?
>
> --
> :wq Claudio
>
> Index: util.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 util.c
> --- util.c 28 Jun 2022 05:49:05 -0000 1.69
> +++ util.c 28 Jun 2022 08:31:10 -0000
> @@ -364,7 +364,7 @@ aspath_strlen(void *data, uint16_t len)
> /*
> * Extract the asnum out of the as segment at the specified position.
> * Direct access is not possible because of non-aligned reads.
> - * ATTENTION: no bounds checks are done.
> + * Only works on verified 4-byte AS paths.
> */
> uint32_t
> aspath_extract(const void *seg, int pos)
> @@ -372,6 +372,9 @@ aspath_extract(const void *seg, int pos)
> const u_char *ptr = seg;
> uint32_t as;
>
> + /* minimal overflow check, return 0 since that is an invalid ASN */
> + if (pos >= ptr[1])
> + return (0);
> ptr += 2 + sizeof(uint32_t) * pos;
> memcpy(&as, ptr, sizeof(uint32_t));
> return (ntohl(as));
>
