Reject secured messages with invalid descriptor count

[simons-hub]

Summary

In libspdm_storage_secured_message_decode(), the guard on num_descriptors only rejected values greater than 1 but allowed 0. When num_descriptors is 0, the for-loop never executes, leaving spdm_msg_offset uninitialized. This uninitialized value is then used as a pointer offset to compute *message - undefined behavior on malformed input.

Defense-in-depth hardening fix by changing if (num_descriptors > 1) to if (num_descriptors != 1), which matches the existing code comments and the encode side (always writes num_descriptors = 1).

Detected using cppcheck with --check-level=exhaustive flagged uninitvar on spdm_msg_offset

Test plan

• cppcheck uninitvar warning eliminated after fix
• Existing CI tests pass

[simons-hub]

Looked into the single failing windows job.

test_crypt exits with code 1 and zero output (no test results printed), indicating a likely runtime environment crash.

I built the similar native Windows test environment locally with the same toolchain and build config: confirmed it passes most of the time but caught another noisy failure (55/56 pass of test_crypt).

Local Windows verification

Built and tested locally on native Windows with the same toolchain and build config as CI:
Commit: 2105a6d Reject secured messages with invalid descriptor count
Platform: Windows 11 (MSYS2 MINGW64)
GCC: gcc.exe (Rev2, Built by MSYS2 project) 14.2.0
Generator: MSYS Makefiles (same as CI)
Config: ARCH=x64 TOOLCHAIN=GCC TARGET=Release CRYPTO=openssl
CFLAGS: all LIBSPDM_ENABLE_CAPABILITY_*=0, ASSERT_CONFIG=3

Full test suite:
test_spdm_requester: PASS (5 tests)
test_spdm_responder: PASS (7 tests)
test_spdm_common: PASS (22 tests)
test_spdm_crypt: PASS (11 tests)
test_crypt: PASS (exit 0)

test_crypt repeated: 55/56 PASS (1 flake at ~2% rate)

Based on this - failure is likely unrelated to the PR. So will try pushing an empty commit to re-kick CI.

[steven-bellock]

The test failure is a known issue. @simons-hub please remove 10317df from the commit history.

@twilfredo and @alistair23 for visibility.

[simons-hub]

The test failure is a known issue. @simons-hub please remove 10317df from the commit history.

@twilfredo and @alistair23 for visibility.

Thanks for taking a look @steven-bellock

Done - 10317df has been removed.

[twilfredo]

good catch! but we should probably handle the num_descriptors == 0 case (iirc this is allowed)? so perhaps an early return with the message buffer offset into the correct place?

Since message starts at the num_descriptors field, something like;

if (!num_descriptors) {
    *message = ((uint8_t *)*message) + 4; <-- define as macro perhaps
    *message_size -= 4;
     return LIBSPDM_STATUS_SUCCESS;
}

[simons-hub]

Thanks @twilfredo,

Taking another look at DSP0286 Table 8, it seems to rule out num_descriptors == 0 as a valid case:

"An SPDM Storage Secured Message shall contain exactly one descriptor from the Command category and may contain up to one descriptor from the Data Buffer category."

Maybe we should distinguish this case as a true INVALID_MSG_FIELD error, while keeping UNSUPPORTED_CAP for > 1 case (which can go away in the future if > 1 becomes supported)?

  /* A valid secured message shall contain at least one descriptor (DSP0286 Table 8) */
  if (!num_descriptors) {
      spdm_error.session_id = session_id;
      spdm_error.error_code = SPDM_STORAGE_SECURED_MSG_ENCAPSULATED_STATUS_INVALID_CMD;
      libspdm_set_last_spdm_error_struct(spdm_context, &spdm_error);
      return LIBSPDM_STATUS_INVALID_MSG_FIELD;
  }

  /* Currently support handling only a single SPDM descriptor */
  if (num_descriptors > 1) {
      spdm_error.session_id = session_id;
      spdm_error.error_code = SPDM_STORAGE_SECURED_MSG_ENCAPSULATED_STATUS_INVALID_CMD;
      libspdm_set_last_spdm_error_struct(spdm_context, &spdm_error);
      return LIBSPDM_STATUS_UNSUPPORTED_CAP;
  }

That said, happy to go with your suggestion of treating 0 as valid if you prefer (though probably needs an underflow check before *message_size -= 4). Let me know.

[twilfredo]

Thanks @twilfredo,

Taking another look at DSP0286 Table 8, it seems to rule out num_descriptors == 0 as a valid case:

"An SPDM Storage Secured Message shall contain exactly one descriptor from the Command category and may contain up to one descriptor from the Data Buffer category."

Maybe we should distinguish this case as a true INVALID_MSG_FIELD error, while keeping UNSUPPORTED_CAP for > 1 case (which can go away in the future if > 1 becomes supported)?

  /* A valid secured message shall contain at least one descriptor (DSP0286 Table 8) */
  if (!num_descriptors) {
      spdm_error.session_id = session_id;
      spdm_error.error_code = SPDM_STORAGE_SECURED_MSG_ENCAPSULATED_STATUS_INVALID_CMD;
      libspdm_set_last_spdm_error_struct(spdm_context, &spdm_error);
      return LIBSPDM_STATUS_INVALID_MSG_FIELD;
  }

  /* Currently support handling only a single SPDM descriptor */
  if (num_descriptors > 1) {
      spdm_error.session_id = session_id;
      spdm_error.error_code = SPDM_STORAGE_SECURED_MSG_ENCAPSULATED_STATUS_INVALID_CMD;
      libspdm_set_last_spdm_error_struct(spdm_context, &spdm_error);
      return LIBSPDM_STATUS_UNSUPPORTED_CAP;
  }

That said, happy to go with your suggestion of treating 0 as valid if you prefer (though probably needs an underflow check before *message_size -= 4). Let me know.

@simons-hub Thanks for checking with the spec! Let's do it your way that seems better specially since the spec seems to rule out the 0 case.

[simons-hub]

Updated with the num_descriptors == 0 and num_descriptors > 1 cases handled separately.

[twilfredo]

with @steven-bellock comment addressed, lgtm. Thanks for the patch!