[john-dev] rules.c bug/feature

[magnum]

Solar,

While playing with some old contest rules I found a bug in John that's
not Jumbo-specific: Apparently it lacks some checks so a 'ddd' rule will
blow the destination buffer even at moderate input lengths (eg. 50).

The implications are a smashed rules_data.classes array which may
eventually lead to a segfault but I think it may also just "seem to
work" although subsequent rules will actually execute incorrectly.

I think the best fix is to quietly truncate the copy so ddd (and even
dddd and so on) will work fine with short enough words? Here is a fix
that seem to work but not much tested and I really did not count the
fence posts very carefully:

diff --git a/src/rules.c b/src/rules.c
index d20d1d5..0bb525b 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -441,7 +441,9 @@ char *rules_apply(char *word, char *rule, int split,
char *last)
break;

case 'd':
- memcpy(in + length, in, length);
+ if (rules_max_length - length > 0)
+ strnzcpy(in + length, in,
+ rules_max_length - length);
in[length <<= 1] = 0;
break;


I'll wait with fixing Jumbo until you comment.

magnum

[magnum]

Post by magnum
- memcpy(in + length, in, length);
+ if (rules_max_length - length > 0)
+ strnzcpy(in + length, in,
+ rules_max_length - length);
in[length <<= 1] = 0;
break;

Correction: Obviously the "in[length <<= 1] = 0;" line should change
too. We no longer need to null terminate but we need to update length
properly to whatever it ended up as.

magnum

[Solar Designer]

Post by magnum
While playing with some old contest rules I found a bug in John that's
not Jumbo-specific: Apparently it lacks some checks so a 'ddd' rule will
blow the destination buffer even at moderate input lengths (eg. 50).

No, this shouldn't be the case. It is assumed that any rule command may
double the word's length, and there's a safeguard inbetween commands.

The buffers are:

char buffer[3][RULE_WORD_SIZE * 2 + CACHE_BANK_SHIFT];

and the safeguard is:

in[RULE_WORD_SIZE - 1] = 0;

Is this somehow broken? We should identify the issue and fix it if so.

Post by magnum
I think the best fix is to quietly truncate the copy so ddd (and even
dddd and so on) will work fine with short enough words? Here is a fix
that seem to work but not much tested and I really did not count the

I think we shouldn't include per-command workarounds like this. The
global safeguard above should be sufficient.

Alexander

[magnum]

Post by Solar Designer

Post by magnum
While playing with some old contest rules I found a bug in John that's
not Jumbo-specific: Apparently it lacks some checks so a 'ddd' rule will
blow the destination buffer even at moderate input lengths (eg. 50).

No, this shouldn't be the case. It is assumed that any rule command may
double the word's length, and there's a safeguard inbetween commands.
char buffer[3][RULE_WORD_SIZE * 2 + CACHE_BANK_SHIFT];
in[RULE_WORD_SIZE - 1] = 0;
Is this somehow broken? We should identify the issue and fix it if so.

Sounds good, but then it must be broken somehow. The memcpy in 'd' did
blow the buffer and overwrote rules_data.classes and I verified this
happens in John proper too. I'm not sure why but I'll let you handle it.

I debugged it like this: Insert an "int canary;" right before and/or
after rules_data.memory. Then have gdb watch rules_data.canary and run
some words through 'dd' and 'ddd' (a ruleset of those two only).

It took me a good while to narrow it down, in Jumbo. I got segfaults
long after the actual thrashing and it was hard to understand what was
happening until I realized rules_data.classes was not intact.

I'm sure you can easily replicate this but if you can't I'll write an
exact procedure.

magnum

[Solar Designer]

Post by magnum

Post by Solar Designer

Post by magnum
While playing with some old contest rules I found a bug in John that's
not Jumbo-specific: Apparently it lacks some checks so a 'ddd' rule will
blow the destination buffer even at moderate input lengths (eg. 50).

No, this shouldn't be the case. It is assumed that any rule command may
double the word's length, and there's a safeguard inbetween commands.
char buffer[3][RULE_WORD_SIZE * 2 + CACHE_BANK_SHIFT];
in[RULE_WORD_SIZE - 1] = 0;
Is this somehow broken? We should identify the issue and fix it if so.

Sounds good, but then it must be broken somehow. The memcpy in 'd' did
blow the buffer and overwrote rules_data.classes and I verified this
happens in John proper too. I'm not sure why but I'll let you handle it.

You're right. I didn't bother reproducing it, but I think I know what
the problem is: when I introduced the "length" variable some years ago,
I forgot to update the loop logic to clamp not only the buffer but also
this integer variable to the maximum length. I think the attached patch
should fix it. I'll test and commit it.

Alexander

[Solar Designer]

magnum -

Post by Solar Designer

Post by magnum

Post by Solar Designer
in[RULE_WORD_SIZE - 1] = 0;
Is this somehow broken? We should identify the issue and fix it if so.

Sounds good, but then it must be broken somehow. The memcpy in 'd' did
blow the buffer and overwrote rules_data.classes and I verified this
happens in John proper too. I'm not sure why but I'll let you handle it.

You're right. I didn't bother reproducing it, but I think I know what
the problem is: when I introduced the "length" variable some years ago,
I forgot to update the loop logic to clamp not only the buffer but also
this integer variable to the maximum length. I think the attached patch
should fix it. I'll test and commit it.

Committed.

Alexander

[magnum]

Post by Solar Designer

Post by Solar Designer

Post by magnum

Post by Solar Designer
in[RULE_WORD_SIZE - 1] = 0;
Is this somehow broken? We should identify the issue and fix it if so.

Sounds good, but then it must be broken somehow. The memcpy in 'd' did
blow the buffer and overwrote rules_data.classes and I verified this
happens in John proper too. I'm not sure why but I'll let you handle it.

You're right. I didn't bother reproducing it, but I think I know what
the problem is: when I introduced the "length" variable some years ago,
I forgot to update the loop logic to clamp not only the buffer but also
this integer variable to the maximum length. I think the attached patch
should fix it. I'll test and commit it.

Committed.

Merged to Jumbo now.

Thanks!
magnum

[Solar Designer]

Post by magnum
- memcpy(in + length, in, length);
+ if (rules_max_length - length > 0)
+ strnzcpy(in + length, in,
+ rules_max_length - length);

In addition to what I just wrote, the above patch looks wrong to me in
its use of rules_max_length. That's hash type dependent maximum length,
whereas the word may reasonably be longer than that inbetween commands.

While looking into our uses of rules_max_length, I just noticed that we
might have a bug in the 'X' command, where it doesn't take into account
that 'M' only stores up to rules_max_length yet records the potentially
larger length into the 'm' variable. Oops. 'M' was implemented that
way since it's all that 'Q' needed - although I'm afraid this aspect of
behavior of 'Q' is undocumented, and arguably is wrong, despite of being
intentional. It looks like now we need to either revise 'M' to store
the entire word, or to revise 'X' (and update its documentation) so that
it doesn't try to restore beyond rules_max_length (but that would be
weird, right?) We could also want to revise 'Q' to compare the entire
word, or we need to document its current behavior.

Alexander