Open Bug 830880 Opened 11 years ago Updated 2 years ago

Remove/replace mfbt/SHA1

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Type:
defect

Tracking

()

People

(Reporter: briansmith, Unassigned)

References

Details

1. We use NSS for all cryptographic operations (*) specifically to support the needs of our partners (particularly Red Hat) that need Firefox to use FIPS140-certified implementations of all the algorithms, since NSS is certified. (It would be great if Bob could clarify what exactly the requirement is here.) Although generally we currently leave it up to Red Hat to deal with the certification stuff, we should avoid making things unnecessarily difficult for them (if for no other reason than to help pay them back for all the crypto work that Red Hat does for us), especially when it is convenient for us to support their needs.

2. SHA1 is on the verge of not being considered a secure hash algorithm (see recent research results) and we should not be offering it as the default most convenient algorithm for Gecko developers to use. We should offer one of the SHA2s (SHA-256, SHA-384, SHA-512) instead, and we should be actively discouraging the use of SHA-1 beyond any legacy needs we have (e.g. interoperability needs within TLS).

3. I had suggested moving some crypto stuff into mfbt/ before and that got shot down immediately on the grounds that crypto (glue) stuff doesn't belong in mfbt. (I hope I am remembering the conclusion of this discussion correctly. cc'd Brendan to make sure I understand it correctly.)

4. There is now a wrapper around NSS for C++ code to use:

     Digest digest;
     nsresult rv = digest.DigestBuf(SEC_OID_SHA256, mybuffer, myBufferLen);
     NS_ENSURE_SUCCESS(rv, rv);
     // result is in digest.get().data with length digest.get().len

Notice that the Digest class supports multiple hash algorithms and also it handles the buffering and buffer sizing automatically.

I suggest we just remove mftb/SHA1 and use the Digest class I wrote. I am happy to modify the interface of the Digest class to make it more convenient to use for non-PSM/NSS developers, and I'm happy to change the implementation of it to use the NSSLOWHASH_* API instead, so that it can be used when NSS hasn't yet been initialized yet (or has been shut down), as I know that is a requirement for one user of the API.

(*) I think WebRTC is currently an exception, but that means that WebRTC must be disabled in certified configurations. We should avoid the need to disable features to meet the certification requirements.
There are cases where hashing is used to just check content for modifications.  Like in case of offline cache update check using md5 (the weakest and best performing algo).

I was glad we were not dependent on NSS provided hashing functions to save some performance.  Sure, it may only be used for cases where security is not a concern.

Or, we can change check-content-changes functions to use some different CRC-like algorithm that would not be considerable for security checks but would save performance and fit the purpose.  (Out of scope of this bug.)
(In reply to Honza Bambas (:mayhemer) from comment #1)
> I was glad we were not dependent on NSS provided hashing functions to save
> some performance.  Sure, it may only be used for cases where security is not
> a concern.

My concern is about uses of mfbt/SHA1 that *are* being used to get some kind of security assurance. If we think it is important to have a high-performance hash function that isn't secure and NSS's performance isn't good enough, then (a) I'd like to improve NSS's performance so that it becomes fast enough, and (b) we should name the hash functions that are designed for use only in non-security-critical applications something that makes it clear that it should not be used in security-critical contexts, e.g. "SHA1_Not_Secure".

> Or, we can change check-content-changes functions to use some different
> CRC-like algorithm that would not be considerable for security checks but
> would save performance and fit the purpose.  (Out of scope of this bug.)

I don't care about the use of MD5 if it isn't being used as a secure hash algorithm.
We could also rename away from SHA1 to not promise it to be SHA1, which might satisfy some of the concerns here.  My understanding is current uses don't care about it being SHA1 specifically.  Dunno about names, exactly, as I don't know much about the performance characteristics of SHA1, which I'd want a name to hint at (and perhaps have an Insecure prefix to indicate that characteristic).
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> We could also rename away from SHA1 to not promise it to be SHA1, which
> might satisfy some of the concerns here.  My understanding is current uses
> don't care about it being SHA1 specifically.  Dunno about names, exactly, as
> I don't know much about the performance characteristics of SHA1, which I'd
> want a name to hint at (and perhaps have an Insecure prefix to indicate that
> characteristic).

The only user of SHA1Sum in the tree (that I find) is nsIDOMFile (nsDOMFile.cpp), which does seem to require a secure hash function to avoid collisions. So, I'm not sure how important it is to have a "not secure" hash function in mfbt/.
(In reply to comment #4)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> > We could also rename away from SHA1 to not promise it to be SHA1, which
> > might satisfy some of the concerns here.  My understanding is current uses
> > don't care about it being SHA1 specifically.  Dunno about names, exactly, as
> > I don't know much about the performance characteristics of SHA1, which I'd
> > want a name to hint at (and perhaps have an Insecure prefix to indicate that
> > characteristic).
> 
> The only user of SHA1Sum in the tree (that I find) is nsIDOMFile
> (nsDOMFile.cpp), which does seem to require a secure hash function to avoid
> collisions. So, I'm not sure how important it is to have a "not secure" hash
> function in mfbt/.

There's also xpcom/build/mozPoisonWriteMac.cpp.
> The only user of SHA1Sum in the tree (that I find) is nsIDOMFile (nsDOMFile.cpp), which 
> does seem to require a secure hash function to avoid collisions.

It's a memory reporter.  The worst thing that happens if an attacker generates a collision is that your memory report is wrong.

I don't mind if we removed SHA1 from MFBT if the NSS-based version is no less convenient to use (the Digest class seems fine offhand) and if instantiating NSS in child processes doesn't require us to pay a significant memory or startup-time cost.
(I know this sounds like premature optimization, and to some extent it is.  But the alternative is fixing bugs when our partners find them, and under the B2G release schedule, that is always too late for comfort.)
It's worth noting here that SHA1.cpp was only added because we *couldn't* use NSS in mozPoisonWriteMac.cpp, because NSS isn't available that late in shutdown.  I don't know what specifically bug 1073302 wanted to use SHA2 for, but there are semi-legitimate uses that can't be handled with NSS.
NSS also doesn't export just a hash interface as far as I can tell.

I recently used mfbt/SHA1 to verify correct demuxing in our media code, and I was glad a simple interface was available. That doesn't require a hash robust against attack, but when it's the easiest interface around it's tempting to use it, even for things with more privacy implications than debug logs.
There's PK11_HashBuf:

SECStatus PK11_HashBuf(SECOidTag hashAlg, unsigned char *out,
		       const unsigned char *in, PRInt32 len);

Use it like so:

    unsigned char certFingerprint[SHA256_LENGTH];
    rv = PK11_HashBuf(SEC_OID_SHA256, certFingerprint,
                      entry.cert->derCert.data,
                      static_cast<int32_t>(entry.cert->derCert.len));

But yes, you do have to use it when NSS is available (and guarantee that NSS won't shut down while you're using it). There's also nsICryptoHash, which will take care of the NSS details for you.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.