Stack overflow in RFC 8203 BGP admin. shutdown comm. handling since 7ff34ca2

[Ondrej Zajicek]

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 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."