Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 10 Mar 2015 01:02:19 +0100
From: magnum <john.magnum@...hmail.com>
To: john-dev@...ts.openwall.com
Subject: Re: format method spec.

On 2015-03-09 11:55, Solar Designer wrote:
> On Sun, Mar 08, 2015 at 02:33:23AM +0100, magnum wrote:
>> We just noticed a problem which stems from the following (line numbers
>> here are from john proper) in crk_salt_loop()
>>
>>    331          crk_methods.clear_keys();
>>    332
>>    333          if (ext_abort)
>>    334                  event_abort = 1;
>>    335
>>    336          if (ext_status && !event_abort) {
>>    337                  ext_status = 0;
>>    338                  event_status = 0;
>>    339                  status_print();
>>    340          }
>>
>> The problem is we call clear_keys() and after that we call get_key() via
>> status_print(). The bleeding-edge mssql12 format currently crashes when
>> this happens.

> You're right - the code snippet above is inconsistent with the comment
> in formats.h.  I think we should fix the code.  Simply move line 331 to
> below line 340.  Please test this in jumbo, and after confirming it
> works as intended remind me to make the same change in core.

It turns out we still call status_print() from john.c after job is finished:

   597				do_external_crack(&database);
   598			else
   599			if (options.flags & FLG_BATCH_CHK)
   600				do_batch_crack(&database);
   601	
   602			status_print();
   603	
   604	#if OS_FORK
   605			if (options.fork && john_main_process)
   606				john_wait();
   607	#endif

...so problem remains. I'll try to zoom out and find a better place for
the clear_keys() in cracker.c (eg. right before setting key 0). I think
the one for Single mode is better placed.


> I think there are no uses of clear_keys() in the core tree right now,
> but since jumbo uses it we continue to provide it.
> 
> I think many (possibly most) of the jumbo formats that use clear_keys()
> now are simply being sloppy/lazy.  Doing a memset() on every
> clear_keys() is probably sub-optimal.  Also, some formats define empty
> clear_keys() instead of simply using fmt_default_clear_keys - that's a
> trivial cleanup you can make right now.

We moved away from things like "if (index == 0)" in set_key() because
the self-test code played tricks on us, not behaving like a real crack.
The uses I know of clear_keys() are because the alternatives were either
too complex (bug prone) or slower.

I wasn't aware of any format with empty functions - I fix them as I see
them. But now that I looked for it I see a few, will fix them right away.

magnum


Powered by blists - more mailing lists

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