CVE-2023-22745: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: tpm2-tss is an open source software implementation of the Trusted Computing Group (TCG) Trusted Platform Module (TPM) 2 Software Stack (TSS2). In affected versions `Tss2_RC_SetHandler` and `Tss2_RC_Decode` both index into `layer_handler` with an 8 bit layer number, but the array only has `TPM2_ERROR_TSS2_RC_LAYER_COUNT` entries, so trying to add a handler for higher-numbered layers or decode a response code with such a layer number reads/writes past the end of the buffer. This Buffer overrun, could result in arbitrary code execution. An example attack would be a MiTM bus attack that returns 0xFFFFFFFF for the RC. Given the common use case of TPM modules an attacker must have local access to the target machine with local system privileges which allows access to the TPM system. Usually TPM access requires administrative privilege.
CWE description: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 6.4
Published: 2023-01-19T23:15Z
Repository: https://github.com/tpm2-software/tpm2-tss
Language: C
Files changed: src/tss2-rc/tss2_rc.c, test/unit/test_tss2_rc.c
Commit date: 2023-01-19 11:57:18 -0600
Commit message: tss2_rc: ensure layer number is in bounds

The layer handler array was defined as 255, the max number of uint8,
which is the size of the layer field, however valid values are 0-255
allowing for 256 possibilities and thus the array was off by one and
needed to be sized to 256 entries. Update the size and add tests.

Note: previous implementations incorrectly dropped bits on unknown error
output, ie TSS2_RC of 0xFFFFFF should yeild a string of 255:0xFFFFFF,
but earlier implementations returned 255:0xFFFF, dropping the middle
bits, this patch fixes that.

Fixes: CVE-2023-22745

Signed-off-by: William Roberts <william.c.roberts@intel.com>

Diff: diff --git a/src/tss2-rc/tss2_rc.c b/src/tss2-rc/tss2_rc.c
index e213d338..0f58e808 100644
--- a/src/tss2-rc/tss2_rc.c
+++ b/src/tss2-rc/tss2_rc.c
@@ -1,7 +1,10 @@
 /* SPDX-License-Identifier: BSD-2-Clause */
-
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+#include <assert.h>
 #include <stdarg.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <string.h>
 
