Uh oh!
There was an error while loading. Please reload this page.
Fix agent going inactive on --increment speed benchmarks - #71
Open
urbanadventurer wants to merge 2 commits into
Open
Fix agent going inactive on --increment speed benchmarks#71urbanadventurer wants to merge 2 commits into
urbanadventurer wants to merge 2 commits into
Conversation
hashcat refuses to run --progress-only together with --increment
("Increment is not allowed in combination with --progress-only."),
so the speed benchmark exited non-zero for every increment task. The
agent sent a clientError, and the server deactivates an agent on its
first error when ignoreErrors=0 (the default), leaving the agent stuck
Inactive until re-enabled by hand.
Strip the increment flags before running --progress-only; the measured
cracking speed is independent of the increment range, so the benchmark
stays valid. Add strip_increment() with unit tests.
strip_increment iterates the tokens directly and must NOT route them
through clean_list(): the assembled benchmark command contains runs of
spaces, and clean_list() deletes real tokens when it hits consecutive
empty entries (it mutates the list while iterating it). That bug dropped
the attack mask entirely (e.g. -a 3 ?d?d?d?d?d became bare -a 3), after
which hashcat fell back to its default ?1 mask and failed with
"Custom-charset 1 is undefined".
Also capture hashcat's real output on failure: 'output' was reset to
b'' before the try block, so CalledProcessError.output was discarded and
the error log line was always empty. Read e.output instead.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Found while fixing the speed-benchmark increment issue: * clean_list() deleted entries from a list while iterating it, which skips elements and leaves real tokens (and empty strings) behind. Rewrite it as a filtered comprehension. This also stops get_wordlist() from raising IndexError on the empty strings clean_list used to leave. * measure_keyspace() reset 'output' to b'' before the try, so on failure it logged an always-empty Output and discarded hashcat's real message in CalledProcessError.output. Read e.output, matching run_speed_benchmark. * run_speed_benchmark()'s PRINCE branch called get_rules_and_hl() with only one argument; the function requires (command, alias), so it raised TypeError. Pass the hashlist alias, as the preprocessor path already does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An agent assigned a mask/brute-force task that uses
--incrementbecomesInactiveand never recovers. The server log shows the benchmark being retriedand failing over and over:
Example task (from a supertask):
Root cause
The speed benchmark runs hashcat with
--progress-only, and hashcat refuses tocombine
--progress-onlywith--increment:So the benchmark exits non-zero for every increment task. The agent turns
that into a
clientError. On the server,APIClientError.phpdeactivates anagent on its first error whenever
ignoreErrors == 0(the default), so theagent flips to
isActive = 0and can only be revived by hand. The agent loopkeeps re-requesting the task every ~10 s, reproducing the repeated log lines.
A second, smaller bug makes this hard to diagnose: in
run_speed_benchmark,outputis reset tob''immediately before thetry, so onCalledProcessErrorthe code logsoutput.decode()(always empty) and neverlooks at
e.output, which holds hashcat's real message. The local error log isalways blank on any speed-benchmark failure.
Fix
Commit 1 — the increment benchmark failure:
--progress-only. The measuredcracking speed is independent of the mask length / increment range, so the
benchmark stays valid. A new
strip_increment()helper inhelpers.pyisapplied to the speed-benchmark command only (not to
--keyspaceor the--runtimerun benchmark, which accept--increment).e.outputinstead of thealways-empty
output, so the error log line for a genuine benchmark failureis no longer blank. The error sent to the server is unchanged
(
"Speed benchmark failed!"), to avoid bloating server-side AgentErrorrecords with multi-line hashcat output.
strip_increment()intentionally iterates the tokens directly rather thanrouting them through the existing
clean_list()helper (see commit 2 for why).Commit 2 — related robustness bugs found while fixing the above (kept as a
separate commit so it can be reviewed or split off on its own):
clean_list()deleted from a list while iterating it (del element_list[index]inside
for part in element_list), which skips elements and leaves realtokens — and empty strings — behind. On the runs of spaces in the assembled
benchmark command this dropped the attack mask itself (e.g.
-a 3 #HL# ?d?d?d?d?d→ bare-a 3), after which hashcat falls back to itsbuilt-in default mask, which references
?1, and fails withCustom-charset 1 is undefined. Rewritten as[p for p in element_list if p].This also stops
get_wordlist()from raisingIndexErroron the emptystrings
clean_list()used to leave (itspart[0]check assumes none remain).clean_list()'s only two callers (get_wordlist,get_rules_and_hl) use thereturn value, so returning a fresh list instead of mutating in place changes
nothing for them.
measure_keyspace()had the same swallowed-output bug as (2) — fixed thesame way (
e.output).run_speed_benchmark()calledget_rules_and_hl(update_files(task['attackcmd']))with one argument; thesignature is
(command, alias), so it raisedTypeError. Pass the hashlistalias, matching the correct call in the preprocessor path. (PRINCE is
deprecated; included only because it's a guaranteed crash if that path runs.)
Alternatives considered
command — but the agent still needs a speed figure for chunk sizing, so
stripping
--incrementand measuring the base mask is the least-disruptive fix(cracking speed for a mask attack is independent of mask length / the increment
range, so the number stays valid).
tasks — larger blast radius, and the agent would still mishandle a
--progress-only+--incrementcommand it was handed. Keeping the fix in theagent that builds the command is the more contained change.
Testing
tests/test_helpers.py: 8 unit tests, all passing —strip_increment(=form, space-separated form, short
-i, no-op cases, and a regression testthat the mask survives runs of spaces) and
clean_list(consecutive empties,full list untouched).
"Increment is not allowed in combination with --progress-only." (exit 255)
and that
strip_incrementclears it.benchmarks both a real increment task and a plain-digit mask task cleanly,
cracks, and stays active — zero benchmark errors.
Notes / follow-ups (not in this PR)
?d,?l, …) are interpolated into the command string that isrun with
shell=True, unquoted;?and*are shell globs, so a mask can beglob-expanded against files in the cracker/working directory. It rarely
matches real filenames, but quoting masks would remove the hazard.
--keyspaceaccepts--incrementbut reports the keyspace of only the basemask, so chunking can under-count for increment tasks.
arguably too aggressive; treating measurement errors as task-level rather than
agent-level would be a more robust server-side change, but that's out of scope
here.