On Sun, Sep 08, 2019 at 01:59:03PM -0400, Daniel McCarney wrote: > Hi there, > > I believe a stack overflow was introduced in the BGP protocol support of BIRD > in > 7ff34ca2[1] that allows a BGP peer to corrupt stack memory via crafted RFC > 8203[0] BGP administrative shutdown communication message.
Hi Yes, you are right, thanks for detailed writeup. > Leading with the best news, it looks like 7ff34ca2 hasn't been included in > a release yet :-) Unfortunately it has been included in released versions 1.6.7 and 2.0.5. > First, if `msg_len + 1` is > `len` but <= 255 then the defensive expression > will > short-circuit as false and execution continues. This results in the > `proto_set_message` buffer being sized equal to `msg_len` but only filed with > `len` bytes (which may be 0) of provided message data. The unaccounted for > message bytes will contain uninitialized memory contents that will be written > to > the log as part of the RFC 8203 shutdown communication. Yes, seems like switched && for ||. > Second, and more crucially, the `msg_len > 255` calculation fails to account > for > the 4 extra bytes that will be written by the `bsprintf` formatting > expression. > E.g. there will be three bytes that precede the `msg_len` message bytes (`: > "`) > and one byte that follows them (`"`). Note that the purpose of this check was not a check for buffer size, but a check for validity of input message (max 128 B according to RFC 8203). It is unnecessary with draft-rfc8203bis, as all possible lengths are permitted. > - /* Handle proper message */ > - if ((msg_len > 255) && (msg_len + 1 > len)) > + /* Handle only messages with a length that does not require reading past > the > + * end of the received data. */ > + if (msg_len < (len - 1)) > return 0; This seems incorrect: msg_len is the nominal message length, while len is remaining length of the packet, so the check for nominal message not fitting into the packet would be: if (msg_len + 1 > len) return 0; For the stack buffer, the proper fix seems to be just enlarge it to fit max-size message. > In practice I find the stack overflow does not impact the availability of the > BIRD instance when built with normal settings. I confirmed the overflow two > ways: with `gdb` and by building BIRD with `-fsanitize=address`. > > These bytes match to `!!"\0`, showing the two attacker controlled bytes and > the > two bytes unconditionally written. The stack overflow seems limited, i would like to check what exactly is overwritten when built with normal settings. Once more, thanks for bugreport and the detailed writeup. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."