Clam 1607 fix benign overflow & leaks loading PDB & WDB databases#530
Merged
val-ms merged 1 commit intoCisco-Talos:mainfrom May 17, 2022
Merged
Clam 1607 fix benign overflow & leaks loading PDB & WDB databases#530val-ms merged 1 commit intoCisco-Talos:mainfrom
val-ms merged 1 commit intoCisco-Talos:mainfrom
Conversation
ragusaa
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
dfd17ca to
c806c45
Compare
Contributor
Author
|
This PR replaces #462 |
ragusaa
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
c806c45 to
0fa734a
Compare
val-ms
reviewed
Apr 11, 2022
val-ms
requested changes
Apr 11, 2022
Contributor
val-ms
left a comment
val-ms
left a comment
There was a problem hiding this comment.
1 error handling thing to tidy up, otherwise is good
ragusaa
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
1d18874 to
ef717ae
Compare
val-ms
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
ef717ae to
cda9fd5
Compare
Contributor
|
From offline conversations: this PR is presently held up because the cleanup code added is slow and is causing timeouts in the test environment. @ragusaa is working on finding a faster way to track those pointers for cleanup. |
ragusaa
force-pushed
the
CLAM-1607-LoadingSig3
branch
4 times, most recently
from
6060484 to
496b4f6
Compare
ragusaa
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
36ee4bd to
9fc8db7
Compare
val-ms
requested changes
May 14, 2022
Contributor
There was a problem hiding this comment.
With exception to these minor changes requested, this all looks really great. It went through the test pipelines nicely. I used the internal-fuzz-corpus PR to verify that it resolves CLAM-1607 and the related ones (1642, 1649, 1653) and with manual testing, confirmed that 1691 is resolved as well.
val-ms
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
9fc8db7 to
cb85b4b
Compare
Contributor
|
Just rebased for you to same some effort, since I did it for testing locally. |
There is a possible overflow read when loading PDB and WDB phishing signatures. This issue is not a vulnerability. Changed const char pointers to uint8_t pointers when they are to be used with data, as well as removing asserts and adding additional error checking. Thank you Michał Dardas for reporting this issue.
ragusaa
force-pushed
the
CLAM-1607-LoadingSig3
branch
from
d0de311 to
5ccb0c3
Compare
val-ms
changed the title
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
2 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.
Fixes a benign overflow loading PDB or WDB databases, reported by Michał Dardas.
This fix also resolves:
This commit also fixes a minor leak of pattern matching trans nodes that was observed when testing with the MPOOL module disabled.
The fix changed
const charpointers touint8_tpointers when they are to be used with data, as well as removing asserts and adding additional error checking.