Repository navigation
fix(yara): require standalone mining terms in crypto_coinjacking $wasm_miner - #648
Conversation
…m_miner The $wasm_miner string (/WebAssembly\.instantiate.*(mine|hash|crypto)/) has no word boundaries and no nocase-independent anchoring, so it fires on any WebAssembly.instantiate() call followed anywhere on the same line by the bare substrings "mine", "hash", or "crypto" — including mid-word matches (determine, combine) and ordinary, unrelated APIs (crypto.getRandomValues, a hashmap variable) that are common in legitimate WASM-loading code, especially minified bundles that keep a loader on one line. Narrow the alternation to standalone mine/miner/mining tokens with \b boundaries, matching the file's own convention elsewhere of using specific, unambiguous mining indicators (xmrig, cryptonight, stratum+tcp) rather than generic dictionary words. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @udsy19, thank you for your contribution to SkillSpector — we really appreciate the time you put into this! A few items need attention before it can be merged; details below.
Reviewed exact head 37ecca28c3d8915d9ea07ed46254e876db311420. The PR narrows crypto_coinjacking's $wasm_miner (src/skillspector/yara_rules/cryptominers.yar:97) from WebAssembly\.instantiate.*(mine|hash|crypto) to WebAssembly\.instantiate.*\b(mine|miner|mining)\b, and adds three negatives and one positive. I compiled the main and head versions of all built-in rules with yara-python 4.5.4 and scanned my own samples; contributor tests were not run. YARA supports \b, and the three reported false positives (hashmap, crypto.getRandomValues, determine) are cleared. However, the narrowing also removes the identifiers that real browser miners use.
Findings
-
[Blocking] Real WASM cryptominers are no longer detected. Browser cryptojackers ship Emscripten-compiled CryptoNight/RandomX modules. On the
WebAssembly.instantiateline they expose names likecryptonight,_cryptonight_hash,hash_cn,cn_hashorrandomx_*, not the English wordsmine/miner/mining. Each single-line loader below iscrypto_coinjackingon main; at head no built-in YARA rule fires:- CoinHive-style glue:
WebAssembly.instantiate(wasmBinary,info)…;var _cryptonight_hash=Module._cryptonight_hash=… - webminerpool-style:
…Module.cwrap("hash_cn",…) WebAssembly.instantiateStreaming(fetch("cryptonight.wasm"))…exports.cn_hash(blob,nonce++)WebAssembly.instantiate(randomxWasm).then(m=>m.instance.exports.randomx_calculate_hash(blob))
crypto_miner_softwareneeds two of its names, socryptonightorrandomxalone doesn't trigger it. Please keep mining-specific tokens next to the bounded words, e.g.cryptonight,randomx,cn_?hash/hash_?cn,hashrate/hashes_?per_?second. These are far more specific than barehash/crypto. Also consider matchingMiner/Miningcase-insensitively (new Miner(,startMining(), which main already misses. Add positive regressions for these shapes. The only positive today (m.exports.mine()) matches both the old and new expressions, so it cannot catch this regression. - CoinHive-style glue:
-
[Non-blocking] Adding
miningintroduces a new prose false positive.WebAssembly.instantiate(bytes).then(m=>runAnalytics(m)); // helpers for text miningis clean on main but CRITICAL at head. If you keepmining, consider a negative for data/text-mining wording. -
[Non-blocking] Bound the
.*.$wasm_minerstill triggers YARA'scontains .*compiler warning. The.{0,N}bound YARA suggests would also limit how far apartWebAssembly.instantiateand an unrelated word can be in a minified bundle.
Tests/CI: All six hosted checks pass, and the PR merges cleanly with main. #489 only changed webshells.yar plus tests, so there is no interaction. The three new negatives fail on main, so they are meaningful. Real miner shapes are not covered. I found no linked issue and no competing PR.
Decision: Changes Requested (reviewed head 37ecca28c3d8915d9ea07ed46254e876db311420)
| $minero_cc = "minero.cc" nocase | ||
| $monerominer = "monerominer" nocase | ||
| $wasm_miner = /WebAssembly\.instantiate.*(mine|hash|crypto)/ | ||
| $wasm_miner = /WebAssembly\.instantiate.*\b(mine|miner|mining)\b/ |
There was a problem hiding this comment.
Dropping bare hash/crypto is right for hashmap and crypto.getRandomValues, but it also drops the identifiers real CryptoNight/RandomX WASM miners expose. Each of these single-line loaders is crypto_coinjacking on main and has no built-in YARA finding at head:
_cryptonight_hashglueModule.cwrap("hash_cn", …)fetch("cryptonight.wasm")+exports.cn_hash(…)randomx_calculate_hash
Please add mining-specific tokens next to \b(mine|miner|mining)\b, e.g. cryptonight|randomx|cn_?hash|hash_?cn|hashrate. Optionally, replace .* with a bounded .{0,N}.
| findings = _run_builtin(content, "loader.js") | ||
| assert not _has_rule(findings, "crypto_coinjacking") | ||
|
|
||
| def test_wasm_instantiate_with_literal_miner_call_is_coinjacking(self): |
There was a problem hiding this comment.
This positive matches both the old and the new expression, so it cannot catch the recall regression. Please add positives for Emscripten CryptoNight/RandomX glue on the WebAssembly.instantiate line, e.g.:
_cryptonight_hashModule.cwrap("hash_cn", …)cryptonight.wasm+cn_hashrandomx_calculate_hash
The bounded \b(mine|miner|mining)\b alternation added to $wasm_miner correctly drops "hashmap"/"crypto.getRandomValues"/"determine", but real browser cryptojackers ship Emscripten-compiled CryptoNight/RandomX modules that expose names like _cryptonight_hash, hash_cn/cn_hash, or randomx_calculate_hash on the WebAssembly.instantiate line — none of which contain the English words mine/miner/mining, so they went undetected. Add mining-software-specific tokens (cryptonight, randomx, cn_hash/hash_cn, hashrate) as unbounded alternatives alongside the bounded English-word group. These tokens are distinctive enough to not need \b: in fact they must NOT be \b-bounded, since real loaders join them to surrounding identifiers with underscores (_cryptonight_hash, randomx_calculate_hash), and underscore is a regex word character, so a \b boundary would silently fail to match exactly these shapes. Add four regression positives covering the reviewer's own reported shapes (CoinHive-style _cryptonight_hash, webminerpool-style hash_cn via cwrap, cryptonight.wasm + cn_hash, randomx_calculate_hash). Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
Three follow-ups from review:
- nocase, so the CoinHive-era shapes match: new Miner(, startMining().
Bare hash/crypto stay out, so hashmap and crypto.getRandomValues are
still clean.
- mining now requires a call, mining\s*\(, rather than a bare word. That
keeps startMining( and drops the 'text mining' prose false positive
adding 'mining' had introduced.
- .{0,400} instead of .*, which bounds how far apart the instantiate call
and the identifier may sit in a minified bundle and clears YARA's
contains-.* compiler warning.
Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
|
Thanks @rng1995 — this is a much better review than the PR deserved, and the Fixed across two commits. The rule is now: 1 (blocking) — real miner identifiers. Added Each has a positive regression test. You were right that the old positive was Case-insensitivity. Added 2 (non-blocking) — the 3 (non-blocking) — bounding the Full file: 97 passed. The negatives still fail on main, so they remain One thing I should flag rather than leave you to find: I have not tested against |
CI lint job failed format-check on the text-mining-prose case added in the prior commit; ruff wants the two-line string literal joined. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
de31a0e to
77314a0
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @udsy19, thank you for the thorough follow-up and for your contribution to SkillSpector!
Re-reviewed head ca4f28524695a38e9ef9e80647a0a7af6e488d90. The earlier findings are addressed:
- [Blocking — fixed]
cryptonight,randomx,cn_?hash,hash_?cnandhashraterestore detection of the four CryptoNight/RandomX loader shapes, and each has a positive regression test.nocasealso catchesnew Miner(,startMining(and"RandomX", whichmainmissed. - [Non-blocking — fixed]
mining\s*\(drops the text-mining prose false positive, and a negative test covers it. - [Non-blocking — fixed]
.{0,400}replaces.*, and the rule now compiles with no YARA warnings.
With the rule set back to main, five of the new tests fail, so they guard the change.
DCO: the DCO check failed on your merge commit de31a0e because it has no Signed-off-by. It was a clean merge, so I reset the branch to your last signed commit 77314a0 and let GitHub's update-branch merge main again. The DCO check exempts GitHub-made merge commits. If you push again, please first reset your local branch to the new remote head (git fetch then git reset --hard origin/fix/yara-wasm-miner-fp), and use git commit -s for any future merges.
Optional follow-ups (non-blocking):
randomxwithnocasealso matches coordinate names such asp.randomX = Math.random()*w, whichmaindid not flag.randomx_would still cover the realrandomx_*API.main's.*still matched a miner token 1,000 characters afterinstantiate, but the new rule stops at 400. If real minified samples put the export table farther away, a larger bound may be worth it.hash_?cnalso matcheshash_cnt/hashCntcounters.mainflags these too, so this is not a regression.hash_?cn\bwould tighten it.
Tests/CI: test_static_yara.py passes locally on this head (108 passed). DCO Check passes, and the rest of CI is running.
Decision: Approved (reviewed head ca4f28524695a38e9ef9e80647a0a7af6e488d90)
Summary
crypto_coinjacking's$wasm_minerstring is/WebAssembly\.instantiate.*(mine|hash|crypto)/. The alternation has no wordboundaries, so it matches those three substrings anywhere on the line —
including inside unrelated identifiers and against ordinary Web APIs that
commonly sit next to a legitimate
WebAssembly.instantiate()call. Any suchfile is reported as CRITICAL cryptojacking.
Impact: silent-wrong-result
Who reaches it: any user running the static YARA analyzer over JavaScript that
loads WebAssembly — the default built-in ruleset, no configuration needed.
Minified bundles are hit hardest, because they keep the loader and unrelated
code on one line, which is exactly what
.*spans.Three benign inputs that are flagged today:
Changes
\b(mine|miner|mining)\b. This dropshashandcryptoas bare triggers,since neither is evidence of mining on its own, and prevents mid-word matches
such as
determine.TestBuiltInCryptominerRulescovering the three false positives aboveand a true positive (
m.exports.mine()), so the narrowing cannot silentlydisable real detection.
Validation
Negative control: with the rule change reverted and the tests left in place,
the three false-positive cases fail while the true-positive case still passes.
Without the fix:
With the fix:
Signed-off-by: Udaya Tejas udayatejas2004@gmail.com