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.

Sign up for GitHub

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

Jump to bottom

integer wraparound in tlv functions #678

Closed
setharnold opened this issue Mar 25, 2022 · 3 comments
Closed

integer wraparound in tlv functions #678

setharnold opened this issue Mar 25, 2022 · 3 comments

Comments

@setharnold
Copy link

swtpm/src/swtpm/tlv.c

Line 138 in 2579038

td->tlv.length = be32toh(td->tlv.length);

Hello, in tlv_data_find_tag() there's a check for td->tlv.length being slightly out of bounds; what happens if it is large enough that adding it to offset causes an integer wraparound?

        td->tlv.length = be32toh(td->tlv.length);
        if (offset + td->tlv.length > buffer_len)
            return NULL;

tlv_data_append() does something similar.

Are these tlv functions working on entirely trusted data?

Thanks
stefanberger added a commit that referenced this issue Mar 26, 2022
@stefanberger
swtpm: Use uint64_t for offset to compare against uint32_t
f150f81 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding an
untrusted 32bit number will allow for comparison against trusted 32bit
number (buffer_len):

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted input data.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
@stefanberger
Copy link
Owner

Thanks for reporting this one. Yes, there may be an integer wrap-around occurring in this function that can then cause crashes when reading from memory locations. Please have a look at the PR that for now only resolves the issue in tlv_data_find_tag().

@stefanberger stefanberger mentioned this issue Mar 26, 2022
Merged
stefanberger added a commit that referenced this issue Mar 28, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
a6b135d 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted input data that have a tlv.length causing an integer
overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit that referenced this issue Mar 28, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
a06ccb0 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted TPM state input data that have a tlv.length that is so
large that is causes an integer overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
@stefanberger
Copy link
Owner

Are these tlv functions working on entirely trusted data?

I would say they work on data that are as trusted as the pflash files for EFI code and variables and the virtual machine image(s)... they should be susceptible to malicious root only.

@setharnold
Copy link
Author

Cool, that's good news. The pull request looks good to me.

Thanks for the quick turnarounds!

stefanberger added a commit that referenced this issue Mar 30, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
43c816c 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted TPM state input data that have a tlv.length that is so
large that is causes an integer overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit that referenced this issue Mar 30, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
245bfb7 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted TPM state input data that have a tlv.length that is so
large that is causes an integer overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit that referenced this issue Mar 30, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
3c7b429 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted TPM state input data that have a tlv.length that is so
large that is causes an integer overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit that referenced this issue Mar 30, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
d60f630 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted TPM state input data that have a tlv.length that is so
large that is causes an integer overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit that referenced this issue Mar 30, 2022
@stefanberger
swtpm: Use uint64_t to avoid integer wrap-around when adding a uint32_t
2ee76e9 
To avoid an integer wrap-around use uint64_t for 'offset' so that adding
an untrusted 32-bit number will allow for comparison against the trusted
'buffer_len' 32-bit number:

        if (offset + td->tlv.length > buffer_len)
            return NULL;

This avoids possible out-of-bound accesses and crashes when reading
specially crafted TPM state input data that have a tlv.length that is so
large that is causes an integer overflow.

Resolves: #678
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@stefanberger @setharnold