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 possible integer overflow in license_read_scope_list() #1649

Closed

Conversation

sidhpurwala-huzaifa
Copy link

On 32-bit arches sizeof(LICENSE_BLOB) * scopeCount > SIZE_MAX and cause an overflow. This will result in a buffer-overflow later in license_read_binary_blob.

Not sure if this cause a server crash or a client crash though

@freerdp-bot
Copy link

Can one of the members verify this patch?

1 similar comment
@freerdp-bot
Copy link

Can one of the members verify this patch?

@bmiklautz
Copy link
Member

@freerdp-bot test.

@freerdp-bot
Copy link

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/100/

@@ -670,6 +670,9 @@ BOOL license_read_scope_list(wStream* s, SCOPE_LIST* scopeList)

Stream_Read_UINT32(s, scopeCount); /* ScopeCount (4 bytes) */

if (Stream_GetRemainingLength(s) / sizeof(LICENSE_BLOB) < scopeCount)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sidhpurwala-huzaifa the check is incorrect as sizeof(LICENSE_BLOB) is the size of the memory structure representing a LICENSE_BLOB, not the size of the BLOB itself

Copy link

Choose a reason for hiding this comment

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

@hardening This was actually intended because sizeof(LICENSE_BLOB) is used in the allocation below. The packet parsing routine has another check against the remaining length of the stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fweimer I disagree, it doesn't make any sense to check lengths between wire data and in memory ones. The check above is too restrictive as sizeof(LICENSE_BLOB) is bigger than 4, and in theory we could have a serie of 0 length blobs and in that case each blob would be 4 bytes in the stream.
The best fix would probably be to put a limit of the acceptable number of blobs as obviously it will never be 2^32.

@bmiklautz
Copy link
Member

@hardening any conclusion on this?

@awakecoding
Copy link
Member

The latest code does the following check:

if (scopeCount > Stream_GetRemainingLength(s) / 4) /* every blob is at least 4 bytes */
return FALSE;

Which is different from the original check that was done at the time of this pull request. It appears to be correct according to the discussion.

As for imposing an upper limit on the number of blobs, I think we'd rather go over all of license.c and add proper checks for out of memory errors. Even if we impose an upper limit on the number of blobs we could still have very large blobs. The real issue here is the lack of OOM error checking in multiple places.

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.

None yet

6 participants