Re: Get Ruby 2.2 test suite passing
Ted Unangst wrote:
Jeremy Evans wrote:
As an aside, crypt(passwd, $2) returns : instead of NULL. I'm not
sure if that's a security issue, but I think it is and we should fix it.
I'll see if I can get a patch for that and send it to tech@.
This is a weird edge case where niels decided to make bcrypt() work
differently than crypt(). i don't really know why. I think null is the safer
return, and we should probably switch. we don't have code that looks for :
(and certainly no third party code ever does), but there is code that checks
for null.
like this.
Index: bcrypt.c
===
RCS file: /cvs/src/lib/libc/crypt/bcrypt.c,v
retrieving revision 1.52
diff -u -p -r1.52 bcrypt.c
--- bcrypt.c 28 Jan 2015 23:33:52 - 1.52
+++ bcrypt.c 18 Jul 2015 00:29:34 -
@@ -385,12 +385,9 @@ char *
bcrypt(const char *pass, const char *salt)
{
static chargencrypted[BCRYPT_HASHSPACE];
- static chargerror[2];
- /* How do I handle errors ? Return ':' */
- strlcpy(gerror, :, sizeof(gerror));
if (bcrypt_hashpass(pass, salt, gencrypted, sizeof(gencrypted)) != 0)
- return gerror;
+ return NULL;
return gencrypted;
}
This feels so much safer
Re: Get Ruby 2.2 test suite passing
Stuart Henderson wrote: On 2015/07/17 20:24, Ted Unangst wrote: Jeremy Evans wrote: As an aside, crypt(passwd, $2) returns : instead of NULL. I'm not sure if that's a security issue, but I think it is and we should fix it. I'll see if I can get a patch for that and send it to tech@. This is a weird edge case where niels decided to make bcrypt() work differently than crypt(). i don't really know why. I think null is the safer return, and we should probably switch. we don't have code that looks for : (and certainly no third party code ever does), but there is code that checks for null. Solar had some concerns about crypt returning null in the past, there's a thread starting at http://www.openwall.com/lists/oss-security/2011/11/15/1 which might be worth a read. moving to tech. Programs may not be checking null, but they're definitely not checking for strange alternative strings. At least null is standard. Now, returning : or * or whatever works in some cases because that's not the input string. But what happens when a program takes the return and saves it to the database? Then you *do* have a string that matches. (i think solar then fixed this by alternating between *1 or *2 but now we're getting really deep into the rabbit hole.) my perspective is: absent clear knowledge of what programs are doing, attempts to second guess them in a library function are perilous. let us be standards compliant, and then at least any resulting holes are clearly the program's fault. have i mentioned that i think the whole crypt(3) api can die in a fire?
Re: Get Ruby 2.2 test suite passing
On Fri, Jul 17, 2015 at 06:48:31PM -0600, Theo de Raadt wrote: my perspective is: absent clear knowledge of what programs are doing, attempts to second guess them in a library function are perilous. let us be standards compliant, and then at least any resulting holes are clearly the program's fault. such programs always deference the pointer. So I agree strongly with a NULL error, rather than something hacky like errno modification. The only objection I can see is something stupid that does not check the error condition, derefs NULL, drops a core file in an insecure place, and therefore leaks information. To my mind this is a buggy program, combined with an insecure configuration, and we shouldn't be trying to save people from their own stupid and make it worse.. NULL sounds right to me.
Re: Get Ruby 2.2 test suite passing
The only objection I can see is something stupid that does not check the error condition, derefs NULL, drops a core file in an insecure place, and therefore leaks information. To my mind this is a buggy program, combined with an insecure configuration, and we shouldn't be trying to save people from their own stupid and make it worse.. I am hoping to see that happen!
Re: Get Ruby 2.2 test suite passing
my perspective is: absent clear knowledge of what programs are doing, attempts to second guess them in a library function are perilous. let us be standards compliant, and then at least any resulting holes are clearly the program's fault. such programs always deference the pointer. So I agree strongly with a NULL error, rather than something hacky like errno modification.
Re: Get Ruby 2.2 test suite passing
Ted Unangst wrote:
Jeremy Evans wrote:
As an aside, crypt(passwd, $2) returns : instead of NULL. I'm not
sure if that's a security issue, but I think it is and we should fix it.
I'll see if I can get a patch for that and send it to tech@.
This is a weird edge case where niels decided to make bcrypt() work
differently than crypt(). i don't really know why. I think null is the safer
return, and we should probably switch. we don't have code that looks for :
(and certainly no third party code ever does), but there is code that checks
for null.
like this.
Index: bcrypt.c
===
RCS file: /cvs/src/lib/libc/crypt/bcrypt.c,v
retrieving revision 1.52
diff -u -p -r1.52 bcrypt.c
--- bcrypt.c28 Jan 2015 23:33:52 - 1.52
+++ bcrypt.c18 Jul 2015 00:29:34 -
@@ -385,12 +385,9 @@ char *
bcrypt(const char *pass, const char *salt)
{
static chargencrypted[BCRYPT_HASHSPACE];
- static chargerror[2];
- /* How do I handle errors ? Return ':' */
- strlcpy(gerror, :, sizeof(gerror));
if (bcrypt_hashpass(pass, salt, gencrypted, sizeof(gencrypted)) != 0)
- return gerror;
+ return NULL;
return gencrypted;
}
