Rules: Complete support for lengths beyond 128

[solardiz]

See #5873

[solardiz]

Actually, the T command implementation is special in that it didn't check against the length. We could simplify (relative to changes in this PR):

+++ b/src/rules.c
@@ -201,16 +201,11 @@ static char *conv_tolower, *conv_toupper;
 #define VALUE(value) { \
        if (!((value) = RULE)) goto out_ERROR_END; \
 }
 
-/* The pos variable may be signed or unsigned int, which affects comparison */
 #define POSITION(pos) { \
-       if (((pos) = rules_vars[ARCH_INDEX(RULE)]) > INFINITE_LENGTH) { \
-               if (pos > (unsigned int)-MAX_PLAINTEXT_LENGTH) \
-                       pos = INFINITE_LENGTH; \
-               else \
-                       goto out_ERROR_POSITION; \
-       } \
+       if (((pos) = rules_vars[ARCH_INDEX(RULE)]) == INVALID_LENGTH) \
+               goto out_ERROR_POSITION; \
 }
 
 #define CLASS_export_pos(start, true, false) { \
        char value, *class; \
@@ -1239,9 +1234,10 @@ char *rules_apply(char *word_in, char *rule, int split)
                case 'T':
                        {
                                unsigned int pos;
                                POSITION(pos)
-                               in[pos] = conv_invert[ARCH_INDEX(in[pos])];
+                               if (pos < length)
+                                       in[pos] = conv_invert[ARCH_INDEX(in[pos])];
                        }
                        break;
 
                case 'D':

However, then we need to worry about possible integer overflows in calculations in a few other command implementations. We could avoid those by making the rules_vars elements 16-bit, but I guess this may be slower on some archs.

Any thoughts?

[solardiz]

Actually, the T command implementation is special in that it didn't check against the length. We could simplify

I've just reworked this PR to implement proper range checking for each command where this was missing or incomplete.

[solardiz]

@AlekseyCherepanov We could use some fuzzing of the rules engine - mangled rules based on our existing rulesets (before or after preprocessor expansion), maybe invoking john on single ruleset lines at a time (so that syntax errors do not skip fuzzing of further lines, or patch it to achieve that). Input wordlists containing both short and very long lines, including near our previous and new maximum. All of this against a build with ASan. Maybe also specifically target the v command (and its multiple uses) followed by rule commands that accept position codes. I think our actual rulesets do not fully explore the possible risks - e.g., we never use multiple v to arrive at really weird position numbers, we only do reasonable calculations there.

[magnumripper]

Looks fine. I'll actually check this out and run some tests as well.

[solardiz]

Thank you @magnumripper. My testing so far included running a tiny wordlist consisting of long lines around 128, 256, and 320 chars with our --rules=All using a build with ASan. It doesn't crash with this PR's current changes. Unfortunately, the output is not supposed to exactly match what we had before. In another test case I ran - with just the word password as input - the output does match what we had before.

[magnumripper]

Ran out of time, I did few tests with very long input (which is what's most important, but I had no time to create a good wordlist for that - just many repeated characters is not good enough)

But I did test it with all sorts of obscure rules not in our repo.

[solardiz]

I've just inserted commit Rules: Move RULE_WORD_SIZE from params.h to rules.c. RULE_WORD_SIZE isn't a separately configurable setting, so let's not expose it to advanced users in params.h.