@@ -844,11 +847,11 @@ tss_err_handler (TSS2_RC rc)
 
 
 static struct {
     char name[TSS2_ERR_LAYER_NAME_MAX];
     TSS2_RC_HANDLER handler;
-} layer_handler[TPM2_ERROR_TSS2_RC_LAYER_COUNT] = {
+} layer_handler[TPM2_ERROR_TSS2_RC_LAYER_COUNT + 1] = {
     ADD_HANDLER("tpm" , tpm2_ehandler),
     ADD_NULL_HANDLER,                       /* layer 1  is unused */
     ADD_NULL_HANDLER,                       /* layer 2  is unused */
     ADD_NULL_HANDLER,                       /* layer 3  is unused */
     ADD_NULL_HANDLER,                       /* layer 4  is unused */
@@ -879,11 +882,11 @@ static const char *
 unknown_layer_handler(TSS2_RC rc)
 {
     static __thread char buf[32];
 
     clearbuf(buf);
-    catbuf(buf, "0x%X", tpm2_error_get(rc));
+    catbuf(buf, "0x%X", rc);
 
     return buf;
 }
 
 /**
@@ -976,23 +979,31 @@ Tss2_RC_Decode(TSS2_RC rc)
         catbuf(buf, "%s:", lname);
     } else {
         catbuf(buf, "%u:", layer);
     }
 
-    handler = !handler ? unknown_layer_handler : handler;
-
     /*
      * Handlers only need the error bits. This way they don't
      * need to concern themselves with masking off the layer
      * bits or anything else.
      */
-    UINT16 err_bits = tpm2_error_get(rc);
-    const char *e = err_bits ? handler(err_bits) : "success";
-    if (e) {
-        catbuf(buf, "%s", e);
+    if (handler) {
+        UINT16 err_bits = tpm2_error_get(rc);
+        const char *e = err_bits ? handler(err_bits) : "success";
+        if (e) {
+            catbuf(buf, "%s", e);
+        } else {
+            catbuf(buf, "0x%X", err_bits);
+        }
     } else {
-        catbuf(buf, "0x%X", err_bits);
+        /*
+         * we don't want to drop any bits if we don't know what to do with it
+         * so drop the layer byte since we we already have that.
+         */
+        const char *e = unknown_layer_handler(rc >> 8);
+        assert(e);
+        catbuf(buf, "%s", e);
     }
 
     return buf;
 }
 
diff --git a/test/unit/test_tss2_rc.c b/test/unit/test_tss2_rc.c
index 9d61572f..1bfedbfe 100644
--- a/test/unit/test_tss2_rc.c
+++ b/test/unit/test_tss2_rc.c
@@ -195,11 +195,11 @@ test_custom_handler(void **state)
 
     /*
      * Test an unknown layer
      */
     e = Tss2_RC_Decode(rc);
-    assert_string_equal(e, "1:0x2A");
+    assert_string_equal(e, "1:0x100");
 }
 
 static void
 test_zero_length_name(void **state)
 {
@@ -405,10 +405,27 @@ test_info_str_null(void **state)
 {
     const char *m = Tss2_RC_DecodeInfoError(NULL);
     assert_null(m);
 }
 
+static void
+test_all_FFs(void **state)
+{
+    (void) state;
+
+    const char *e = Tss2_RC_Decode(0xFFFFFFFF);
+    assert_string_equal(e, "255:0xFFFFFF");
+}
+
+static void
+test_all_FFs_set_handler(void **state)
+{
+    (void) state;
+    Tss2_RC_SetHandler(0xFF, "garbage", custom_err_handler);
+    Tss2_RC_SetHandler(0xFF, NULL, NULL);
+}
+
 /* link required symbol, but tpm2_tool.c declares it AND main, which
  * we have a main below for cmocka tests.
  */
 bool output_enabled = true;
 
@@ -447,9 +464,11 @@ main(int argc, char* argv[])
             cmocka_unit_test(test_info_str_fmt1_ff),
             cmocka_unit_test(test_info_str_fmt0_err),
             cmocka_unit_test(test_info_str_fmt0_warn),
             cmocka_unit_test(test_info_str_fmt0_ff),
             cmocka_unit_test(test_info_str_null),
+            cmocka_unit_test(test_all_FFs),
+            cmocka_unit_test(test_all_FFs_set_handler)
     };
 
     return cmocka_run_group_tests(tests, NULL, NULL);
 }

Vulnerable code: 
} layer_handler[TPM2_ERROR_TSS2_RC_LAYER_COUNT] = {
    catbuf(buf, "0x%X", tpm2_error_get(rc));
    handler = !handler ? unknown_layer_handler : handler;

    UINT16 err_bits = tpm2_error_get(rc);
    const char *e = err_bits ? handler(err_bits) : "success";
    if (e) {
        catbuf(buf, "%s", e);
        catbuf(buf, "0x%X", err_bits);
    assert_string_equal(e, "1:0x2A");
Fixed code: #ifdef HAVE_CONFIG_H
#include "config.h"
#endif
#include <assert.h>
} layer_handler[TPM2_ERROR_TSS2_RC_LAYER_COUNT + 1] = {
    catbuf(buf, "0x%X", rc);
    if (handler) {
        UINT16 err_bits = tpm2_error_get(rc);
        const char *e = err_bits ? handler(err_bits) : "success";
        if (e) {
            catbuf(buf, "%s", e);
        } else {
            catbuf(buf, "0x%X", err_bits);
        }
        /*
         * we don't want to drop any bits if we don't know what to do with it
         * so drop the layer byte since we we already have that.
         */
        const char *e = unknown_layer_handler(rc >> 8);
        assert(e);
        catbuf(buf, "%s", e);
    assert_string_equal(e, "1:0x100");
static void
test_all_FFs(void **state)
{
    (void) state;

    const char *e = Tss2_RC_Decode(0xFFFFFFFF);
    assert_string_equal(e, "255:0xFFFFFF");
}

static void
test_all_FFs_set_handler(void **state)
{
    (void) state;
    Tss2_RC_SetHandler(0xFF, "garbage", custom_err_handler);
    Tss2_RC_SetHandler(0xFF, NULL, NULL);
}

            cmocka_unit_test(test_all_FFs),
            cmocka_unit_test(test_all_FFs_set_handler)