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.