Re: Todo for release?

[Solar Designer <solar@openwall.com>]

On Mon, Aug 13, 2012 at 10:58:05PM -0400, Rich Felker wrote:
> On Tue, Aug 14, 2012 at 06:49:03AM +0400, Solar Designer wrote:
> > > If this issue is as simple to solve as it sounds, it might make sense
> > > to allow arbitrary key sizes. After all, programs that could be DoS'd
> > > by long keys are already going to be limiting key length themselves.
> > 
> > That's wishful thinking.
> 
> Are you sure? I haven't read the code lately, but I can't imagine any
> login daemon is going to be calling realloc() in a loop to read an
> arbitrarily long password before authentication. That just sounds
> gratuitously broken (i.e. someone went out of their way to write
> painful code that does nothing useful and makes their daemon
> susceptible to DoS).

I guess many daemons written in C limit the length at a few kilobytes -
which may allow for about 100 times greater than intended (by sysadmin)
crypt() running time.  For md5crypt and sha*crypt, the first slowdown
occurs between length 15 and 16.

Then, it does not take explicit realloc() for just the password string
to support arbitrarily long passwords.  The daemon may be using an
abstraction layer for all strings - e.g., qmail, Postfix, and vsftpd
have such dynamic string libraries of their own, and overall this is
good (it avoids buffer overflows and artificial limits in other places).
I don't know if vsftpd would in fact pass arbitrarily long passwords to
crypt() - this is worth checking.

Finally, some services are written in languages that support dynamically
allocated strings natively.  I recall that OpenStack's Python code was
patched to impose a limit of 4096 chars on passwords recently,
specifically in response to risks like what we're discussing here.
(And 4096 is still a lot - may allow for some attacks.)

> > > something that can never match. By the way, would you agree that all
> > > programs that generate new password hashes should do so by calling
> > > crypt twice, the second time using the output of the first as the
> > > setting/salt, and verify that the results match? This seems to be the
> > > only safe/portable way to make sure you got a valid hash and not an
> > > error.
> > 
> > This makes sense, yet it sounds overkill to me.  I'd simply check for
> > hash && strlen(hash) >= 13.
> 
> Indeed, strlen(hash)>=13 is certainly a necessary condition, but is it
> sufficient? I could imagine a hypothetical crypt implementation that
> puts error messages in the unmatchable hash as a debugging aid to why
> generating the hash failed, but I agree they probably don't exist.
> Still, changing your password should not be a frequent action, so it
> might make the most sense to do the check the way I suggested.

An aspect to consider is that if you call crypt() twice for just one
request from the user (to set a password), you effectively double the
iteration count as it relates to potential CPU time exhaustion DoS
attacks (twice more CPU time is consumed per request).  So if these
attacks are the limiting factor in a sysadmin's ability to set a higher
iteration count, this will result in the iteration count being twice
lower (and accordingly weaker hashes being used on the system).  (If the
protocol is such that the same request contains the old password to be
checked first, then the difference may be 3/2.)  While each individual
sysadmin is somewhat unlikely to apply this reasoning and consider
password setting as the worst DoS attack vector, I think that overall
this may in fact result in somewhat lower iteration counts being used
(average across many systems).

Alexander