#81 Multiple out-of-bounds accesses in bacerror code paths

[Anthony DeLorenzo]

Description

Code paths below bacerror_decode_error_class_and_code do not check if memory accesses go beyond the end of the apdu buffer. I have observed crashes in both decode_tag_number_and_value() and decode_enumerated() child function calls.

Example:

/* decode the application class and code */
int bacerror_decode_error_class_and_code(uint8_t *apdu,
    unsigned apdu_len,
    BACNET_ERROR_CLASS *error_class,
    BACNET_ERROR_CODE *error_code)
{
    int len = 0;
    ...
    if (apdu && apdu_len) {
        // !! Bug: No way for decode_tag_number_and_value to check against the end of the apdu
        len += decode_tag_number_and_value(
            &apdu[len], &tag_number, &len_value_type);
        if (tag_number != BACNET_APPLICATION_TAG_ENUMERATED) {
            return 0;
        }

Stack Traces

==687152==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200093d7f6 at pc 0x5593967bbb46 bp 0x7fff4a770f50 sp 0x7fff4a770f48
READ of size 1 at 0x60200093d7f6 thread T0
    #0 0x5593967bbb45 in decode_tag_number_and_value /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacdcode.c:496:13
    #1 0x5593967cc424 in bacerror_decode_error_class_and_code /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacerror.c:78:16
    #2 0x559396803b9c in apdu_handler /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/basic/service/h_apdu.c:699:27
    #3 0x5593967a084b in my_routing_npdu_handler /mnt/net/lab_share/Bacnet/bacnet-stack/apps/router-mstp/main.c:1122:25


==693132==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200214a0d6 at pc 0x5576ae8565b9 bp 0x7fffbee3ab80 sp 0x7fffbee3ab78
READ of size 1 at 0x60200214a0d6 thread T0
    #0 0x5576ae8565b8 in decode_unsigned24 /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacint.c:78:42
    #1 0x5576ae8487ce in bacnet_unsigned_decode /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacdcode.c:1737:17
    #2 0x5576ae8496a6 in bacnet_enumerated_decode /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacdcode.c:2036:9
    #3 0x5576ae8496a6 in decode_enumerated /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacdcode.c:2059:12
    #4 0x5576ae855efd in bacerror_decode_error_class_and_code /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/bacerror.c:83:16
    #5 0x5576ae88d61c in apdu_handler /mnt/net/lab_share/Bacnet/bacnet-stack/src/bacnet/basic/service/h_apdu.c:699:27
    #6 0x5576ae8295d9 in my_routing_npdu_handler /mnt/net/lab_share/Bacnet/bacnet-stack/apps/router-mstp/main.c:1036:25

Impact

An attacker could abuse this to possibly achieve arbitrary code execution. Difficulty of exploitation would depend on the target environment, since this library is designed to be deployed onto varying platforms. Additionally, these all seem to be out-of-bound reads, which are less worrysome when compared to writes.

Remediation

I believe API changes will have to be made to inform the lower parsing layers of the maximum space left in the apdu buffer:

        len += decode_tag_number_and_value(
            &apdu[len], apdu_len, &tag_number, &len_value_type);
            //          ^^^^^^^^

[Steve Karg]

CVE-2023-38341

[Steve Karg]

Fixed bacerror_decode_error_class_and_code() function by creating/modifying all the context and application codecs for the 13 primitive value to securely decode the APDU. See:
https://sourceforge.net/p/bacnet/src/ci/f641aacddb0fc718867ecf1eefe3908a810480b9/