goToMain / libosdp

Implementation of IEC 60839-11-5 OSDP (Open Supervised Device Protocol); provides a C library with support for C++, Rust and Python3
https://libosdp.sidcha.dev
Apache License 2.0
130 stars 69 forks source link

overflow bugs #81

Closed qingkaishi closed 2 years ago

qingkaishi commented 2 years ago

https://github.com/goToMain/libosdp/blob/a3eb794b4599094ab6d6c57b3117c2fafdb9a22e/src/osdp_phy.c#L292-L312

There is an overflow bug in the code above.

Line 293 reads the value of pkt_len from a message.

Line 294 and Line 299 attempt to validate the packet length, pkt_len, which guarantees that pkt_len <= min(len, OSDP_PACKET_BUF_SIZE)

Let us assume pkt_len = 1 and reach Line 309, pkt_len-=2 leads to pkt_len = -1. At Line 311, pkt_len = -1 is passed into the function osdp_compute_crc16 via the second parameter, which is of the type size_t, an unsigned integer type.

Hence, in osdp_compute_crc16, pkt_len = -1 will be interpreted as a very large positive integer, thus buf[pkt_len] will overflow in osdp_compute_crc16.

The bug may be triggered by a malformed packet, and the above idea is validated by address sanitizer as follows:

==310842==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffcbdd12310 at pc 0x00000056f37f bp 0x7ffcbdd10e90 sp 0x7ffcbdd10e88
READ of size 1 at 0x7ffcbdd12310 thread T0
    #0 0x56f37e in crc16_itu_t /home/parallels/libosdp/src/osdp_common.c:28:11
    #1 0x56f479 in osdp_compute_crc16 /home/parallels/libosdp/src/osdp_common.c:38:9
    #2 0x572ece in osdp_phy_check_packet /home/parallels/libosdp/src/osdp_phy.c:312:10
    #3 0x55468f in pd_decode_packet /home/parallels/libosdp/src/osdp_pd.c:931:8

possible fix

add a check pkt_len >= 2 before Line 309

add a check pkt_len >= 1 before Line 319

sidcha commented 2 years ago

Thanks again, fixed this by checking if packet_len is at least as large as the header:

diff --git a/src/osdp_phy.c b/src/osdp_phy.c
index cd3cab0..083c008 100644
--- a/src/osdp_phy.c
+++ b/src/osdp_phy.c
@@ -296,7 +296,8 @@ int osdp_phy_check_packet(struct osdp_pd *pd, uint8_t *buf, int len,
                return OSDP_ERR_PKT_WAIT;
        }

-       if (pkt_len > OSDP_PACKET_BUF_SIZE) {
+       if (pkt_len > OSDP_PACKET_BUF_SIZE ||
+           (unsigned long)pkt_len < sizeof(struct osdp_packet_header)) {
                pd->reply_id = REPLY_NAK;
                pd->ephemeral_data[0] = OSDP_PD_NAK_CMD_LEN;
                return OSDP_ERR_PKT_NACK;