--rules-stack broken (now fixed but may still need some TLC) #5722

solardiz · 2025-03-27T03:14:26Z

Looks like we (perhaps I) broke it in the recent week. Before recent changes:

$ echo password > w
$ ./john- -w=w --rules-stack=':[lu]' -stdo
Using default input encoding: UTF-8
password
PASSWORD
2p 0:00:00:00 100.00% (2025-03-27 04:13) 40.00p/s PASSWORD

Now:

$ ./john -w=w --rules-stack=':[lu]' -stdo | head
Using default input encoding: UTF-8
Press 'q' or Ctrl-C to abort, 'h' for help, almost any other key for status
password
password
password
password
password
password
password
password
password
password

The text was updated successfully, but these errors were encountered:

solardiz · 2025-03-27T03:15:49Z

Reverting d265b3c repairs it.

magnumripper · 2025-03-27T03:26:28Z

Happens with PRINCE mode too (indeed any mode).

solardiz · 2025-03-27T03:27:45Z

This hack repairs it (just to narrow down where the problem is):

+++ b/src/rules.c
@@ -1863,8 +1863,11 @@ char *rules_process_stack_all(char *key, rule_stack *ctx)
        rules_stacked_after = 0;
 
        while (ctx->rule) {
-               if ((word = rules_apply(key, ctx->rule->data, -1)))
+               static char last[99];
+               if ((word = rules_apply(key, ctx->rule->data, -1)) && strcmp(word, last)) {
+                       strcpy(last, word);
                        return word;
+               }
                if ((ctx->rule = ctx->rule->next)) {
                        rules_stacked_number++;
                        if (!stack_rules_mute)

solardiz · 2025-03-27T03:30:17Z

So somehow rules_process_stack_all depended on this check against last word, which rules_apply no longer has. We should remove this dependency. @magnumripper I'm very tired now and I think stacked rules is your code, so maybe you could take over?

magnumripper · 2025-03-27T03:31:37Z

I'll look into it right away

solardiz · 2025-03-27T03:32:21Z

Thank you! I'm sorry I didn't test this feature before merging - I should have, as it was obviously touched by the changes.

solardiz · 2025-03-27T03:36:31Z

This almost works:

+++ b/src/rules.c
@@ -1863,9 +1863,11 @@ char *rules_process_stack_all(char *key, rule_stack *ctx)
        rules_stacked_after = 0;
 
        while (ctx->rule) {
-               if ((word = rules_apply(key, ctx->rule->data, -1)))
+               char *rule = ctx->rule->data;
+               ctx->rule = ctx->rule->next;
+               if ((word = rules_apply(key, rule, -1)))
                        return word;
-               if ((ctx->rule = ctx->rule->next)) {
+               if (ctx->rule) {
                        rules_stacked_number++;
                        if (!stack_rules_mute)
                            log_event("+ Stacked Rule #%u: '%.100s' accepted",

but doesn't terminate when it should:

$ ./john -w=w --rules-stack=':[ul]' -stdo | head
Using default input encoding: UTF-8
Press 'q' or Ctrl-C to abort, 'h' for help, almost any other key for status
PASSWORD
password
PASSWORD
password
PASSWORD
password
PASSWORD
password
PASSWORD
password

solardiz · 2025-03-27T03:43:06Z

This may actually be correct (passes my test):

+++ b/src/rules.c
@@ -1852,25 +1852,19 @@ char *rules_process_stack_all(char *key, rule_stack *ctx)
 {
        char *word;
 
-       if (!ctx->rule) {
+       if (!ctx->rule && !rules_stacked_number)
                ctx->rule = ctx->stack_rule->head;
-               rules_stacked_number = 0;
-               if (!stack_rules_mute)
-                       log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                                 rules_stacked_number + 1, ctx->rule->data);
-       }
 
        rules_stacked_after = 0;
 
        while (ctx->rule) {
-               if ((word = rules_apply(key, ctx->rule->data, -1)))
+               char *rule = ctx->rule->data;
+               rules_stacked_number++;
+               if (!stack_rules_mute)
+                       log_event("+ Stacked Rule #%u: '%.100s' accepted", rules_stacked_number, rule);
+               ctx->rule = ctx->rule->next;
+               if ((word = rules_apply(key, rule, -1)))
                        return word;
-               if ((ctx->rule = ctx->rule->next)) {
-                       rules_stacked_number++;
-                       if (!stack_rules_mute)
-                           log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                                 rules_stacked_number + 1, ctx->rule->data);
-               }
        }
 
        rules_stacked_after = !!(options.flags & (FLG_RULES_CHK | FLG_SINGLE_CHK | FLG_BATCH_CHK));

Edit: correct the logged rule number.

It's also simpler code than we had before. But it relies on two things:

That rules_stacked_number was initialized to 0.
That we increment it at least once (there's at least one stacked rule).

solardiz · 2025-03-27T03:55:17Z

Also seem to have found a bug we had before, where single mode would log stacked rule number 1 twice. Fix:

+++ b/src/rules.c
@@ -1831,9 +1831,9 @@ char *rules_process_stack(char *key, rule_stack *ctx)
 
        if (!ctx->rule) {
                ctx->rule = ctx->stack_rule->head;
-               rules_stacked_number = 0;
+               rules_stacked_number = 1;
                log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                         rules_stacked_number + 1, ctx->rule->data);
+                         rules_stacked_number, ctx->rule->data);
        }
 
        rules_stacked_after = 0;

…ssion Apparently --rules-stack was b0rken all the time, relying on the dupe suppression to proceed to next rule. This musy have been ineffective. Closes openwall#5722

magnumripper · 2025-03-27T03:59:40Z

I came up with this

diff --git a/src/rules.c b/src/rules.c
index 5ea260326..78509e546 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -1858,6 +1858,13 @@ char *rules_process_stack_all(char *key, rule_stack *ctx)
                if (!stack_rules_mute)
                        log_event("+ Stacked Rule #%u: '%.100s' accepted",
                                  rules_stacked_number + 1, ctx->rule->data);
+       } else {
+               if ((ctx->rule = ctx->rule->next)) {
+                       rules_stacked_number++;
+                       if (!stack_rules_mute)
+                               log_event("+ Stacked Rule #%u: '%.100s' accepted",
+                                         rules_stacked_number + 1, ctx->rule->data);
+               }
        }
 
        rules_stacked_after = 0;

magnumripper · 2025-03-27T04:01:01Z

This was b0rken all the time - it must have produced one consecutive dupe for each call. Not very effective.

solardiz · 2025-03-27T04:02:21Z

I came up with this

This is certainly a less invasive change. If it works, let's get it in first, and then think of my code cleanup ideas. Thank you!

solardiz · 2025-03-27T04:04:17Z

Also seem to have found a bug we had before, where single mode would log stacked rule number 1 twice.

Fixing this may expose something else, as single.c looks at the stacked rule number in a few places. So please review that first.

Apparently --rules-stack was b0rken all the time, relying on the dupe suppression to proceed to next rule. This musy have been ineffective. Closes openwall#5722

solardiz · 2025-03-27T04:06:34Z

single.c looks at the stacked rule number in a few places. So please review that first.

At least restore_rule_number would need to be consistent with that fix.

Apparently --rules-stack was b0rken all the time, relying on the dupe suppression to proceed to next rule. This must have been ineffective. Closes openwall#5722

magnumripper · 2025-03-27T04:10:32Z

Added a PR. My code is very likely to just restore functionality to what it was (actually way better than it was) so I think we can merge it as a quick fix. Meanwhile I'll try to wrap my head around your version.

solardiz · 2025-03-27T04:12:51Z

This may actually be correct (passes my test):

It did pass my test as posted in here, but it failed with actual stacking on top of normal rules. So it's not ready.

solardiz · 2025-03-27T04:17:00Z

This passes my current tests:

diff --git a/src/cracker.c b/src/cracker.c
index da2c9fc5c..a7db9c7f0 100644
--- a/src/cracker.c
+++ b/src/cracker.c
@@ -1234,6 +1234,7 @@ static int process_key_stack_rules(char *key)
        int ret = 0;
        char *word;
 
+       rules_stacked_number = 0;
        while ((word = rules_process_stack_all(key, &crk_rule_stack)))
                if ((ret = crk_direct_process_key(word)))
                        break;
diff --git a/src/rules.c b/src/rules.c
index 5ea260326..e1378616f 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -1831,9 +1831,9 @@ char *rules_process_stack(char *key, rule_stack *ctx)
 
        if (!ctx->rule) {
                ctx->rule = ctx->stack_rule->head;
-               rules_stacked_number = 0;
+               rules_stacked_number = 1;
                log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                         rules_stacked_number + 1, ctx->rule->data);
+                         rules_stacked_number, ctx->rule->data);
        }
 
        rules_stacked_after = 0;
@@ -1852,25 +1852,19 @@ char *rules_process_stack_all(char *key, rule_stack *ctx)
 {
        char *word;
 
-       if (!ctx->rule) {
+       if (!ctx->rule && !rules_stacked_number)
                ctx->rule = ctx->stack_rule->head;
-               rules_stacked_number = 0;
-               if (!stack_rules_mute)
-                       log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                                 rules_stacked_number + 1, ctx->rule->data);
-       }
 
        rules_stacked_after = 0;
 
        while (ctx->rule) {
-               if ((word = rules_apply(key, ctx->rule->data, -1)))
+               char *rule = ctx->rule->data;
+               rules_stacked_number++;
+               if (!stack_rules_mute)
+                       log_event("+ Stacked Rule #%u: '%.100s' accepted", rules_stacked_number, rule);
+               ctx->rule = ctx->rule->next;
+               if ((word = rules_apply(key, rule, -1)))
                        return word;
-               if ((ctx->rule = ctx->rule->next)) {
-                       rules_stacked_number++;
-                       if (!stack_rules_mute)
-                           log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                                 rules_stacked_number + 1, ctx->rule->data);
-               }
        }
 
        rules_stacked_after = !!(options.flags & (FLG_RULES_CHK | FLG_SINGLE_CHK | FLG_BATCH_CHK));
