On Wed, Sep 19, 2018 at 01:45:03PM +0000, Aaron A. Glenn wrote:
> * Theo de Raadt <[email protected]> [2018-09-19 11:34]:
> >
> > Repeat the test please.
>
> (gdb) bt
> #0 0x00000bcf682ccf80 in memcpy (dst0=0x7f7ffffec304, src0=0xbceff4c9ffd,
> length=4)
> at /usr/src/lib/libc/string/memcpy.c:97
> #1 0x00000bccac440498 in aspath_verify (data=Variable "data" is not
> available.
> ) at /usr/src/usr.sbin/bgpd/util.c:488
> #2 0x00000bccac41a404 in rde_attr_parse (p=Variable "p" is not available.
> ) at /usr/src/usr.sbin/bgpd/rde.c:1509
> #3 0x00000bccac417ee9 in rde_update_dispatch (imsg=0x7f7ffffec560) at
> /usr/src/usr.sbin/bgpd/rde.c:1066
> #4 0x00000bccac41708a in rde_dispatch_imsg_session (ibuf=0xbceccd84000) at
> /usr/src/usr.sbin/bgpd/rde.c:400
> #5 0x00000bccac41577b in rde_main (debug=0, verbose=Variable "verbose" is not
> available.
> ) at /usr/src/usr.sbin/bgpd/rde.c:309
> #6 0x00000bccac401047 in main (argc=0, argv=0x7f7ffffed248) at
> /usr/src/usr.sbin/bgpd/bgpd.c:183
> (gdb) bt full
> #0 0x00000bcf682ccf80 in memcpy (dst0=0x7f7ffffec304, src0=0xbceff4c9ffd,
> length=4)
> at /usr/src/lib/libc/string/memcpy.c:97
> dst = 0x7f7ffffec304 ""
> src = 0xbceff4ca000 <Address 0xbceff4ca000 out of bounds>
> t = 4
> #1 0x00000bccac440498 in aspath_verify (data=Variable "data" is not
> available.
> ) at /usr/src/usr.sbin/bgpd/util.c:488
> as = Cannot access memory at address 0x0
> (gdb)
>
> Had to build bgpd w/ symbols on another machine (though a timestamp matched
> source tree)
>
Please try the following diff. This shuffles a few things around and fixes
two issues:
a) The 0 lenght check needs to be made agains seg_len not seg_size (since
that can never be 0). I moved the check a bit up to make it clearer.
b) don't increase the ptr in the AS 0 check before the memcpy().
b) results in the 4 byte read overflow which you tripped on.
--
:wq Claudio
Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
retrieving revision 1.35
diff -u -p -r1.35 util.c
--- util.c 14 Sep 2018 10:22:11 -0000 1.35
+++ util.c 19 Sep 2018 14:24:32 -0000
@@ -451,7 +451,7 @@ aspath_verify(void *data, u_int16_t len,
as_size = 4;
for (; len > 0; len -= seg_size, seg += seg_size) {
- const u_char *ptr;
+ const u_int8_t *ptr;
int pos;
if (len < 2) /* header length check */
@@ -459,6 +459,10 @@ aspath_verify(void *data, u_int16_t len,
seg_type = seg[0];
seg_len = seg[1];
+ if (seg_len == 0)
+ /* empty aspath segments are not allowed */
+ return (AS_ERR_BAD);
+
/*
* BGP confederations should not show up but consider them
* as a soft error which invalidates the path but keeps the
@@ -475,19 +479,15 @@ aspath_verify(void *data, u_int16_t len,
if (seg_size > len)
return (AS_ERR_LEN);
- if (seg_size == 0)
- /* empty aspath segments are not allowed */
- return (AS_ERR_BAD);
-
/* RFC 7607 - AS 0 is considered malformed */
ptr = seg + 2;
for (pos = 0; pos < seg_len; pos++) {
- u_int32_t as = 0;
+ u_int32_t as;
- ptr += as_size;
memcpy(&as, ptr, as_size);
if (as == 0)
error = AS_ERR_SOFT;
+ ptr += as_size;
}
}
return (error); /* aspath is valid but probably not loop free */
