Re: htpasswd: use crypt_newhash instead of bcrypt API
Michal Mazurek wrote: > On 15:31:50, 10.06.17, Ted Unangst wrote: > > > @@ -58,17 +58,29 @@ The provided > > > .Fa password > > > is randomly salted and hashed and stored in > > > .Fa hash . > > > +.Fa hash > > > +must already be allocated, and > > > +.Fa hashsize > > > +must contain its size, which cannot be less than 61 bytes. > > > > that's an implementation detail. if we're advising a limit, i think we > > should say 128 or so. > > How come? Tracing the code of crypt_newhash() we end up in > bcrypt_hashpass(), which has the following check: > > if (encryptedlen < BCRYPT_HASHSPACE) > goto inval; > > BCRYPT_HASHSPACE is defined thus: > libc/crypt/bcrypt.c:#define BCRYPT_HASHSPACE61 Part of the design criteria for the crypt_newhash function is that it support other hashes as well. We happen to only support bcrypt (and that's unlikely to change) but there's no reason to hardcode that assumption into an interface designed to abstract away such concerns. PASS_MAX (_PASSWORD_LEN) is 128, though it suffers from nul not included disease, so I don't know if we want to specify the limit in terms of that constant. In practice, however, if you look at encrypt/encrypt.c it uses char buffer[_PASSWORD_LEN]; which is missing the nul, but nevertheless 128. passwd does the same. so we can use the constant, or just say 128.
Re: htpasswd: use crypt_newhash instead of bcrypt API
On 15:31:50, 10.06.17, Ted Unangst wrote: > > @@ -58,17 +58,29 @@ The provided > > .Fa password > > is randomly salted and hashed and stored in > > .Fa hash . > > +.Fa hash > > +must already be allocated, and > > +.Fa hashsize > > +must contain its size, which cannot be less than 61 bytes. > > that's an implementation detail. if we're advising a limit, i think we > should say 128 or so. How come? Tracing the code of crypt_newhash() we end up in bcrypt_hashpass(), which has the following check: if (encryptedlen < BCRYPT_HASHSPACE) goto inval; BCRYPT_HASHSPACE is defined thus: libc/crypt/bcrypt.c:#define BCRYPT_HASHSPACE61 -- Michal Mazurek
Re: htpasswd: use crypt_newhash instead of bcrypt API
Michal Mazurek wrote:
> When talking about this with mulander@ it came out that the docs could
> use a touch.
>
> The commit message for the diff that didn't update the docs was:
>
> permit "bcrypt" as an alias for "blowfish". this is, after all, what
> 99% of the world calls it.
> allow just "bcrypt" without params to mean auto-tune ("bcrypt,a").
> default remains 8 rounds (for now)
>
> Comments? OK?
>
> Index: lib/libc/crypt/crypt_checkpass.3
> ===
> RCS file: /cvs/src/lib/libc/crypt/crypt_checkpass.3,v
> retrieving revision 1.9
> diff -u -p -r1.9 crypt_checkpass.3
> --- lib/libc/crypt/crypt_checkpass.3 23 Jul 2015 22:20:02 - 1.9
> +++ lib/libc/crypt/crypt_checkpass.3 6 Jun 2017 19:06:59 -
> @@ -58,17 +58,29 @@ The provided
> .Fa password
> is randomly salted and hashed and stored in
> .Fa hash .
> +.Fa hash
> +must already be allocated, and
> +.Fa hashsize
> +must contain its size, which cannot be less than 61 bytes.
that's an implementation detail. if we're advising a limit, i think we
should say 128 or so.
i think the rest is fine.
Re: htpasswd: use crypt_newhash instead of bcrypt API
On 21:16:08, 6.06.17, Michal Mazurek wrote:
> When talking about this with mulander@ it came out that the docs could
> use a touch.
>
> The commit message for the diff that didn't update the docs was:
>
> permit "bcrypt" as an alias for "blowfish". this is, after all, what
> 99% of the world calls it.
> allow just "bcrypt" without params to mean auto-tune ("bcrypt,a").
> default remains 8 rounds (for now)
>
> Comments? OK?
Discussion about the defaults aside, any comments on the doc change? OK?
--
Michal Mazurek
Re: htpasswd: use crypt_newhash instead of bcrypt API
Michal Mazurek wrote: > Yes, the function seems a bit inconsistent, in that "bcrypt" means "bcrypt,a" > but NULL means "bcrypt,8". awolk@ points out that the function is used in > just a few places - src and some ports patches, so we should be able to > change it. Judging by the commit message the author was aware of this > discrepancy though, and marked is as "for now", so let's see what others say. this was complicated because it's trying to unify several different programs and everybody has their own idea of what the default should be. so it tries to squeeze in some useful behaviors without changing everything about how all the calling programs behave. at least now we're in a situation where we should be able to discuss changing one program at a time.
Re: htpasswd: use crypt_newhash instead of bcrypt API
On Tue, Jun 06, 2017 at 08:29:23PM +, Florian Obser wrote: > On Tue, Jun 06, 2017 at 08:49:32PM +0200, Adam Wolk wrote: > > On Tue, Jun 06, 2017 at 12:28:59PM -0600, Theo de Raadt wrote: > > > > The only thing against using automatic rounds would be having them > > > > guessed on a > > > > weaker machine and used on a more powerful server - doubt though that > > > > would ever > > > > pick something below 8 rounds. > > > > > > I don't see the concern. It has a lower bound. > > > > Attaching the diff with rounds changed to auto, results with 9 rounds on my > > server. > > > > OK florian@ > Committed, thanks!
Re: htpasswd: use crypt_newhash instead of bcrypt API
> On 20:49:05, 6.06.17, Jason McIntyre wrote: > > right now this man page suggests that people will use "bcrypt,a" > > to "automatically suggest rounds based on system performance". is > > that right? i'd have expected people to just use "bcrypt" (w/o > > args). Because you can't change everything in one step. Seriously, feel free to pull on this string, and test all the cases. > > in fact, why have "a" at all? why not just have the default > > as automatically selecting rounds, but you can optionally specify > > an amount of rounds? Waiting for a volunteer who understands the impact. > > sorry, i know that's not really the main concern of your diff. it just > > seems a bit weird, and that reflects in the way you're having to word > > this. > > Yes, the function seems a bit inconsistent, in that "bcrypt" means "bcrypt,a" > but NULL means "bcrypt,8". awolk@ points out that the function is used in > just a few places - src and some ports patches, so we should be able to > change it. Judging by the commit message the author was aware of this > discrepancy though, and marked is as "for now", so let's see what others say. Yes, let's change everything at once. Did you test what you propose? Your mail seems to be coming in before you could have changed it, done a full build, and a study. Am I correct?
Re: htpasswd: use crypt_newhash instead of bcrypt API
On Tue, Jun 06, 2017 at 08:49:32PM +0200, Adam Wolk wrote:
> On Tue, Jun 06, 2017 at 12:28:59PM -0600, Theo de Raadt wrote:
> > > The only thing against using automatic rounds would be having them
> > > guessed on a
> > > weaker machine and used on a more powerful server - doubt though that
> > > would ever
> > > pick something below 8 rounds.
> >
> > I don't see the concern. It has a lower bound.
>
> Attaching the diff with rounds changed to auto, results with 9 rounds on my
> server.
>
OK florian@
> ? htpasswd
> Index: htpasswd.c
> ===
> RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 htpasswd.c
> --- htpasswd.c5 Nov 2015 20:07:15 - 1.15
> +++ htpasswd.c6 Jun 2017 18:46:39 -
> @@ -47,7 +47,7 @@ int nagcount;
> int
> main(int argc, char** argv)
> {
> - char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
> + char tmpl[sizeof("/tmp/htpasswd-XX")];
> char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
> char *line = NULL, *login = NULL, *tok;
> int c, fd, loginlen, batch = 0;
> @@ -133,10 +133,8 @@ main(int argc, char** argv)
> explicit_bzero(pass2, sizeof(pass2));
> }
>
> - if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
> - errx(1, "salt too long");
> - if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
> - errx(1, "hash too long");
> + if (crypt_newhash(pass, "bcrypt,a", hash, sizeof(hash)) != 0)
> + err(1, "can't generate hash");
> explicit_bzero(pass, sizeof(pass));
>
> if (file == NULL)
--
I'm not entirely sure you are real.
Re: htpasswd: use crypt_newhash instead of bcrypt API
On 20:49:05, 6.06.17, Jason McIntyre wrote: > right now this man page suggests that people will use "bcrypt,a" > to "automatically suggest rounds based on system performance". is > that right? i'd have expected people to just use "bcrypt" (w/o > args). in fact, why have "a" at all? why not just have the default > as automatically selecting rounds, but you can optionally specify > an amount of rounds? > > sorry, i know that's not really the main concern of your diff. it just > seems a bit weird, and that reflects in the way you're having to word > this. Yes, the function seems a bit inconsistent, in that "bcrypt" means "bcrypt,a" but NULL means "bcrypt,8". awolk@ points out that the function is used in just a few places - src and some ports patches, so we should be able to change it. Judging by the commit message the author was aware of this discrepancy though, and marked is as "for now", so let's see what others say. -- Michal Mazurek
Re: htpasswd: use crypt_newhash instead of bcrypt API
On Tue, Jun 06, 2017 at 09:16:08PM +0200, Michal Mazurek wrote:
> When talking about this with mulander@ it came out that the docs could
> use a touch.
>
> The commit message for the diff that didn't update the docs was:
>
> permit "bcrypt" as an alias for "blowfish". this is, after all, what
> 99% of the world calls it.
> allow just "bcrypt" without params to mean auto-tune ("bcrypt,a").
> default remains 8 rounds (for now)
>
> Comments? OK?
>
the diff itself reads fine. one question:
> Index: lib/libc/crypt/crypt_checkpass.3
> ===
> RCS file: /cvs/src/lib/libc/crypt/crypt_checkpass.3,v
> retrieving revision 1.9
> diff -u -p -r1.9 crypt_checkpass.3
> --- lib/libc/crypt/crypt_checkpass.3 23 Jul 2015 22:20:02 - 1.9
> +++ lib/libc/crypt/crypt_checkpass.3 6 Jun 2017 19:06:59 -
> @@ -58,17 +58,29 @@ The provided
> .Fa password
> is randomly salted and hashed and stored in
> .Fa hash .
> +.Fa hash
> +must already be allocated, and
> +.Fa hashsize
> +must contain its size, which cannot be less than 61 bytes.
> The
> .Fa pref
> argument identifies the preferred hashing algorithm and parameters.
> +If set to
> +.Dv NULL
> +it defaults to
> +.Dq bcrypt,8 .
> Possible values are:
> .Bl -tag -width Ds
> -.It Dq bcrypt,
> +.It Dq bcrypt[,]
> The bcrypt algorithm, where the value of rounds can be between 4 and 31 and
> specifies the base 2 logarithm of the number of rounds.
> The special rounds value
> .Sq a
> automatically selects rounds based on system performance.
> +This is the default if rounds is omitted.
right now this man page suggests that people will use "bcrypt,a"
to "automatically suggest rounds based on system performance". is
that right? i'd have expected people to just use "bcrypt" (w/o
args). in fact, why have "a" at all? why not just have the default
as automatically selecting rounds, but you can optionally specify
an amount of rounds?
sorry, i know that's not really the main concern of your diff. it just
seems a bit weird, and that reflects in the way you're having to word
this.
jmc
> +.Dq blowfish
> +can be used as an alias for
> +.Dq bcrypt .
> .El
> .Sh RETURN VALUES
> .Rv -std crypt_checkpass crypt_newhash
> @@ -89,7 +101,9 @@ to
> .Er EINVAL
> if
> .Fa pref
> -is unsupported.
> +is unsupported, or the value of
> +.Fa hashsize
> +is insufficient.
> .Sh SEE ALSO
> .Xr crypt 3 ,
> .Xr login.conf 5 ,
>
> --
> Michal Mazurek
>
Re: htpasswd: use crypt_newhash instead of bcrypt API
When talking about this with mulander@ it came out that the docs could
use a touch.
The commit message for the diff that didn't update the docs was:
permit "bcrypt" as an alias for "blowfish". this is, after all, what
99% of the world calls it.
allow just "bcrypt" without params to mean auto-tune ("bcrypt,a").
default remains 8 rounds (for now)
Comments? OK?
Index: lib/libc/crypt/crypt_checkpass.3
===
RCS file: /cvs/src/lib/libc/crypt/crypt_checkpass.3,v
retrieving revision 1.9
diff -u -p -r1.9 crypt_checkpass.3
--- lib/libc/crypt/crypt_checkpass.323 Jul 2015 22:20:02 - 1.9
+++ lib/libc/crypt/crypt_checkpass.36 Jun 2017 19:06:59 -
@@ -58,17 +58,29 @@ The provided
.Fa password
is randomly salted and hashed and stored in
.Fa hash .
+.Fa hash
+must already be allocated, and
+.Fa hashsize
+must contain its size, which cannot be less than 61 bytes.
The
.Fa pref
argument identifies the preferred hashing algorithm and parameters.
+If set to
+.Dv NULL
+it defaults to
+.Dq bcrypt,8 .
Possible values are:
.Bl -tag -width Ds
-.It Dq bcrypt,
+.It Dq bcrypt[,]
The bcrypt algorithm, where the value of rounds can be between 4 and 31 and
specifies the base 2 logarithm of the number of rounds.
The special rounds value
.Sq a
automatically selects rounds based on system performance.
+This is the default if rounds is omitted.
+.Dq blowfish
+can be used as an alias for
+.Dq bcrypt .
.El
.Sh RETURN VALUES
.Rv -std crypt_checkpass crypt_newhash
@@ -89,7 +101,9 @@ to
.Er EINVAL
if
.Fa pref
-is unsupported.
+is unsupported, or the value of
+.Fa hashsize
+is insufficient.
.Sh SEE ALSO
.Xr crypt 3 ,
.Xr login.conf 5 ,
--
Michal Mazurek
Re: htpasswd: use crypt_newhash instead of bcrypt API
On Tue, Jun 06, 2017 at 12:28:59PM -0600, Theo de Raadt wrote:
> > The only thing against using automatic rounds would be having them guessed
> > on a
> > weaker machine and used on a more powerful server - doubt though that would
> > ever
> > pick something below 8 rounds.
>
> I don't see the concern. It has a lower bound.
Attaching the diff with rounds changed to auto, results with 9 rounds on my
server.
? htpasswd
Index: htpasswd.c
===
RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
retrieving revision 1.15
diff -u -p -r1.15 htpasswd.c
--- htpasswd.c 5 Nov 2015 20:07:15 - 1.15
+++ htpasswd.c 6 Jun 2017 18:46:39 -
@@ -47,7 +47,7 @@ int nagcount;
int
main(int argc, char** argv)
{
- char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
+ char tmpl[sizeof("/tmp/htpasswd-XX")];
char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
char *line = NULL, *login = NULL, *tok;
int c, fd, loginlen, batch = 0;
@@ -133,10 +133,8 @@ main(int argc, char** argv)
explicit_bzero(pass2, sizeof(pass2));
}
- if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
- errx(1, "salt too long");
- if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
- errx(1, "hash too long");
+ if (crypt_newhash(pass, "bcrypt,a", hash, sizeof(hash)) != 0)
+ err(1, "can't generate hash");
explicit_bzero(pass, sizeof(pass));
if (file == NULL)
Re: htpasswd: use crypt_newhash instead of bcrypt API
> The only thing against using automatic rounds would be having them guessed on > a > weaker machine and used on a more powerful server - doubt though that would > ever > pick something below 8 rounds. I don't see the concern. It has a lower bound.
Re: htpasswd: use crypt_newhash instead of bcrypt API
On Tue, Jun 06, 2017 at 02:20:38PM -0400, Bryan Steele wrote: > > > > - if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt)) > > - errx(1, "salt too long"); > > - if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash)) > > - errx(1, "hash too long"); > > + if (crypt_newhash(pass, "bcrypt,8", hash, sizeof(hash)) != 0) > > + err(1, "can't generate hash"); > > explicit_bzero(pass, sizeof(pass)); > > > > if (file == NULL) > > It should be possible to use the automatic rouds, i.e: "bcrypt,a" instead > of just hardcoding 8. > I wasn't sure about introducing that change, went the minimal changes from existing behavior. The only thing against using automatic rounds would be having them guessed on a weaker machine and used on a more powerful server - doubt though that would ever pick something below 8 rounds. Roughly, if the general consensus is to move to automatic rounds then I'm all for it. Side note, does anyone know why crypt_newhash defaults to blowfish? The docs don't mention it.
Re: htpasswd: use crypt_newhash instead of bcrypt API
On Tue, Jun 06, 2017 at 07:43:02PM +0200, Adam Wolk wrote:
> Hi tech@
>
> While reading htpasswd and htpasswd handling in httpd I noticed that both use
> different APIs to handle encrypting/decrypting the passwords.
>
> - htpasswd uses the bcrypt API
> - httpd uses the new crypt API
>
> The documentation for bcrypt states:
> These functions are deprecated in favor of crypt_checkpass(3) and
> crypt_newhash(3).
>
> I'm attaching a diff moving htpasswd to the new API. Tested with httpd from
> 6.1 with a htpasswd generated with the diff applied on current.
>
> Feedback? OK's?
>
> Regards,
> Adam
> ? htpasswd
> Index: htpasswd.c
> ===
> RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 htpasswd.c
> --- htpasswd.c5 Nov 2015 20:07:15 - 1.15
> +++ htpasswd.c6 Jun 2017 17:26:31 -
> @@ -47,7 +47,7 @@ int nagcount;
> int
> main(int argc, char** argv)
> {
> - char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
> + char tmpl[sizeof("/tmp/htpasswd-XX")];
> char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
> char *line = NULL, *login = NULL, *tok;
> int c, fd, loginlen, batch = 0;
> @@ -133,10 +133,8 @@ main(int argc, char** argv)
> explicit_bzero(pass2, sizeof(pass2));
> }
>
> - if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
> - errx(1, "salt too long");
> - if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
> - errx(1, "hash too long");
> + if (crypt_newhash(pass, "bcrypt,8", hash, sizeof(hash)) != 0)
> + err(1, "can't generate hash");
> explicit_bzero(pass, sizeof(pass));
>
> if (file == NULL)
It should be possible to use the automatic rouds, i.e: "bcrypt,a" instead
of just hardcoding 8.
