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-edition.html On Sun, Sep 8, 2019 at 5:12 PM Ondrej Zajicek <[email protected]> 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: [email protected]) > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) > "To err is human -- to blame it on a computer is even more so."
