[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151203182805.GA8280@openwall.com>
Date: Thu, 3 Dec 2015 21:28:05 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: rules.c patch for ASan fault
magnum,
On Thu, Dec 03, 2015 at 07:14:46PM +0100, magnum wrote:
> 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.
... but (rules_vars['m'] + 1) is then 0, isn't it?
> 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.
> 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.
> For background, see
> https://github.com/magnumripper/JohnTheRipper/issues/1744
Thank you for bringing this to my attention.
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
