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

[solardiz]

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

[solardiz]

Reverting d265b3cf097510c26e80bd0d18b5779605b34c8e repairs it.

[magnumripper]

Happens with PRINCE mode too (indeed any mode).

[solardiz]

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]

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]

I'll look into it right away

[solardiz]

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]

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]

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:

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

[solardiz]

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;

[magnumripper]

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]

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

[solardiz]

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]

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.

[solardiz]

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.

[magnumripper]

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]

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]

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]

That looks right to me

[solardiz]

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]

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]

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

[magnumripper]

•   if (!ctx->rule) {
  

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

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

[solardiz]

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

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

[solardiz]

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]

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

[solardiz]

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.

[magnumripper]

Reopening for considering better code

[solardiz]

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

[magnumripper]

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]

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?