diff --git a/src/single.c b/src/single.c
index 4568cf60e..7652604eb 100644
--- a/src/single.c
+++ b/src/single.c
@@ -74,12 +74,12 @@ static int restore_rule_number(void)
 
        if (options.rule_stack) {
                single_rule_stack.rule = single_rule_stack.stack_rule->head;
-               rules_stacked_number = 0;
+               rules_stacked_number = 1;
                while (rules_stacked_number < rec_rule[1])
                        if (!rules_advance_stack(&single_rule_stack, 1))
                                return 1;
                log_event("+ Stacked Rule #%u: '%.100s' accepted",
-                         rules_stacked_number + 1, single_rule_stack.rule->data);
+                         rules_stacked_number, single_rule_stack.rule->data);
        }
 
        return 0;

magnumripper · 2025-03-27T04:21:16Z

That looks right to me

solardiz · 2025-03-27T04:22:51Z

There's room for optimization here, as we check ctx->rule for non-NULL twice on most calls. Also we reset rules_stacked_after on every call. This is written as if it actually looped over rules, but the loop merely skips rules that couldn't apply anything, so we return and re-enter the function frequently.

magnumripper · 2025-03-27T04:25:23Z

I'm unsure about the changes to rules_stacked_number. I think I recall having "struggled" with it in the past back and forth between zero-based and one-based.

