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: santiago@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."