Bug with data unmarshaling and buffer read overrun in SecurityPkg/Library/Tpm2CommandLib/Tpm2Object.c

[wmjdgla]

edk2/SecurityPkg/Library/Tpm2CommandLib/Tpm2Object.c

Lines 162 to 190 in 8f698f0

	switch (OutPublic->publicArea.type) {
	case TPM_ALG_KEYEDHASH:
	OutPublic->publicArea.parameters.keyedHashDetail.scheme.scheme = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	switch (OutPublic->publicArea.parameters.keyedHashDetail.scheme.scheme) {
	case TPM_ALG_HMAC:
	OutPublic->publicArea.parameters.keyedHashDetail.scheme.details.hmac.hashAlg = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	break;
	case TPM_ALG_XOR:
	OutPublic->publicArea.parameters.keyedHashDetail.scheme.details.xor.hashAlg = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	OutPublic->publicArea.parameters.keyedHashDetail.scheme.details.xor.kdf = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	break;
	default:
	return EFI_UNSUPPORTED;
	}
	case TPM_ALG_SYMCIPHER:
	OutPublic->publicArea.parameters.symDetail.algorithm = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	switch (OutPublic->publicArea.parameters.symDetail.algorithm) {
	case TPM_ALG_AES:
	OutPublic->publicArea.parameters.symDetail.keyBits.aes = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	OutPublic->publicArea.parameters.symDetail.mode.aes = SwapBytes16 (ReadUnaligned16 ((UINT16 *)Buffer));
	Buffer += sizeof (UINT16);
	break;

case TPM_ALG_KEYEDHASH is missing the break statement at the end (line 180), causing the code execution to flow to the next case (TPM_ALG_SYMCIPHER). OutPublic->publicArea.parameters.keyedHashDetail is a union with OutPublic->publicArea.parameters.symDetail, so this means the unmarshaled data in case TPM_ALG_KEYEDHASH would be overwritten in case TPM_ALG_SYMCIPHER.

In addition, the buffer pointer is advanced in each TPM_ALG_XXX case, so the incorrect flow through to case TPM_ALG_SYMCIPHER would advance buffer more than it should, causing subsequent unmarshalling to go beyond the correct bounds.

[jyao1]

Yes, agree this is a bug. Do you want to propose a patch?