Discussion:
rules.c patch for ASan fault
(too old to reply)
magnum
2015-12-03 18:14:46 UTC
Permalink
Solar,

Here's a (maybe) proposed patch against john proper:

diff --git a/src/rules.c b/src/rules.c
index 35cfe15..7eae64e 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -825,7 +825,7 @@ char *rules_apply(char *word, char *rule, int split,
char *last)
POSITION(mpos)
POSITION(count)
POSITION(ipos)
- mleft = (int)(rules_vars['m'] + 1) - mpos;
+ mleft = (int)(rules_vars['l']) - mpos;
if (count > mleft)
count = mleft;
if (count <= 0)


This is within the 'X' command. The rationale is that rules_vars['m'] is
an unsigned char, initially set to (length - 1). When length is 0,
rules_vars['m'] is thus 255. This leads to an ASan fault (at least a
"read" fault) unless this patch is applied. There doesn't seem to be any
more instance of similar problem.

Is there some intended behavior that this patch would break? I can't
imagine any.

For background, see
https://github.com/magnumripper/JohnTheRipper/issues/1744

magnum
Solar Designer
2015-12-03 18:28:05 UTC
Permalink
magnum,
Post by magnum
diff --git a/src/rules.c b/src/rules.c
index 35cfe15..7eae64e 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -825,7 +825,7 @@ char *rules_apply(char *word, char *rule, int split,
char *last)
POSITION(mpos)
POSITION(count)
POSITION(ipos)
- mleft = (int)(rules_vars['m'] + 1) - mpos;
+ mleft = (int)(rules_vars['l']) - mpos;
if (count > mleft)
count = mleft;
if (count <= 0)
This is within the 'X' command. The rationale is that rules_vars['m'] is
an unsigned char, initially set to (length - 1). When length is 0,
rules_vars['m'] is thus 255.
... but (rules_vars['m'] + 1) is then 0, isn't it?
Post by magnum
This leads to an ASan fault (at least a "read" fault)
I'll need to figure out why this is the case and how to fix that.
Post by magnum
unless this patch is applied. There doesn't seem to be any
more instance of similar problem.
Is there some intended behavior that this patch would break? I can't
imagine any.
Indeed, the patch looks wrong: the 'X' command is defined to extract
from memory, and it's the 'm' variable that holds the memorized word's
length. Your patch might be correct for the case when the 'M' command
hasn't been used, but it probably is incorrect when 'M' has been used.
Post by magnum
For background, see
https://github.com/magnumripper/JohnTheRipper/issues/1744
Thank you for bringing this to my attention.

Alexander
jfoug
2015-12-03 19:57:47 UTC
Permalink
Post by Solar Designer
... but (rules_vars['m'] + 1) is then 0, isn't it?
Nope, it is 256 due to this:

/*
* This assumes that RULE_WORD_SIZE is small enough that length can't
reach or
* exceed INVALID_LENGTH.
*/
rules_vars['l'] = length;
rules_vars['m'] = (unsigned char)length -1;


rules_vars['m'] is 255. Then (rules_vars['m']+1) will convert to int,
and 256 is the expression result.
Post by Solar Designer
Post by magnum
This leads to an ASan fault (at least a "read" fault)
I'll need to figure out why this is the case and how to fix that.
This is a core (IIRC), not just an ASAN error

Jim.
jfoug
2015-12-03 20:01:19 UTC
Permalink
Here is example code:

$ cat x.c
#include <stdio.h>
int main() {
unsigned char c=255;
int x;
x = (int)(c+1);
printf ("%d\n", x);
}

$ ./a
256
Post by jfoug
Post by Solar Designer
... but (rules_vars['m'] + 1) is then 0, isn't it?
/*
* This assumes that RULE_WORD_SIZE is small enough that length can't
reach or
* exceed INVALID_LENGTH.
*/
rules_vars['l'] = length;
rules_vars['m'] = (unsigned char)length -1;
rules_vars['m'] is 255. Then (rules_vars['m']+1) will convert to int,
and 256 is the expression result.
Post by Solar Designer
Post by magnum
This leads to an ASan fault (at least a "read" fault)
I'll need to figure out why this is the case and how to fix that.
This is a core (IIRC), not just an ASAN error
Jim.
Solar Designer
2015-12-03 20:09:51 UTC
Permalink
Post by Solar Designer
... but (rules_vars['m'] + 1) is then 0, isn't it?
Yeah, it's a promotion issue. So a valid fix would be to use:

mleft = (int)(unsigned char)(rules_vars['m'] + 1) - mpos;

Arguably, the very definition of 'm' is wrong, though - not allowing for
intuitive handling of zero-length strings:

"m initial or memorized word's last character position"

"Character positions are numbered starting with 0."

But this isn't something we should change now.

Alexander
jfoug
2015-12-03 20:06:50 UTC
Permalink
How about this change:

diff --git a/src/rules.c b/src/rules.c
index 35cfe15..7eae64e 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -825,7 +825,7 @@ char *rules_apply(char *word, char *rule, int split,
char *last)
POSITION(mpos)
POSITION(count)
POSITION(ipos)
- mleft = (int)(rules_vars['m'] + 1) - mpos;
+ mleft = (int)((unsigned
char)(rules_vars['m'] + 1)) - mpos;
if (count > mleft)
count = mleft;
if (count <= 0)
Post by magnum
Solar,
diff --git a/src/rules.c b/src/rules.c
index 35cfe15..7eae64e 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -825,7 +825,7 @@ char *rules_apply(char *word, char *rule, int
split, char *last)
POSITION(mpos)
POSITION(count)
POSITION(ipos)
- mleft = (int)(rules_vars['m'] + 1) - mpos;
+ mleft = (int)(rules_vars['l']) - mpos;
if (count > mleft)
count = mleft;
if (count <= 0)
This is within the 'X' command. The rationale is that rules_vars['m']
is an unsigned char, initially set to (length - 1). When length is 0,
rules_vars['m'] is thus 255. This leads to an ASan fault (at least a
"read" fault) unless this patch is applied. There doesn't seem to be
any more instance of similar problem.
Is there some intended behavior that this patch would break? I can't
imagine any.
For background, see
https://github.com/magnumripper/JohnTheRipper/issues/1744
magnum
Solar Designer
2015-12-03 20:11:05 UTC
Permalink
This is what I was about to commit, just without the extra braces.

Alexander
jfoug
2015-12-03 20:22:07 UTC
Permalink
Is there really a reason for the (int) ?? If not, then simply replace
it with (unsigned char) and be done with it. The compiler will up-cast
the unsigned char from the inner expression into an int before doing the
subtraction, even without the cast.
Post by Solar Designer
This is what I was about to commit, just without the extra braces.
Alexander
Solar Designer
2015-12-03 20:27:15 UTC
Permalink
Post by jfoug
Is there really a reason for the (int) ?? If not, then simply replace
it with (unsigned char) and be done with it. The compiler will up-cast
the unsigned char from the inner expression into an int before doing the
subtraction, even without the cast.
You're right, but I've just committed this with the (int). Arguably, it
makes the code and its intent clearer.

http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/john/john/src/rules.c.diff?r1=1.27;r2=1.28

revision 1.28
date: 2015/12/03 20:21:55; author: solar; state: Exp; lines: +2 -1
In the 'X' command, handle zero-length memorized strings correctly: avoid
undesired integer promotion to let the 'm' variable value (last character
position) overflow from 255 back to 0 (length). Thanks to magnum and JimF:
http://www.openwall.com/lists/john-dev/2015/12/03/1

Alexander

Loading...