Re: crypt_checkpass.3: mention additional failure case for crypt_newhash
> On Jul 21, 2017, at 10:24 PM, Ted Unangst wrote: > > Scott Cheloha wrote: >> crypt_newhash(3) will return -1 and set errno to EINVAL if hashsize is >> too small to accommodate bcrypt's hash space. I imagine this would >> also be the case if anything other than bcrypt were supported. > > i went ahead and reworked the page a bit. i think it clarifies a few other > edge conditions better now. It does. I didn't even know you could omit ",a" to equal effect.
Re: crypt_checkpass.3: mention additional failure case for crypt_newhash
Scott Cheloha wrote: > crypt_newhash(3) will return -1 and set errno to EINVAL if hashsize is > too small to accommodate bcrypt's hash space. I imagine this would > also be the case if anything other than bcrypt were supported. i went ahead and reworked the page a bit. i think it clarifies a few other edge conditions better now.
crypt_checkpass.3: mention additional failure case for crypt_newhash
Hi,
crypt_newhash(3) will return -1 and set errno to EINVAL if hashsize is
too small to accommodate bcrypt's hash space. I imagine this would
also be the case if anything other than bcrypt were supported.
Test program:
#include
#include
#include
int
main(int argc, char *argv[])
{
char buf[20], hash1[61], hash2[60];
memset(buf, 'a', sizeof(buf));
buf[sizeof(buf) - 1] = '\0';
if (crypt_newhash(buf, "bcrypt,a", hash1, sizeof(hash1)) == -1)
err(1, "crypt_newhash 1");
if (crypt_newhash(buf, "bcrypt,a", hash2, sizeof(hash2)) == -1)
err(1, "crypt_newhash 2");
return 0;
}
Attached patch documents the case.
--
Scott Cheloha
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.321 Jul 2017 22:33:51 -
@@ -89,7 +89,12 @@ to
.Er EINVAL
if
.Fa pref
-is unsupported.
+is unsupported,
+or if
+.Fa pref Ns 's
+hash space exceeds
+.Fa hashsize
+bytes.
.Sh SEE ALSO
.Xr crypt 3 ,
.Xr login.conf 5 ,
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.
htpasswd: use crypt_newhash instead of bcrypt API
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.c 5 Nov 2015 20:07:15 - 1.15
+++ htpasswd.c 6 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)
Re: crypt_newhash
On Sun, Nov 23, 2014 at 01:42, Jonas 'Sortie' Termansen wrote: > > This can be easily solved if crypt_checkpass checks the hash doesn't start > with > '$' and then calls the utility crypt_hashpass directly (will need to be > visible > from outside its current translation unit), and storing the temporary hash > in a > little stack buffer (21 bytes is sufficient). The DES tables must also be > safely > initialized with a once lock, or precomputed at compile time. Alternatively, > support for checking older hashes could be dropped altogether. Cleaing up the DES crypt code was on my list for a while, but I got kind of sidetracked. Fixing the code now competes with deleting the code for priority, but since we probably can't do without it entirely, I'll have to fix it eventually. The lack of thread safety should be considered a bug in the implementation. > > Third, the purpose of the hashlen parameter to bcrypt_newhash(3) and > crypt_newhash(3) is undocumented, but judging from its name and uses > across the > tree, it obviously looks like the size of the hash buffer for bounds-checking > purposes. bcrypt_hashpass(3) never uses the size of the buffer, despite > printing > and base64 encoding into it! Yup. As you noticed, the existing code still has more than a few remnants of origins.
Re: crypt_newhash
Hi, On 11/21/2014 06:37 AM, Ted Unangst wrote: > Sneaking the change in was easier than I thought, and upon further > review, some of the extra login_cap handling was unnecessary for most > callers. Updated to use a string parameter (arguments regarding its > defintion postponed til next week :)). Thanks for the quick feedback. Sounds good. :) I've been looking through the code more closely and I see some more problems: First, crypt_checkpass makes a call to crypt() to be compatible with old hashes. However it doesn't check the return value, which is NULL if the hash is invalid, in which case the next code will segfault. Secondly, the call to crypt isn't thread-safe. The crypt call only happens if the hash isn't blowfish (thus wasn't made with crypt_newhash). However, since the flak blog post criticizes crypt for being thread unsafe, and the manual page for crypt_checkpass(3) doesn't mention threading issues and looks safe, one would not expect older inputs to be dangerous in threaded programs. This can be easily solved if crypt_checkpass checks the hash doesn't start with '$' and then calls the utility crypt_hashpass directly (will need to be visible from outside its current translation unit), and storing the temporary hash in a little stack buffer (21 bytes is sufficient). The DES tables must also be safely initialized with a once lock, or precomputed at compile time. Alternatively, support for checking older hashes could be dropped altogether. Third, the purpose of the hashlen parameter to bcrypt_newhash(3) and crypt_newhash(3) is undocumented, but judging from its name and uses across the tree, it obviously looks like the size of the hash buffer for bounds-checking purposes. bcrypt_hashpass(3) never uses the size of the buffer, despite printing and base64 encoding into it! The function should verify the buffer is large enough (rather than assuming the caller uses _PASSWORD_LAN) before doing any work and failing with ERANGE, so the caller can use a bigger buffer. Some miscellaneous notes: It would be good bcrypt_newhash and bcrypt_checkpass set errno (to EINVAL and EACCES) upon failure like their crypt_checkpass and crypt_newhash counterparts. bcrypt_gensalt is forward-declared in bcrypt.c and doesn't need to be for any reason, and pwd.h supplies the function prototype regardless. The u_key_perm array in crypt.c is set, but not used and serve no purpose. The crypt(3) code likes to pretend the DES encryption functions can fail, but they have no error cases except bad parameters, and that can easily be seen never happens. Jonas
Re: crypt_newhash
On Thu, Nov 20, 2014 at 20:22, Ted Unangst wrote: > So far only passwd uses the new function; it is still easy for me to > change the interface. (The only constraint being that I need to > coordinate library changes with other developments so it may not be an > immediate change.) Sneaking the change in was easier than I thought, and upon further review, some of the extra login_cap handling was unnecessary for most callers. Updated to use a string parameter (arguments regarding its defintion postponed til next week :)). Thanks for the quick feedback.
Re: crypt_newhash
On Thu, Nov 20, 2014 at 19:29, Jonas 'Sortie' Termansen wrote: > Hi, > > The flak blog just had an interesting post about why the old crypt() > interface > should be replaced, and on the new crypt_newhash() and crypt_checkpass() that > were added to OpenBSD. I would like to see this API become portable and > perhaps > standardized, but crypt_newhash is currently tied to login_cap_t, which is > not a > portable abstraction. The current synopsis is: > > #include > int crypt_newhash(const char *pass, login_cap_t *lc, char *hash, size_t > hashlen); > > The purpose of the lc parameter is to determine which algorithm to use: The > implementation merely does a login_getcapstr(lc, "localcipher", NULL, > NULL) call > to convert that into a string telling what algorithm to use. If lc is > NULL, then > it defaults to a reasonable algorithm. > > It would be superior to move the login_getcapstr call to the caller and > instead > have a string parameter. This removes the association with login_cap and > it can > be moved to or alongside the other functions. It also > becomes > more like crypt (where various algorithms can be requested) and thus more > reusable in other situations than local-user authentication (like a > web-server). Yes. Deciding which level of abstraction to incorporate was part of the fun. The factors in favor of login_cap is that the getcapstr code is basically boilerplate that all OpenBSD callers are going to duplicate. The various passwd/user/encrypt programs all have access to login cap. You have some good points that the interface could be changed to be more accomodating to other users. The bcrypt extension functions are standalone precisely for that reason, so that the code could be packaged and reused by anybody. The crypt_newhash function felt more like a "system" interface, particular to OpenBSD, but it is not strictly necessary for that to be the case. Parity with checkpass would be a bonus. So far only passwd uses the new function; it is still easy for me to change the interface. (The only constraint being that I need to coordinate library changes with other developments so it may not be an immediate change.)
crypt_newhash
Hi, The flak blog just had an interesting post about why the old crypt() interface should be replaced, and on the new crypt_newhash() and crypt_checkpass() that were added to OpenBSD. I would like to see this API become portable and perhaps standardized, but crypt_newhash is currently tied to login_cap_t, which is not a portable abstraction. The current synopsis is: #include int crypt_newhash(const char *pass, login_cap_t *lc, char *hash, size_t hashlen); The purpose of the lc parameter is to determine which algorithm to use: The implementation merely does a login_getcapstr(lc, "localcipher", NULL, NULL) call to convert that into a string telling what algorithm to use. If lc is NULL, then it defaults to a reasonable algorithm. It would be superior to move the login_getcapstr call to the caller and instead have a string parameter. This removes the association with login_cap and it can be moved to or alongside the other functions. It also becomes more like crypt (where various algorithms can be requested) and thus more reusable in other situations than local-user authentication (like a web-server). Jonas
