Fix a suppressor segfault due to being called twice from wordlist

[magnumripper]

We had a dupe suppression segfault (mentioned with the mgetl problem in #5704, but unrelated). It turns out it would only trigger when running --loopback. The problem is that wordlist.c would call suppressor_init() twice in that case, resulting in old_process_key being set to suppressor_process_key() and losing the original pointer to crk_process_key() forever. Later, crk_process_key would be reset to the incorrect old_process_key and as a result we'd eventually call suppressor_process_key() after filter is freed.

In wordlist.c, the label REDO_AFTER_LMLOOP: is a hint of why it happens. I separately added protection in suppressor.c as well (does not affect performance) - in my opinion we should do both.

BTW Presumably the problem would only surface when we actually had some LM hashes in the input file, and some of them was present in the pot file so assembled to the special loop. That's why it went under the radar for quite some time. I didn't verify this explanation though, the available time / curiosity ratio is too low.

[magnumripper]

Just for reference, it looks like using --pipe could also trigger the bug in the same way (with a goto back to before suppression init).

[solardiz]

looks like using --pipe could also trigger the bug in the same way (with a goto back to before suppression init).

I've just tried using --pipe along with suppressor and --rules, but could not trigger any misbehavior (without this PR).

[solardiz]

Actually, this may be wrong. We do call suppressor_init a second time on purpose when transitioning from wordlist to incremental mode. I guess that worked fine because crk_process_key got reset to incremental mode's before that second call. But now the above check probably breaks it.

[magnumripper]

Maybe we could just do this

diff --git a/src/suppressor.c b/src/suppressor.c
index d4537f19e..3b36476c9 100644
--- a/src/suppressor.c
+++ b/src/suppressor.c
@@ -66,7 +66,8 @@ void suppressor_init(unsigned int new_flags)
        flags = new_flags;
        status.suppressor_end = 0;
        status.suppressor_end_time = 0;
-       old_process_key = crk_process_key;
+       if (!old_process_key)
+               old_process_key = crk_process_key;
        crk_process_key = suppressor_process_key;
 }

[magnumripper]

Hmm no that might not be right either, as crk_process_key apparently changes over time.

[magnumripper]

Perhaps this (edited for typo)

diff --git a/src/suppressor.c b/src/suppressor.c
index 7fa3fef0d..bed090f54 100644
--- a/src/suppressor.c
+++ b/src/suppressor.c
@@ -27,6 +27,9 @@ static int suppressor_process_key(char *key);
 
 void suppressor_init(unsigned int new_flags)
 {
+       if (crk_process_key == suppressor_process_key)
+               return;
+
        if (!flags) {
                if (!(new_flags & SUPPRESSOR_UPDATE))
                        return;

[magnumripper]

looks like using --pipe could also trigger the bug in the same way (with a goto back to before suppression init).

I've just tried using --pipe along with suppressor and --rules, but could not trigger any misbehavior (without this PR).

I didn't verify but it sure looks like it. Did you run it until the "Disabling duplicate candidate password suppressor" appeared? After that, next crk_process_key() should go boom.

[solardiz]

it would only trigger when running --loopback. The problem is that wordlist.c would call suppressor_init() twice in that case, resulting in old_process_key being set to suppressor_process_key() and losing the original pointer to crk_process_key() forever.

I am able to reproduce this part, as seen from a debugging print I added.

Later, crk_process_key would be reset to the incorrect old_process_key and as a result we'd eventually call suppressor_process_key() after filter is freed.

I could not reproduce this part yet. I'd expect we'd first trigger recursion when suppressor_process_key ends up calling itself, but somehow this didn't happen in my testing either.

[solardiz]

I'll try to come up with a proper fix. This is quite a bit more involved.

[solardiz]

I didn't verify but it sure looks like it. Did you run it until the "Disabling duplicate candidate password suppressor" appeared? After that, next crk_process_key() should go boom.

With --pipe --rules, got "Disabling ..." to appear, but no second call to suppressor_init.

[magnumripper]

I didn't verify but it sure looks like it. Did you run it until the "Disabling duplicate candidate password suppressor" appeared? After that, next crk_process_key() should go boom.

With --pipe --rules, got "Disabling ..." to appear, but no second call to suppressor_init.

I debugged it now, and it doesn't happen because the pipe buffer is (always?) large enough that dupe suppression gets disabled before the goto, and then a re-init should be OK - right?

[solardiz]

the pipe buffer is (always?) large enough that dupe suppression gets disabled before the goto, and then a re-init should be OK - right?

No, these two statements don't sound right to me.

Anyway, I've just prepared a new PR, which passes my tests (including with debugging print added, and with wordlist to incremental mode transitions and the suppressor getting disabled before or after the transition).

[solardiz]

dupe suppression gets disabled before the goto, and then a re-init should be OK - right?

This statement may be right. Looks like the done call does enough of a cleanup that a new init is safe, even if non-optimal.

[magnumripper]

the pipe buffer is (always?) large enough that dupe suppression gets disabled before the goto, and then a re-init should be OK - right?

No, these two statements don't sound right to me.

I believe the first statement is correct, I saw suppression being disabled way before any goto/re-init. The default pipe buffer is 150 MB. Then again it would depend on number of rules - I used -rules:all.