#79 Out of bounds jump in h_apdu.c:apdu_handler

[Anthony DeLorenzo]

Description

The function apdu_handler (src/bacnet/basic/service/h_apdu.c:544) indexes into various function tables to forward the apdu to its requested service handler. The index used is an 8-bit integer pulled from the attacker-controlled apdu (service_choice) and is not checked against the maximum array length for PDU_TYPE_SIMPLEE_ACK and PDU_TYPE_COMPLEX_ACK service requests:

Bug Snippet

void apdu_handler(BACNET_ADDRESS *src,
    uint8_t *apdu, /* APDU data */
    uint16_t apdu_len)
{
    case PDU_TYPE_COMPLEX_ACK:
        if (apdu_len < 3) {
            break;
        }
        ...
        service_choice = apdu[len++];
        service_request = &apdu[len];
        service_request_len = apdu_len - (uint16_t)len;
        if (!apdu_confirmed_simple_ack_service(service_choice)) {
            if (Confirmed_ACK_Function[service_choice]
                    .complex != NULL) { 

                    // !! BUG HERE
                    // service_choice could be greater than Confirmed_ACK_Function
                Confirmed_ACK_Function[service_choice].complex(
                    service_request, service_request_len, src,
                    &service_ack_data);
            }
            ...
        }
        break;
}

Example crash

==59399==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55caa8b73d58 at pc 0x55caa84fdbb1 bp 0x7ffeb78ae790 sp 0x7ffeb78ae788
READ of size 8 at 0x55caa8b73d58 thread T0
#0 0x55caa84fdbb0 in apdu_handler /mnt/net/lab_share/bacnet-stack/src/bacnet/basic/service/h_apdu.c:644:30
    #1 0x55caa849b27b in my_routing_npdu_handler /mnt/net/lab_share/bacnet-stack/apps/router-mstp/main.c:1095:25

 0x55caa8b73d58 is located 8 bytes to the left of global variable 'Error_Function' defined in '/mnt/net/lab_share/bacnet-stack/src/bacnet/basic/service/h_apdu.c:316' (0x55caa8b73d60) of size 272
0x55caa8b73d58 is located 72 bytes to the right of global variable 'Confirmed_ACK_Function' defined in '/mnt/net/lab_share/bacnet-stack/src/bacnet/basic/service/h_apdu.c:252' (0x55caa8b73c00) of size 272

Example Payload:
00000000: 55ff 050f 0000 166f 010c aacc 06ac 1006  U......o........
00000010: 1fba c130 0323 0c0c 0140 0001 1955 db53  ...0.#...@...U.S

Impact

An attacker could abuse this flaw to achieve arbitrary code execution on the vulnerable device. Given that this is an out-of-bounds call, the steps to achieve code execution are drastically reduced. This out-of-bounds is far enough to index into the TSM_List, which could contain untrusted data allowing an attacker to call an arbitrary address .

Remediation

Employ guard checks against the maximum length of the array:

                // Length Check
                if (service_choice < sizeof(Confirmed_ACK_Function)) {
                    if (Confirmed_ACK_Function[service_choice]
                            .complex != NULL) {                             
                        Confirmed_ACK_Function[service_choice].complex(
                            service_request, service_request_len, src,
                            &service_ack_data);
                    }
                }

[Anthony DeLorenzo]

This was meant to be placed in v1.1.1 !

[Steve Karg]

The inverted check for valid values didn't check for valid values in the correct array range. I added array bounds checking in those cases.

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

[Steve Karg]

CVE-2023-38339