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

Clam 1643 cli_ac_addsig 2 byte over-read #611

Merged

Conversation

micahsnyder
Copy link
Contributor

  • Fix possible 2-byte overread when adding sig pattern

    It is possible to create a signature pattern that tries to add a
    zero-byte matching pattern to the A-C trie. A missing check at this
    stage can end up with a 2-byte overread when indexing the (empty)
    pattern to make sure the bytes added to the A-C trie are static and
    not both zero.

    This over read issue is not a vulnerability.

    This commit fixes the issue by adding a check for the pattern length.

    Resolves: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=43832

    Also added:

    • type casts and a "fall-through" comment to silence compile warnings.
    • a few additional length checks to protect against an additional 1-byte
      over read.
  • Add code comments to explain AC pattern prefix process

    When adding a pattern to the AC trie, checks are done to make sure the
    bytes that go in the AC trie don't have any ? wildcards and
    additionally that the first two bytes are not "\x00\x00".
    If they are, the position of the pattern that goes in the AC trie can be
    shifted right until a static pattern is identified that can go in the
    AC trie. Any bytes to the left of the new start of the pattern become a
    "prefix".

    During matching, once the AC trie match occurs and the bytes to the
    right of that pattern are matched, then the bytes from the prefix are
    matched.

    The reason that we don't want the bytes that go in the AC trie to start
    with "\x00\x00" is that it is such a common pattern in files that it
    would match constantly, and the scan process would spend a lot of time
    just checking through the list of patterns associated with a "\x00\x00"
    AC match, and that'd be crazy slow.
    But it is important to note that when shifting right, if there aren't
    enough nonzero, non-wildcard bytes to form a good prefix for the AC
    trie, that it is tolerable to bend the rule and let some patterns start
    with "\x00\x00". In that way, a small pattern like "0000ab" is still
    valid, and can be matched.

@micahsnyder micahsnyder requested a review from ragusaa June 4, 2022 20:13
It is possible to create a signature pattern that tries to add a
zero-byte matching pattern to the A-C trie. A missing check at this
stage can end up with a 2-byte overread when indexing the (empty)
pattern to make sure the bytes added to the A-C trie are static and
not both zero.

This over read issue is not a vulnerability.

This commit fixes the issue by adding a check for the pattern length.

Resolves: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=43832

Also added:
- type casts and a "fall-through" comment to silence compile warnings.
- a few additional length checks to protect against an additional 1-byte
over read.
When adding a pattern to the AC trie, checks are done to make sure the
bytes that go in the AC trie don't have any `?` wildcards and
additionally that the first two bytes are not "\x00\x00".
If they are, the position of the pattern that goes in the AC trie can be
shifted right until a static pattern is identified that can go in the
AC trie. Any bytes to the left of the new start of the pattern become a
"prefix".

During matching, once the AC trie match occurs and the bytes to the
right of that pattern are matched, then the bytes from the prefix are
matched.

The reason that we don't want the bytes that go in the AC trie to start
with "\x00\x00" is that it is such a common pattern in files that it
would match constantly, and the scan process would spend a lot of time
just checking through the list of patterns associated with a "\x00\x00"
AC match, and that'd be crazy slow.
But it is important to note that when shifting right, if there aren't
enough nonzero, non-wildcard bytes to form a good prefix for the AC
trie, that it is tolerable to bend the rule and let some patterns start
with "\x00\x00". In that way, a small pattern like "0000ab" is still
valid, and can be matched.
@micahsnyder
Copy link
Contributor Author

After rebase fuzz corpus test with the new sample passed.

@micahsnyder micahsnyder merged commit 7488787 into Cisco-Talos:main Jun 10, 2022
22 of 23 checks passed
@micahsnyder micahsnyder deleted the CLAM-1643-addsig-2-byte-read branch June 10, 2022 16:13
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

2 participants