Native Format Parsing - Integer Overflow Resizing Arrays#33024
Closed
varadarajkumar wants to merge 7 commits intoClickHouse:masterfrom
Closed
Native Format Parsing - Integer Overflow Resizing Arrays#33024varadarajkumar wants to merge 7 commits intoClickHouse:masterfrom
varadarajkumar wants to merge 7 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-bugfix
|
|
Signed-off-by: Boris Kuschel <[email protected]>
src/Common/PODArray.h
Outdated
|
|
||
| static size_t byte_size(size_t num_elements) | ||
| { | ||
| if (num_elements > SIZE_MAX/ELEMENT_SIZE) |
Member
There was a problem hiding this comment.
Better to use __builtin_mul_overflow for this purpose.
Contributor
Author
There was a problem hiding this comment.
Hi Alexey, I have updated the code as per your suggestion. Thanks
alexey-milovidov
added
the
can be tested
src/Common/PODArray.h
Outdated
| static size_t byte_size(size_t num_elements) | ||
| { | ||
| size_t test; | ||
| if (__builtin_mul_overflow(num_elements,ELEMENT_SIZE, &test)) |
Member
There was a problem hiding this comment.
Suggested change
| if (__builtin_mul_overflow(num_elements,ELEMENT_SIZE, &test)) | |
| if (__builtin_mul_overflow(num_elements, ELEMENT_SIZE, &test)) |
src/Common/PODArray.h
Outdated
| if (__builtin_mul_overflow(num_elements,ELEMENT_SIZE, &test)) | ||
| throw Exception("Amount of memory requested to allocate is more than allowed", ErrorCodes::CANNOT_ALLOCATE_MEMORY); | ||
| else | ||
| return (num_elements * ELEMENT_SIZE); |
Member
There was a problem hiding this comment.
Suggested change
| return (num_elements * ELEMENT_SIZE); | |
| return num_elements * ELEMENT_SIZE; |
src/Common/PODArray.h
Outdated
| { | ||
| size_t test; | ||
| if (__builtin_mul_overflow(num_elements,ELEMENT_SIZE, &test)) | ||
| throw Exception("Amount of memory requested to allocate is more than allowed", ErrorCodes::CANNOT_ALLOCATE_MEMORY); |
Member
There was a problem hiding this comment.
Suggested change
| throw Exception("Amount of memory requested to allocate is more than allowed", ErrorCodes::CANNOT_ALLOCATE_MEMORY); | |
| throw Exception("Amount of memory requested to allocate is more than allowed", ErrorCodes::CANNOT_ALLOCATE_MEMORY); |
Contributor
Author
There was a problem hiding this comment.
Hi @alexey-milovidov, Thanks, updated the code now.
src/Common/PODArray.h
Outdated
| if (__builtin_mul_overflow(num_elements,ELEMENT_SIZE, &test)) | ||
| throw Exception("Amount of memory requested to allocate is more than allowed", ErrorCodes::CANNOT_ALLOCATE_MEMORY); | ||
| else | ||
| return (num_elements * ELEMENT_SIZE); |
Member
There was a problem hiding this comment.
Why don't return test?
Merged
Trailing space Removed trailing whitespaces
alexey-milovidov
added a commit
that referenced
this pull request
Dec 23, 2021
Member
|
Merged here: #33024 |
robot-clickhouse
pushed a commit
that referenced
this pull request
Dec 23, 2021
robot-clickhouse
pushed a commit
that referenced
this pull request
Dec 23, 2021
robot-clickhouse
pushed a commit
that referenced
this pull request
Dec 23, 2021
robot-clickhouse
pushed a commit
that referenced
this pull request
Dec 23, 2021
robot-clickhouse
pushed a commit
that referenced
this pull request
Dec 23, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Integer overflow to resize the arrays causes heap corrupt
Detailed description / Documentation draft:
Problem Description:
Attacker can take advantage and abuse the overflow of integer to write to the heap.
the argument - num_elements to function byte_size function which is a 64-bit unsigned integer on the machines and it can
overflow due to the multiplication by ELEMENT_SIZE and/or the addition of the padding sizes.
The result of this arithmetic is passed to the allocation (realloc) that is expected to hold
num_elements items. If an overflow occurred during these arithmetic operations, then the
PODArray instance will believe the array can hold a much larger amount than was actually
allocated. When those items are initialized they will overwrite adjacent data on the heap.
Solution proposed:
As part of fix to address Integer overflow, proper check added in PODArray.h to prevent integer overflow conditions prior to allocations or copy operations using the results of the calculations. and also Throwing exception when CH requests for larger than MAX_SIZE.
Committer: Rajkumar Varada