Re: FreeSec crypt()

[Rich Felker <dalias@aerifal.cx>]

On Sun, Jun 24, 2012 at 11:21:12AM +0400, Solar Designer wrote:
> On Wed, Jun 13, 2012 at 10:53:18AM -0400, Rich Felker wrote:
> > 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)?
> 
> You're right.  Additionally, preserving this requires that we keep
> sensitive data around (from the previous password hashed).  I've dropped
> this stuff now.

Ah, nice catch!

> > > 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.
> 
> Are you performing such static analysis on musl?

Not yet/only to a minimal extent so far.

I haven't spent much time on it, but users/contributors have often
posted clang's analysis outputs for discussion on IRC, and it looks
like a promising direction.

> > > 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?
> 
> I think the majority of daemons, etc. that do authentication with
> crypt() don't handle possible NULL returns.  This is just starting to
> change now.  They don't really break under normal conditions because
> normally crypt() returns the hashed password and not NULL; but if it
> does somehow return NULL (e.g., because the salt read from the shadow
> file is invalid), then I'd expect most users of crypt() to crash.

Indeed, the first program I checked, dropbear, uses the return value
without checking it. It's rather disheartening that something as
important as authentication code is not checking the return value of
each function that could possibly fail. With TCB shadow especially,
password hashes could be highly invalid...

Anyway, nowhere does POSIX say crypt has to fail at all; as far as I
can tell, it's completely possible and reasonable to make an
implementation that always succeeds even on invalid salt. As long as
you ensure that the output can never be matched, it's probably fine to
do this.

> ....Attached is my latest revision of crypt_freesec.  I've reduced the
> table sizes even further (7 KB, may be precomputed) and I made certain
> other changes as discussed.  I'd appreciate another review, and some
> fuzzing against another implementation wouldn't hurt.

I put this off until after the release so as not to break anything at
the last minute, but I'll try to get it integrated soon.

Rich