solardiz · 2025-03-27T04:26:23Z

Yes, need more review/testing of zero- vs. one-based, especially as it relates to single.c storing those numbers (for what use?)

magnumripper · 2025-03-27T04:26:51Z

```
  if (!ctx->rule) {
```

  if (!ctx->rule && !rules_stacked_number)

Why isn't (!ctx->rule) enough here?

solardiz · 2025-03-27T04:27:30Z

Why isn't (!ctx->rule) enough here?

We'd end up going over the rules again, infinitely.

solardiz · 2025-03-27T04:32:53Z

We'd end up going over the rules again, infinitely.

Oh, could use ctx->done for that, like the single mode version of this function uses.

magnumripper · 2025-03-27T04:32:54Z

Not sure I understand. The stacked rules are supposed to rewind for each and every candidate. The old code had no such check.

solardiz · 2025-03-27T04:34:56Z

The old code had no such check.

The old code returned with ctx->rule pointing at the just used rule. The new code returns with it pointing at the next rule, which can be NULL, yet we need to record somewhere that we don't want to repeat for the same input word after that.

Apparently --rules-stack was b0rken all the time, relying on the dupe suppression to proceed to next rule. This must have been ineffective. Closes openwall#5722

magnumripper · 2025-03-27T04:41:24Z

Reopening for considering better code

solardiz · 2025-03-27T04:46:41Z

We also should not forget to fix "bug we had before, where single mode would log stacked rule number 1 twice".

magnumripper · 2025-03-27T04:55:17Z

Maybe I'm too tired but this sounds backwards to me

/*
 * Advance stacked rules. We iterate main rules first and only then we
 * advance the stacked rules (and rewind the main rules). Repeat until
 * main rules are done with the last stacked rule.
 */
int rules_advance_stack(rule_stack *ctx, int quiet)

The actual operation is we iterate over all stacked rules, then advance the main rules (if any) and rewind the stacked ones.

magnumripper · 2025-03-27T05:02:18Z

