On Tue, Jul 19, 2022 at 12:31:47PM +0200, Theo Buehler wrote: > 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]?
Makes sense. It is again not really an issue because aspath_verify() filters those ASPATHs out and aspath_extract() should only be called on valid ASPATHs. I will commit this with the pos < 0 check. > > > > -- > > :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)); > > > -- :wq Claudio
