Stack overflow in RFC 8203 BGP admin. shutdown comm. handling since 7ff34ca2
santiago at crfreenet.org
Sun Sep 8 23:12:20 CEST 2019
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 that allows a BGP peer to corrupt stack memory via crafted RFC
> 8203 BGP administrative shutdown communication message.
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
> - /* 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)
For the stack buffer, the proper fix seems to be just enlarge it to fit
> 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 at 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."
More information about the Bird-users