[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110311192248.GA15765@openwall.com>
Date: Fri, 11 Mar 2011 22:22:48 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: New update to md5-gen, AND changes to the john core 'format' structures. (diff posted to Wiki)
Jim, all -
On Fri, Mar 11, 2011 at 08:49:39AM -0600, JimF wrote:
> 1. Added a void *data to the 'fmt_private' structure. Should be a
> zero impact for all formats (except ones that want to use it.
> 2. Added fmt_main pointer to the valid() and init() functions. This
> required changing almost every format.
As previously discussed off-list, I'll accept these two. In fact, #2
was my proposal, after Jim made a similar one.
> 3. Added a 'new' prepare() function. This would be called prior to
> valid(). It will 'build' the string to use as the ciphertext string. This
> also was a change for every format, since it is a new method. The
> majority, simply use fmt_prepare_default()
Basically, this moves the per-format hacks out of loader.c and takes
them into the formats themselves. I like this, but I didn't have a
chance to give this much thought yet, let alone review the specific
proposal/code.
I could "blindly" apply Jim's patch including the above (after all, it's
jumbo, where similar "blind" application of contributed patches was done
in the past), but the loader.c changes I already committed for 1.7.7
will "conflict" with this change (not fundamentally, but in terms of
specific source code changes). This means that I'd have a hard time
updating the jumbo patch for 1.7.7 then, whereas my plan was to put out
1.7.7 and 1.7.7-jumbo-1 simultaneously. (This is getting delayed, sorry.)
A way around this could be to base 1.7.7-jumbo-1's jumbo patch portion
on 1.7.6-jumbo-12 (the last version that does not include Jim's latest
changes yet) even if we have a newer revision of the jumbo patch by
then. After this release, Jim could work on 1.7.7-jumbo-2, which would
re-introduce his changes. Unfortunately, some users won't like this
temporary downgrade of functionality of the jumbo patch... yet this
might be the best way to avoid the "conflict" and not have anyone block
another developer's work.
A comment on the implementation:
In struct fmt_tests, why not have a 10- or 9-element array instead of
these fields:
char *fld1, *fld3, *fld4, *fld5, *fld6, *fld7, *fld8, *fld9, *fld10;
I see that fld2 is special, but that's not enough of a reason to replace
would-be loops with explicit duplicate/similar code for the different
ones of the above fields.
> 4. Added 'multiple' format support in john.c and md5gen_fmt.c Steps
> 1 and 2 above were needed for this.
We had discussed this off-list before. My proposal was to do it in an
even simpler fashion initially, but I don't mind the way you did it.
> 5. Gutted the md5-gen 'logic' out of bench.c and formats.c, since the
> format is now a bunch of 'normal' formats.
Great!
> 6. Gutted loader.c (Step 3 was required for this). All that logic
> is down in the prepare() functions of the formats which 'needed' that type
> of support. NOTE, the generic crypt(3) was not done this way, since I had
> no way to test it. However, all other formats which had bindings in
> loader.c have been stripped out. All functionality is there (including a
> little 'extra' for NT). The extra is NT now can be used against a file of
> 'flat' 32 byte hash values.
This sounds right given #3.
> 7. Added the 'tiny_memory' cleanup. It is simply a linked list each
> time real allocation happens, and then at the end of john_done(), a call to
> the 'cleanup' is done. This is a pretty trivial change, but could easily
> be left out. It would be nice to make sure there are no leaks, but hard to
> do, if there is lots of 'stagnant' memory, which simply appears like a leak.
The code looks buggy. Here you record a possibly-altered start address:
start = ((unsigned long)
mem_alloc(size + align) + align) & ~align;
+ add_memory_link((void*)start);
but then you'll be calling free() on it:
+ p->mem = v;
+ free(p->mem);
More importantly, I am missing the rationale behind this "cleanup".
> Upper case possible for md5 to base-16 conversions.
If so, and if I understood you correctly, you need to have split() unify
case, and set FMT_SPLIT_UNIFIES_CASE.
> This patch is pretty much assured to cause the patches to have conflicts.
> That is the reason for this email. Can this possibly be put into a jumbo,
> so that the 'existing' patches can be made against this. I do NOT see any
> additional changes to the format structure or addition/changes of
> fmt.methods
If you like, I may get this (or rather your next revision of it with at
least #7 fixed) into 1.7.6-jumbo-13, but then plan to base 1.7.7's
initial revision of the jumbo patch on 1.7.6-jumbo-12.
What do you say?
Thanks,
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.
