Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhanced Entropy: sha384 and NUL bytes #15

Closed
Indigo744 opened this issue Apr 30, 2018 · 13 comments
Closed

Enhanced Entropy: sha384 and NUL bytes #15

Indigo744 opened this issue Apr 30, 2018 · 13 comments
Assignees
Labels

Comments

@Indigo744
Copy link

Indigo744 commented Apr 30, 2018

Hello,

I was reading this article (which actually recommend this library) about using bcrypt for password hashing.

It suggests to pass the password through SHA384 before using bcrypt in order to circumvent the bcrypt limitation.

I was glad to find that your library implemented it through the enhancedEntropy parameter.

However, looking at the code, I couldn't find any mention of base64 encoding after the SHA384 and prior bcrypt.

base64 is suggested in the article because:

Bcrypt truncates on NUL bytes.

And

A base64-encoded hash is guaranteed to not contain NUL bytes

Thanks for your insight.

EDIT Adding some reference implementation seen elsewhere:
In PasswordLock PHP library, they effectively perform base64 after sha384.
In passlib Python library, they also perform base64 after sha256.

@ChrisMcKee
Copy link
Collaborator

ChrisMcKee commented Apr 30, 2018

I'll take a look in a bit, I've looked into this before, there's a test around null byte in strings

I've added further tests to the null check (it runs the test cases against enhanced and normal)
I can't surface anything using those.

The spec says we should treat null like any other character so I don't believe we've any issues with null bytes surfaced through direct cryptraw access (which is a private method) or direct-via-hmac.

image

I'm not discounting adding in b64 wrapping the hmac as this seems to be the default in multiple nix platforms and I love compatibility. I need to get some test vectors so I can validate mine vs say php/ruby etal.

Didn't realise @paragonie-scott had linked it.

@ChrisMcKee ChrisMcKee self-assigned this Apr 30, 2018
@Indigo744
Copy link
Author

Indigo744 commented Apr 30, 2018

Well, the NULL char is actually not related to the fact that the password can contain a NULL char, but instead that the sha384 function can return a string an array of bytes containing a NULL char.

See this Paragonie article (once again!) but it's about sha256:

There is a nontrivial chance that one of the raw bytes in the hash will be 0x00. The sooner this byte appears in the string, the cost of finding a collision becomes exponentially cheaper.
For example, both 1]W and @1$ produce a SHA-256 hash output that begins with ab00.

@Indigo744
Copy link
Author

Indigo744 commented Apr 30, 2018

Run a quick test with SHA384:

The sha384 hash value of d27a37starts with a NULL byte.
00b6f66181bc7659bef1a7ef89068b06e800f5a6215202d86f2ec1914b13e3a0042842d343b21652d8cf08f0feaea3bb

The sha384 hash value of 956ed9 has a second NULL byte
c700925b7e08d2135110544e3bb4efdfa4ad36966ee601db17851635e3ccff88f1d7a9a6b61fd0515afec94a82183d30

And the sha384 hash value of d3d807 has the 25th byte as NULL
b35533db641f47cccecca17687a6751a939ea64b5c2a3f3ce20061cfcd2eace1f20453bf88024fd73a45b5f6d1fc4723

These values could be used as test passwords when enhancedEntropy is used and check if it gets truncated...

@ChrisMcKee
Copy link
Collaborator

I'm aware of the why, I was trying to avoid a rambling answer.
If the HMAC passes a null value along the resultant byte array would be the same as passing any other byte array containing a null. The existing code handles null bytes as part of meeting the 2a standard which added.

* the string must be UTF-8 encoded
* the null terminator must be included

@ChrisMcKee
Copy link
Collaborator

ChrisMcKee commented Apr 30, 2018

Just to show it at a lower level;

image

I don't push the crypt-raw test as the method is private; as is decode. I may add them and change private to internal so the tests can access those areas of the method.

Similar test using the value you provided

image

You'll probably find this interesting also https://blog.ircmaxell.com/2015/03/security-issue-combining-bcrypt-with.html < most annoying part about this implementation but of course being that it was at the module level so pretty ubiquitous.

@Indigo744
Copy link
Author

So this means that the C# implementation of bcrypt isn't affected by the NULL byte issue?
This is a good news, mainly for people already using it 😄

However, as you noted, there is still a compatibility problem, as other libs uses base64 and not this one.
But then you would have the issue of the retrocompatibility of your own lib.
Maybe a new and different option enhancedEntropyEncoded would to the trick... Albeit adding more complexity to the maintenance...

@ChrisMcKee
Copy link
Collaborator

@Indigo744 tbh I'll just bump the major version and add a legacy option. I want to make the HMAC an option between current and 512... it adds bugger all real gain but some folk seem to want it 🙄

Ta for the nudge I'd been meaning to finish off these changes and add in better test coverage for a while

@Indigo744
Copy link
Author

Since you are adding sha512, maybe also add an option to add sha256 for interoperability with other lib (such as Passlib)? I don't know if it's worth it actually...

Anyway, I'm glad we were able to work things out and that everything is OK and secure 😄
Thanks for actively maintaining this lib. You're doing an awesome job.

@Indigo744
Copy link
Author

Oh, I just noticed that Passlib uses a specific algorithm name in the crypt format: $bcrypt-sha256$

A quick search tells me they are the only one doing it (I may be wrong tho). However, it looks interesting, since it clearly explains the underlying algorithm.

@ChrisMcKee
Copy link
Collaborator

ChrisMcKee commented May 1, 2018

@Indigo744 suprised the didnt go with $2a-sha2$ but as it stands I think they ditched the Modular Crypto Format for PHF (https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md).

It would be handy from a standpoint of auto-picking which route needs to be taken (similar to extracting salt/workload). Hmmmmm 🤔

@Indigo744
Copy link
Author

You're right! I didn't know this format... which seems to be the the format for Argon2.
It looked so similar with MCF at first glance I didn't even checked. This is actually good to know!

Well, since this is a bcrypt lib, I don't think there is a point of using PHF... It should remain compatible with crypt.
Anyway, only passlib seems to do this. Even Paragonie's own implementation doesn't do this. I guess it's safe to continue as before.

@ChrisMcKee
Copy link
Collaborator

ChrisMcKee commented May 1, 2018

Yeah, I won't be breaking the version standard for normal bcrypt.
Could be useful in the enhanced bit for dealing with hashes created using SHA256 when the libs set to use 512.
The mass majority will be using the standard crypt route; the likes of dropbox adding a pepper to the user key then hmac512 the b64 then bcrypt. So the base routes are the most important. The enhanced stuffs just basically wrapping some of this up.
One to ponder, though I'm not wholly keen on altering the hash format, it surfaces the hash type used for decryption; but in the same breath that could be a negative thing.

I'd considered upping the workload but with the crappy processor bug patches I'd risk shooting people in the face by accident.

@Indigo744
Copy link
Author

Alright. Looking forward the new version. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants