Skip to content

CLAM-2353 Abort signature load for short signature patterns#934

Merged
val-ms merged 2 commits intoCisco-Talos:mainfrom
val-ms:CLAM-2353-abort-for-bad-signature
Jun 13, 2023
Merged

CLAM-2353 Abort signature load for short signature patterns#934
val-ms merged 2 commits intoCisco-Talos:mainfrom
val-ms:CLAM-2353-abort-for-bad-signature

Conversation

@val-ms
Copy link
Contributor

@val-ms val-ms commented May 30, 2023

If a signature has a pattern that is too short will fail to load the siganture but does not cause the entire load process to abort. This is bad for two reasons:

  1. It is not immediately apparent that the signature is bad, and so it could be published accidentally.
  2. The signature is partially loaded by the time the bad pattern is observed and that may cause a crash later.

Because of (1), it is not worth it to try to unload the first part of the signature. Instead, we should just abort the signature load.

Fixes: #923

If a signature has a pattern that is too short will fail to load the
siganture but does not cause the entire load process to abort.
This is bad for two reasons:
1) It is not immediately apparent that the signature is bad, and so it
could be published accidentally.
2) The signature is partially loaded by the time the bad pattern is
observed and that may cause a crash later.

Because of (1), it is not worth it to try to unload the first part of the
signature. Instead, we should just abort the signature load.

Fixes: Cisco-Talos#923
@m-sola m-sola self-requested a review June 1, 2023 06:06
Copy link
Contributor

@m-sola m-sola left a comment

Choose a reason for hiding this comment

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

Testing current main showed:

LibClamAV Warning: Don't know how to create filter for: Win.Downloader.LNKAgent-10001628-0
LibClamAV Warning: cli_ac_addsig: cannot use filter for trie
Loading:     0s, ETA:   0s [========================>]        1/1 sigs
Compiling:   0s, ETA:   0s [========================>]       40/40 tasks

AddressSanitizer:DEADLYSIGNAL
=================================================================
==18764==ERROR: AddressSanitizer: SEGV on unknown address 0x7fda0a808fff (pc 0x7fd910cad7ec bp 0x7fff45018270 sp 0x7fff45018200 T0)
==18764==The signal is caused by a READ memory access.```

After changes:

```../../bin/clamscan -d Win.Downloader.LNKAgent-10001628-0.ldb ./s00234-023-03120-1.pdf
LibClamAV Warning: Don't know how to create filter for: Win.Downloader.LNKAgent-10001628-0
LibClamAV Warning: cli_ac_addsig: cannot use filter for trie
LibClamAV Error: cli_add_content_match_pattern: Problem adding signature (1).
LibClamAV Error: cli_add_content_match_pattern: Problem adding signature (1b).
LibClamAV Error: cli_loadldb: failed to parse subsignature 2 in Win.Downloader.LNKAgent-10001628-0
LibClamAV Error: Problem parsing database at line 1
LibClamAV Error: Can't load Win.Downloader.LNKAgent-10001628-0.ldb: Malformed database
ERROR: Malformed database     [                         ]        0/3 sigs

----------- SCAN SUMMARY -----------
Known viruses: 0
Engine version: 1.2.0-devel-20230601```

Fix works great, no issues there.

And this is certainly an edge case, but does it make sense to *not* print the loading bar when we have a malformed db?

We should also abort loading if the filter pattern for the boyer-moore
matcher is shorter than 2 bytes.

Also, do not print the final "Loading" progress bar if an error occured.
@val-ms
Copy link
Contributor Author

val-ms commented Jun 2, 2023

@m-sola good idea. I just pushed up a commit that adds a check if success before trying to print that loading progress bar. This way it doesn't print overtop the error message.

While looking at that I also found that the same logic of adding a filter (and failing on a pattern < 2 bytes) exists for the Boyer-Moore matcher as well. I don't have a test case to demonstrate it, but it definitely needs to fail out if a signature fails to load. So I added a return CL_EMALFDB; there as well.

@val-ms val-ms requested a review from m-sola June 2, 2023 17:46
Copy link
Contributor

@m-sola m-sola left a comment

Choose a reason for hiding this comment

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

Updated printout looks great!

@val-ms val-ms merged commit b778a6b into Cisco-Talos:main Jun 13, 2023
@val-ms val-ms added the 🍒cherry-pick-candidate A PR that should be backported once approved. label Jun 13, 2023
@val-ms val-ms deleted the CLAM-2353-abort-for-bad-signature branch June 13, 2023 01:05
@val-ms
Copy link
Contributor Author

val-ms commented Jun 13, 2023

Marking as a cherry-pick candidate because while this isn't super critical, it's safest to backport the fix for 1.0 LTS.

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.

Win.Downloader.LNKAgent-10001628-0 causing clamd to crash

2 participants