microsoft / ms-tpm-20-ref

Reference implementation of the TCG Trusted Platform Module 2.0 specification.
Other
333 stars 133 forks source link

Properly handle requests larger than INT32_MAX #91

Closed DemiMarie closed 1 year ago

DemiMarie commented 1 year ago

ExecuteCommand() is passed the request length as an unsigned 32-bit integer, but then immediately assigns it to command.parameterSize, which is a signed 32-bit integer. Therefore, if requestSize is greater than INT32_MAX, command.parameterSize will become negative.

TPMI_ST_COMMAND_TAG_Unmarshal will subtract 2 from command.parameterSize and fail if the result is negative. If command.parameterSize is between INT32_MIN+2 and 1 (inclusive), subtracting 2 from it will result in a negative value, causing TPMI_ST_COMMAND_TAG_Unmarshal to fail and the TPM to (correctly) reject the request. However, if requestSize is INT32_MAX+1 or INT32_MAX+2, command.parameterSize will be INT32_MIN or INT32_MIN+1 respectively. Subtracting 2 will then result in an integer overflow, which is undefined behavior. If the overflow wraps (either by chance or because of compiler options such as -fwrapv), the resulting length will be INT32_MAX-1 or INT32_MAX respectively. These are correct, so the command will be processed correctly, almost certainly being rejected by the MAX_COMMAND_SIZE check.

Furthermore, I do not believe any real TPM will accept requests this large. swtpm limits requests to 4096 bytes, and hardware TPMs are unlikely to have INT32_MAX bytes of RAM. Therefore, the overflow is likely impossible to trigger. Even if it does get triggered, it is probably harmless in practice. Therefore, I do not consider this to be a security vulnerability.

Fix this problem by clamping requestSize to MAX_COMMAND_SIZE+1. Add a preprocessor conditional to check that MAX_COMMAND_SIZE+1 will be handled correctly by both TpmFailureMode and the unmarshaling code. This is not the most elegant fix, but it is correct and requires no changes to the rest of the TPM.

bradlitterell commented 1 year ago

❤️ #error to remove extra complexity.