Date: Mon, 30 Mar 2015 03:14:27 +0300 From: Alexander Cherepanov <ch3root@...nwall.com> To: john-dev@...ts.openwall.com Subject: Re: Lengths of fields recorded in hashes On 2015-03-23 05:40, Solar Designer wrote: >> Does anybody consider them useful? They could potentially be useful when >> one needs to allocate memory for the data but even in this case they >> save only one strlen (or equivalent). >> OTOH each superfluous field now requires several lines of code in >> valid() and get_salt(). And parsing numbers is not that easy, e.g. atoi >> exhibits undefined behavior on overflows. > > Right. BTW, we should get rid of all atoi()'s, like in favor of a > custom wrapper around strtol() that we'd introduce into our misc.[ch]. > > Maybe we should adopt OpenBSD's strtonum(): > > http://www.tedunangst.com/flak/post/the-design-of-strtonum > > although its use of "long long" is unnecessarily not nice for 32-bit > builds. If we deviate from strtonum() in any way, we'll need to use a > different function name. Such a function doesn't seem to be a good fit for one-pass parsing at all. It doesn't return a position where it has stopped parsing and it "doesn’t handle [...] trailing non-digit characters". This leads to strdup+strtok-like approach which I thought we want to get rid of. > This is one of many tasks for the "robustness officer" we're seeking. > >> Extra fields also take extra >> space etc. Recently the point was raised that lengths are an obstacle to >> fuzzing. And it would be nice if most of the hashes are constructed from >> a small number of simple blocks (a field containing a length of another >> field is not one of them). > > While maybe being a problem for fuzzing, this also suggests that the > fields were maybe not totally useless: they serve as poor man's > checksums, even if only for metadata. For very long encoded strings, > this makes some sense to me. Actually, I thought about it but decided that it's minor enough to not be worth mentioning. >> I propose the following plan: >> 1) gradually rewrite valid()s and get_salt()s to ignore lengths recorded >> in hashes; > > No, valid() must continue to check whatever practical. > > I am OK with rewriting get_salt(). > > BTW, officially - such as in formats.h - this function is called > salt(). And ditto for binary(). The get_ prefix, as used by many > formats, isn't exactly right: these function aren't "getters" (from the > format's state), they are "extractors" (from the function argument). > Maybe we should rename them dropping the get_ prefix, or changing it to > extract_ (but that's longer, and ext_ would conflict with the naming > we're already using for functions implementing external mode support). > For now, let's not bother - not the worst of our worries, by far. Ok, I've added it into todo list: - Unify function names (get_salt -> salt etc.). This makes grepping/refactoring easier. >> 2) change the format of hashes in john and in 2john tools at some later >> point. > > Hmm, to be compatible with your changes proposed as step 1 above, would > the versions of *2john tools from step 2 produce empty fields in place > of the lengths? That is, two field separators in a row? That's still > not pretty. > > Anyway, we probably should do this, for the reasons magnum explained. You mean to accept empty length but check that it's right if it's not empty? >> Can we change format of such hashes at will (by requiring to always use >> john and 2john tools of matching versions)? > > Not necessarily. We could if we really needed to, though. > >> Attached is a patch to 7z format which implements the first part of the >> plan as an illustration of the approach. (It also fixes a potential >> overflow in data field and removes strange dealing with zeroes in iv.) > > I didn't review it closely, but in general I think that your changes to > get_salt() are acceptable since get_salt() can assume(*) that its input > is valid, but your removal of checks from valid() is not acceptable. Ok > (*) We should discuss this separately. In fact, I recall this was > already brought up in another thread. I think it's another task for > "robustness officer" to review loader.c very carefully, and to fuzz an > instrumented version of JtR with assertions added, and make sure that > only output from valid(), possibly passing through split(), can pass > into salt() and binary(). If this is not currently the case, then > whatever exceptions to this might exist should be patched in loader.c. > > Please write down the tasks I am mentioning here into some sort of to-do > list, such as a page you'd create on our wiki for the robustness project. I've posted the todo list but I'm yet to create a wiki page for it. -- Alexander Cherepanov
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.