Date: Wed, 13 Jun 2012 10:53:18 -0400 From: Rich Felker <dalias@...ifal.cx> To: musl@...ts.openwall.com Subject: Re: FreeSec crypt() On Wed, Jun 13, 2012 at 04:07:54PM +0400, Solar Designer wrote: > > I also removed > > some unused code for things like keeping salt/keys between multiple > > runs since the data is not preserved across multiple runs anyway, > > It can be preserved if you make the (tiny) struct _crypt_extended_local > static. I suppose this is worth considering. I was assuming it would be difficult to preserve (require otherwise-unneeded locking), but for crypt_r each caller could be responsible for its own copy. However, on the other hand I'm not sure I see the benefit. Why would you call crypt or crypt_r multiple times with the same key/salt except possibly for password cracking (in which case you'll want a highly optimized-for-speed implementation)? > > and the lookup tables for bitshifts. > > As discussed on IRC, I had tried that, but reverted my changes because > they resulted in code size increase (greater than the savings from not > having those tables). Apparently, the table lookups helped avoid mov's > (on 2-operand archs like x86) and add/sub (becomes part of effective > address calculation). Did you check how this affected total size in > your case? Or do you prefer cleaner source code anyway (makes sense)? I didn't compare, but yes I do prefer cleaner source. I'm a bit surprised that you found the difference in code size to be >136 bytes though. Are you sure the code size changed by that much? > > > Also, we could want to add a runtime self-test, which would detect > > > possible miscompiles. > > > > I understand your motivation for doing this with security-critical > > things, but really most/all of libc is security-critical, and we can't > > have runtime miscompilation tests all over the place. Moreover, the > > vast majority of cases of GCC "miscompiling" something turn out to be > > code that's invoking undefined behavior; the only non-UB example I've > > encountered while working on musl is gcc 4.7's bad breakage, which is > > so bad you can't even get programs to start... > > I agree that the vast majority of cases may involve UB, but so what - we > can't be 100% certain we don't have any cases of UB in our source code > even if we review it very carefully. For purely computational code that doesn't recurse or loop arbitrarily long based on the input or read and write all over memory, static analysis can determine the absence of UB. Having no UB, and having the code tested with both signed and unsigned char (easy with gcc -f options) should give an extremely high level of confidence that cases which have been tested on one system/compiler will behave identically on another system or another compiler. > We might miss things, and besides > source code changes over time may introduce UB (e.g., changes in > variable declarations may result in code that continues to work > correctly, but actually depends on undefined behavior). And the same is true elsewhere in much more critical code, like snprintf, strcpy, strchr, etc. all of which could create huge vulns if miscompiled. What I was saying is that if you're going to call into question the compiler, I think you need some basis for deciding which functions need to double-check that their results are correct, unless your intent is to make ALL of your code fault-tolerant in the form of double-checking its results. The latter would certainly be an interesting project in itself, but I think it goes way beyond the scope (and contrary to many of the goals, including size, speed, and simplicity) of musl. If one really wants to make such a system, I think it would make a lot more sense to have the checking automatically generated by the compiler/toolchain (e.g. use 2 separate compilers and compare the results constantly). > We have a fairly large chunk of code here (a few KB), which we can have > self-tested by a few hundred bytes of code more. I think this is worth > it. I'm open to seeing them if they're really that small and non-invasive. > Besides, the self-test may serve to zeroize sensitive data - and in > a more reliable way than a memset() would (which may be optimized out, > and it would not necessarily zeroize stack and registers). In fact, I > am using this approach to zeroization in crypt_blowfish. Yes, the > self-test needs to run _after_ hashing the real password, and then > depending on self-test result you either return the originally computed > value or you return an error indication (what to return from crypt() on > error is a separate non-trivial topic). I saw the notes on this. What real-world code breaks from the conformant behavior? > > * This version is derived from the original implementation of FreeSec > .... > > Can you please add a comment documenting your changes, too? Sure, I'll do that before committing. What I sent to the list was just a quick draft, and I certainly don't want to misrepresent my modified version as if it were your original, especially if it has bugs. I really should have added a comment before even sending it... > I notice that you fully dropped the tests - are you going to have > compile-time tests elsewhere? I just did it while shuffling around the code -- I think I moved them and the adjacent table-gen code I wrote to a separate file at the same time. I'm fairly indifferent to putting them in the file, but I'd much rather have a test for the external API in an external test module as the actual tests we use. Rich
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.