#80 Out of bounds accesses in bacnet_npdu_decode

[Anthony DeLorenzo]

Description

Multiple locations within bacnet_npdu_decode do not check for arithmetic overflows when decoding unsigned 16 bit integers from the untrusted npdu.

Example snippet

if (npdu[1] & BIT(5)) {
    if (pdu_len >= (len + 3)) {
        // !! Bug: len might wrap around to a negative value
        len += decode_unsigned16(&npdu[len], &dest_net);
       ..
        dlen = npdu[len++]; // !! Segfault
        if (dest) {
            dest->net = dest_net;
            dest->len = dlen;
        }

Impact

If the len marker wraps around to a negative value, then it will fail to be caught by subsequent checks, and will be treated as an unsigned value during memory de-references, causing out-of-bounds accesses. Since len is a signed integer, it can easily be overflowed by an unsigned uint16_t.

Remediation

Utilize safe arithmetic wrappers when parsing untrusted input:

bool safe_length_add(uint32_t op1, uint32_t op2, uint32_t max, uint32_t * out) {
    // Integer overflow
    if ((op1 + op2) < op1) {
        return false;
    }

    // Straight overflow
    if ((op1 + op2) > max) {
        return false;
    }

    *out = op1 + op2;
    return true;
}

[Anthony DeLorenzo]

Another caveat of this function that I'm sure you're aware of: the length value is MAX_NPDU when called through npdu_decode (such as in router-mstp).

int npdu_decode(uint8_t *npdu,
    BACNET_ADDRESS *dest,
    BACNET_ADDRESS *src,
    BACNET_NPDU_DATA *npdu_data)
{
    return bacnet_npdu_decode(npdu, MAX_NPDU, dest, src, npdu_data);
}

I'm assuming this is for compatibility, but wanted to point out the unsafe code path.

[Steve Karg]

I marked npdu_decode() function as deprecated in the library, and replaced its usage throughout the examples with bacnet_npdu_decode() which uses the apdu length as part of the decoding process to prevent buffer overreading.

See https://github.com/bacnet-stack/bacnet-stack/pull/447

[Anthony DeLorenzo]

After double checking this one, it looks like I mistakenly thought decode_unsigned16 returned the decoded value, but it actually returns the number of bytes consumed.

I'm going to leave this one open since I am worried about there being some hidden arithmetic overflows, but wanted to provide some updated thoughts. I'll do another pass and update the ticket this week.

[Steve Karg]

CVE-2023-38340