Hi Ondrej, Thanks for the quick response.
Unfortunately it has been included in released versions 1.6.7 and 2.0.5.
Bummer, apologies for missing that. Do you want to request a CVE or should I?
It is unnecessary with draft-rfc8203bis, as all possible lengths are permitted.
That makes sense. In retrospect my patch will clearly reject messages that have a permitted length and isn't a suitable fix.
For the stack buffer, the proper fix seems to be just enlarge it to fit max-size message
That makes sense and I agree makes a better fix. Thanks for your patience/explanation.
The stack overflow seems limited, i would like to check what exactly is overwritten when built with normal settings.
Agreed. I spent a little bit of time thinking about exploitation conditions given the limited control and while I wasn't able to come up with anything practical myself on x86_64 there is evidence[0] of similarly limited vulnerabilities producing RCE when those skilled in that particular dark art dedicate their attention to the matter. [0]: https://googleprojectzero.blogspot.com/2014/08/the-poisoned-nul-byte-2014-ed... On Sun, Sep 8, 2019 at 5:12 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
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."