This is also weird

/*
 * Return next word from stacked rules, calling rules_advance_stack() as
 * necessary. Once there's no rules left in stack, return NULL-
 */
extern char *rules_process_stack_all(char *key, rule_stack *ctx);

The actual code does not call rules_advance_stack(). Maybe it should?

solardiz · 2025-03-27T05:04:37Z

The actual code does not call rules_advance_stack(). Maybe it should?

I actually thought of unifying this. I'm not sure we need rules_process_stack_all at all - could it be better to use the same functions that we use in single.c also in cracker.c? I'd rather not make the call chain deeper, because we'd be returning and re-entering on almost every candidate.

solardiz · 2025-03-27T05:06:04Z

Anyway, I'm done with this for right now - can't afford to unexpectedly spend a lot of time on it.

Just drop a couple of redundant log_event and move the last one so it covers all three See openwall#5722

magnumripper · 2025-03-27T16:55:58Z

I had time to test this further today and it seems 100% OK. Added an innocent cleanup in #5725, also well tested. That PR also contains a log call just before session end, that says how many candidates were processed. Good to have with verifications like this one.

Just drop a couple of redundant log_event and move the last one so it covers all three. See openwall#5722

Just drop a couple of redundant log_event and move the last one so it covers all three. See #5722

solardiz · 2025-03-28T02:52:54Z

Also seem to have found a bug we had before, where single mode would log stacked rule number 1 twice. Fix:

Opened #5729 for this. The fix I had posted in here was on top of my other changes here, which we didn't merge, and it may create other issues. So we'll need to come up with a different fix. We also need to choose what we fix first - simplify and optimize the code, or fix this bug.

solardiz · 2025-03-28T02:54:44Z

I'd rather not make the call chain deeper, because we'd be returning and re-entering on almost every candidate.

In fact, we may want to remove a level - move the logic directly into cracker.c or into an inline function in rules.h.

solardiz added bug regression Bug introduced with certain change(s), causing performance degradation or feature loss labels Mar 27, 2025

solardiz added this to the Definitely 2.0.0 milestone Mar 27, 2025

magnumripper mentioned this issue Mar 27, 2025

rules.c: Fix regression from drop of consecutive dupe suppression #5723

Merged

solardiz closed this as completed in #5723 Mar 27, 2025

solardiz closed this as completed in 1545624 Mar 27, 2025

magnumripper reopened this Mar 27, 2025

magnumripper changed the title ~~--rules-stack broken~~ --rules-stack broken (now fixed but may still need some TLC) Mar 27, 2025

magnumripper added enhancement and removed bug regression Bug introduced with certain change(s), causing performance degradation or feature loss labels Mar 27, 2025

magnumripper added a commit to magnumripper/john that referenced this issue Mar 27, 2025

rules.c: Simplify the latest bugfix to stacked rules

4073598

Just drop a couple of redundant log_event and move the last one so it covers all three See openwall#5722

magnumripper mentioned this issue Mar 27, 2025

rules.c: Simplify the latest bugfix to stacked rules #5725

Merged

magnumripper added a commit to magnumripper/john that referenced this issue Mar 27, 2025

rules.c: Simplify the latest bugfix to stacked rules

995c5d9

Just drop a couple of redundant log_event and move the last one so it covers all three See openwall#5722

magnumripper added a commit to magnumripper/john that referenced this issue Mar 27, 2025

rules.c: Simplify the latest bugfix to stacked rules

a6b35de

Just drop a couple of redundant log_event and move the last one so it covers all three. See openwall#5722

magnumripper added a commit to magnumripper/john that referenced this issue Mar 27, 2025

rules.c: Simplify the latest bugfix to stacked rules

7f2e468

Just drop a couple of redundant log_event and move the last one so it covers all three. See openwall#5722

magnumripper added a commit that referenced this issue Mar 27, 2025

rules.c: Simplify the latest bugfix to stacked rules

3e75e1b

Just drop a couple of redundant log_event and move the last one so it covers all three. See #5722

--rules-stack broken (now fixed but may still need some TLC) #5722

--rules-stack broken (now fixed but may still need some TLC) #5722

Comments

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025 • edited Loading

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025 • edited Loading

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 27, 2025

solardiz commented Mar 27, 2025

magnumripper commented Mar 27, 2025

solardiz commented Mar 28, 2025

solardiz commented Mar 28, 2025

magnumripper commented Mar 27, 2025 •

edited

Loading

solardiz commented Mar 27, 2025 •

edited

Loading