[elhoim]

BLUF

• Priority: medium
• Problem: query_hashlookup increments error_hash_counter, a local that is never assigned. The first event whose hash field is not exactly 64 characters (for example an MD5 in hash) raises UnboundLocalError, and the whole Hashlookup analysis ends in ERROR.
• Fix: Remove the dead counter increment. Nothing reads it, so the existing warning plus continue now skips the event as intended.
• Test: TestHashlookup.test_hash_wrong_length_is_skipped
• Verified: The new test fails on master with UnboundLocalError and passes with the fix. The module runs 3 passed, 2 skipped (both skips are pre-existing). black --check and pylint --rcfile=.pylintrc are clean on both changed files.

• What existing problem does this PR solve? query_hashlookup increments error_hash_counter, a local that is never assigned. The first event whose hash field is not exactly 64 characters (for example an MD5 in hash) raises UnboundLocalError, and the whole Hashlookup analysis ends in ERROR.
• What new feature is being introduced with this PR? None, this is a bug fix.
• Overview of changes to existing functions if required. Remove the dead counter increment. Nothing reads it, so the existing warning plus continue now skips the event as intended.

Details

Root cause. The wrong-length branch of timesketch/lib/analyzers/contrib/hashlookup_analyzer.py runs error_hash_counter += 1, but error_hash_counter is never assigned anywhere in the module. The analyzer query _exists_:hash OR ... also matches the generic hash field, and that field often holds an MD5 or SHA1. For any such event:

len(hash_value) != 64 → warning is logged → UnboundLocalError → run_wrapper marks the analysis as ERROR, and any events after it are never looked up.

How to reproduce. Use a timeline with one event where hash is d41d8cd98f00b204e9800998ecf8427e (an MD5), followed by an event with a valid 64-hex sha256_hash. Before the fix, the analyzer crashes on the first event. After the fix, it skips that event with the existing warning, queries Hashlookup once for the SHA256, and returns Hashlookup Matches: 1. The regression test covers exactly this case.

Fix. The change is one line: the dead increment is removed. Behaviour does not change for valid SHA256 values.

Possible follow-ups (left out of this PR to keep it narrow):

• An isinstance(hash_value, str) guard. A null or non-string field would raise TypeError at len(). This is unlikely, because _exists_ does not match nulls.
• A [0-9a-fA-F]{64} check before the value is placed into {hashlookup_url}sha256/{hash_value}, as hardening against path-segment injection. This would change behaviour, because 64-character non-hex values would no longer be sent.

I can add either one if you want it.

Checks

• All tests succeed.
• Unit tests added.
• e2e tests added.
• Documentation updated.

Closing issues

No issue is linked to this PR yet.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.