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 2001 regex slash colon fixup #696

Merged

Conversation

micahsnyder
Copy link
Contributor

Fix issue preventing multiple LDB PCRE subsignatures

My recent fix for the issue where a '' followed by ':' in a Yara regex
string would fail to parse introduced a new issue that broke loading a
signature in the current daily.ldb database.

Unbeknownst to me at the time, you can have multiple PCRE subsignatures
in a logical signature, so long as they're the last subsignatures.
The previous fix made it so the signature parser muddled more than one
PCRE subsignature into one messed up regex string.

This commit essentially reverts the previous fix, while keeping some of
the code readability improvements in that function.
Instead, it addresses the problem a different way. To resolve the
original problem, I'm simply checking if the signame starts with "YARA".
If it does, we don't tokenize it by ':' delimiters.

Tests: Update LDB PCRE test per previous fix, and add more tests

I was unaware that while Yara rule regex strings may-or-may-not
escape '/' characters in the regex string, Clam logical sigs MUST escape
them. The Yara rule parser automatically removes the unnecessary '/':
https://github.com/Cisco-Talos/clamav/blob/clamav-0.105.1/libclamav/yara_lexer.l#L509-L514

That's a good feature, we don't want to remove that. But the Clam
logical sigs don't have an equivalent feature. So I changed the LDB
version of the regex '/' + ':' test to include the escape '/'
character.

This commit also adds some new tests to make sure we don't break support
for LDB sigs with multiple PCRE subsignatures in the future, and to test
that the offset feature and the case-insensitive feature work for PCRE
subsignatures.

My recent fix for the issue where a '\' followed by ':' in a Yara regex
string would fail to parse introduced a new issue that broke loading a
signature in the current daily.ldb database.

Unbeknownst to me at the time, you can have multiple PCRE subsignatures
in a logical signature, so long as they're the last subsignatures.
The previous fix made it so the signature parser muddled more than one
PCRE subsignature into one messed up regex string.

This commit essentially reverts the previous fix, while keeping some of
the code readability improvements in that function.
Instead, it addresses the problem a different way. To resolve the
original problem, I'm simply checking if the signame starts with "YARA".
If it does, we don't tokenize it by ':' delimiters.
I was unaware that while Yara rule regex strings may-or-may-not
escape '/' characters in the regex string, Clam logical sigs MUST escape
them. The Yara rule parser automatically removes the unnecessary '/':
https://github.com/Cisco-Talos/clamav/blob/clamav-0.105.1/libclamav/yara_lexer.l#L509-L514

That's a good feature, we don't want to remove that. But the Clam
logical sigs don't have an equivalent feature. So I changed the LDB
version of the regex '/' + ':' test to include the escape '\/'
character.

This commit also adds some new tests to make sure we don't break support
for LDB sigs with multiple PCRE subsignatures in the future, and to test
that the offset feature and the case-insensitive feature work for PCRE
subsignatures.
@micahsnyder micahsnyder merged commit 3dc50c8 into Cisco-Talos:main Sep 16, 2022
23 of 24 checks passed
@micahsnyder micahsnyder deleted the CLAM-2001-regex-slash-colon-fixup branch September 16, 2022 20:53
@micahsnyder micahsnyder added the 🍒cherry-pick-candidate A PR that should be backported once approved. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒cherry-pick-candidate A PR that should be backported once approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants