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

[Daniel McCarney]

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