Re: lock(1): use crypt_checkpass(3) for one-off keys

[Scott Cheloha]

> On Jun 26, 2017, at 10:49 PM, Ted Unangst  wrote:
> 
> [...]
> 
>> 
>> CC'd tedu@ because I'm not sure if I'm using crypt_newhash(3)
>> correctly.
>> 
>> Ted: In other places people use _PASSWORD_LEN for the length
>>  of the hash buffer.  Clearly this works, but it feels off.
>>  _PASSWORD_LEN is meant to be an upper bound on length of
>>  the plaintext, not the hash output, right?
>> 
>>  Is there a better way to size my buffer for use with
>>  crypt_newhash(3)?
> 
> yes, but there is no better define for the output buffer. perhaps PASS_MAX,
> i'm not sure why everything settled on _PASSWORD_LEN.

fwiw, PASS_MAX is now deprecated by POSIX.  I think leaving it as _PASSWORD_LEN
for consistency with the rest of the tree for now is probably better.

Would a later patch exposing something like, I dunno, "BCRYPT_HASHMAX" to
includers of unistd.h be welcome?  A documented define would round out the API.

--
Scott Cheloha

Re: lock(1): use crypt_checkpass(3) for one-off keys

[Ted Unangst]

Scott Cheloha wrote:
> Hi,
> 
> Using strcmp(3) to check a password is just asking for a timing
> attack.
> 
> I admit that setting up such an attack on a custom lock(1) key at,
> say, a physical terminal would be cumbersome, so maybe this is just
> paranoia.
> 
> However, passwords *do* get reused all the time, so I think it
> makes sense to hash the key, even if it's a "one-off" key.  That
> way in the worst case a nefarious actor only has the hash.

this seems quite reasonable to me.

> 
> CC'd tedu@ because I'm not sure if I'm using crypt_newhash(3)
> correctly.
> 
> Ted:  In other places people use _PASSWORD_LEN for the length
>   of the hash buffer.  Clearly this works, but it feels off.
>   _PASSWORD_LEN is meant to be an upper bound on length of
>   the plaintext, not the hash output, right?
> 
>   Is there a better way to size my buffer for use with
>   crypt_newhash(3)?

yes, but there is no better define for the output buffer. perhaps PASS_MAX,
i'm not sure why everything settled on _PASSWORD_LEN.


> 
> --
> Scott Cheloha
> 
> Index: usr.bin/lock/lock.c
> ===
> RCS file: /cvs/src/usr.bin/lock/lock.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 lock.c
> --- usr.bin/lock/lock.c 3 May 2017 09:51:39 -   1.34
> +++ usr.bin/lock/lock.c 27 Jun 2017 02:20:33 -
> @@ -76,6 +76,7 @@ int
>  main(int argc, char *argv[])
>  {
> char hostname[HOST_NAME_MAX+1], s[BUFSIZ], s1[BUFSIZ], date[256];
> +   char hash[_PASSWORD_LEN];
> char *p, *style, *nstyle, *ttynam;
> struct itimerval ntimer, otimer;
> int ch, sectimeout, usemine, cnt, tries = 10, backoff = 3;
> @@ -162,7 +163,9 @@ main(int argc, char *argv[])
> warnx("\apasswords didn't match.");
> exit(1);
> }
> +   (void)crypt_newhash(s, "bcrypt,a", hash, sizeof(hash));
> explicit_bzero(s, sizeof(s));
> +   explicit_bzero(s1, sizeof(s1));
> }
>  
> /* set signal handlers */
> @@ -210,9 +213,9 @@ main(int argc, char *argv[])
> explicit_bzero(s, sizeof(s));
> break;
> }
> -   } else if (strcmp(s, s1) == 0) {
> +   } else if (crypt_checkpass(s, hash) == 0) {
> explicit_bzero(s, sizeof(s));
> -   explicit_bzero(s1, sizeof(s1));
> +   explicit_bzero(hash, sizeof(hash));
> break;
> }
> (void)putc('\a', stderr);
>