Bug 1158489 (CVE-2015-7575)

Prevent MD5 Downgrade in TLS 1.2 Signatures

RESOLVED FIXED in Firefox 43

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: karthikeyan.bhargavan, Assigned: mt)

Tracking

({sec-moderate})

trunk
3.21

Firefox Tracking Flags

(firefox39 wontfix, firefox40- wontfix, firefox41+ wontfix, firefox42+ wontfix, firefox43+ fixed, firefox44+ fixed, firefox45+ fixed, firefox-esr31 affected, firefox-esr3844+ fixed)

Details

(Whiteboard: [adv-main43+][adv-esr38.6+])

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Since 2011 (Bug 650355), NSS was supposed to "stop accepting MD5 as a
hash algorithm in signatures".  While this applies to signatures
within certificates, NSS still allows MD5 signatures in the server
signature in the TLS 1.2 ServerKeyExchange message. This exposes NSS
based clients such as Firefox to theoretical collision-based forgery
attacks. More pragmatically, we believe it is important to draw
attention to this bug before TLS 1.3 is implemented, because a similar
bug in the processing of the TLS 1.3 ServerCertificateVerify message
would lead to a serious server impersonation attack.


To see the bug, connect with Firefox to an openssl 1.0.1m server with
a modified ssl/s3_srvr.c (attached) which always signs the
ServerKeyExchange with RSA-MD5.  When Firefox connects to a server,
its signature algorithms extension only advertises the following
signature/hash algorithms: SHA256/RSA, SHA384/RSA, SHA1/RSA,
SHA256/ECDSA, SHA384/ECDSA, SHA1/ECDSA, SHA256/DSA, SHA1/DSA. Notably,
it should not allow any MD5 signature. However, when our server sends
it an RSA-MD5 signature, NSS does not check that this algorithm is
included in the allowed algorithms and quietly accepts it.

The root cause for this bug is that TLS 1.2 allows the signer to
choose any combination of signature and hash algorithm (even ECDSA-MD5
if it so wishes) and it is up to the verifier to check that the
algorithm is acceptable.  This is a change from TLS 1.1 (which only
allowed MD5SHA1); notably the current TLS 1.3 draft also allows MD5
signatures.

We note that NSS allows any signature/hash algorithm combination in
ServerKeyExchange but in the ClientCertificateVerify it forces the
client to use the hash algorithm that will be used in the Finished
message (SHA-256 for TLS 1.2, MD5SHA1 for TLS 1.1) again ignoring the
signature algorithms extension. We anticipate that a future update to
NSS would also generalize the client signature (to support SHA-512,
for example); and this report warns that any such update should be
careful to avoid MD5 downgrades.

The impact of the bug depends on whether an adversary can trigger
meaningful collisions in TLS client and server signatures. Folklore
about TLS has previously suggested that only second preimage attacks
"break" TLS signatures, collisions do not. We attach a draft research
report that shows that collision resistance is essential for the
security of client and server signatures in TLS. We show how
chosen-prefix collisions in the hash function can be used to forge
client signatures in TLS 1.2 and server signatures in TLS 1.3.
We believe this result is of independent interest. Collisions in
any of the handshake hashes in TLS can be translated to real
man-in-the-middle attacks.

We would be happy to provide further tests (such as a test server)
and discuss countermeasures. The straightforward fix is to check
that the signature/hash algorithm in the ServerKeyExchange message
is one of the algorithms sent in the ClientHello.
(In reply to Karthikeyan Bhargavan from comment #0)
> We would be happy to provide further tests (such as a test server)
> and discuss countermeasures. The straightforward fix is to check
> that the signature/hash algorithm in the ServerKeyExchange message
> is one of the algorithms sent in the ClientHello.

The conclusion of your paper is that collision resistance is a requirement for the digest algorithm. We basically don't consider SHA-1 to be collision resistant any more. Consequently, SHA-1 should be treated the same way and all the SHA-1 algorithms should be removed from the supported signature algorithms extension.

While there are probably few compatibility concerns regarding removing support for MD5 signatures (especially since they aren't advertised in the extension), there may be more compatibility concerns for removing SHA-1. It would be good to see results of scans indicating (a) How many servers choose a digest weaker than SHA-256 when SHA-1 is advertised in the supported signature algorithms extension, and (b) how many servers break if SHA-1 is removed. But, dropping MD5 support shouldn't be blocked on that.
(Reporter)

Comment 3

4 years ago
I agree that we should prepare for SHA-1 being deprecated, but as far as I know
chosen-prefix attacks for SHA-1 are still not considered practical, although 
they're in the "coming soon" category.

The report focuses on the use of hash functions in signatures, since this is
configurable since TLS 1.2. However, the same argument holds for the use of
hash functions in the Finished message. In TLS 1.2 we use SHA-256 which is considered
safe, but in earlier versions we use MD5+SHA1 and practical chosen-prefix collisions 
on this combined hash function would break the integrity of TLS handshakes.
(Reporter)

Comment 4

4 years ago
Disregarding the general discussion above on collision resistance, the MD5 downgrade should be fixed soon.
Anything I need to do to get this bug confirmed?

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

4 years ago
Karthikeyan: thank you very much for reporting the bug. I just gave the
"canconfirm" access to your Bugzilla account, so that future bugs you
file will start in the NEW state.

Martin: I assigned the bug to you. I will be happy to help or review
the patches.
Assignee: nobody → martin.thomson
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.19.1
(Assignee)

Comment 6

4 years ago
I've identified what I think the most trivial fix for this bug could be:

Delete this line:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#4353

  /* tlsHashOIDMap contains the mapping between TLS hash identifiers and the
   * SECOidTag used internally by NSS. */
  static const struct {
      int tlsHash;
      SECOidTag oid;
  } tlsHashOIDMap[] = {
-     { tls_hash_md5, SEC_OID_MD5 },
      { tls_hash_sha1, SEC_OID_SHA1 },
      { tls_hash_sha224, SEC_OID_SHA224 },
      { tls_hash_sha256, SEC_OID_SHA256 },
      { tls_hash_sha384, SEC_OID_SHA384 },
      { tls_hash_sha512, SEC_OID_SHA512 }
  };

I think that works for now, but it has been suggested that perhaps JC might have more time for doing this work.

If we are going to fix this properly - which I'm going to recommend happen in a follow-up - a more complete solution would have a global hash configuration.  That config could be used in ssl3_ClientSendSigAlgsXtn and it would then also drive ssl3_CheckSignatureAndHashAlgorithmConsistency.  That would allow NSS users, like Firefox, to set a policy that disabled SHA-1 at the point in time where that was considered feasible.

We'd need good numbers to support a decision to do that though.  It might be that this is sooner than we phase out SHA-1 in certificates, but that seems like a reasonable point in time to consider a change in policy.

Kai also pointed out that it's possible to detect attempts at hash collisions as described in http://marc-stevens.nl/research/papers/PhD%20Thesis%20Marc%20Stevens%20-%20Attacks%20on%20Hash%20Functions%20and%20Applications.pdf
I don't think that's worth doing if we're going to be ripping SHA-1 out.  That might be a good backup plan if it turns out that telemetry tells us that is a bad idea.
Flags: needinfo?(jjones)

Comment 7

4 years ago
Martin: we probably should go for a direct fix by only allowing the
signature algorithms advertised in ClientHello. I agree your trivial
fix should solve this problem, but it is an indirect fix and is
likely to break in the future.
In the immediate term I'm pretty slammed, Martin - racking & configuring ISRG's DR datacenter Sunday-Wednesday. Earliest I can start taking a look would be 7 May. :(
Flags: needinfo?(jjones)
(Reporter)

Comment 9

4 years ago
Could we also fix the handling of signatureAlgorithms in CertificateRequest (and hence in CertificateVerify)? NSS currently ignores the requested signature algos and uses MD5SHA1 in TLS < 1.2
and SHA256 in TLS 1.2. This isn't standards compliant, but I guess not too many people are using client
signatures with other hash algorithms. It also means that a client that support SHA-512 signatures cannot
take advantage of the stronger hash algorithm. Again, clients using certs with hashAlg > SHA256 are probably rare.
(Assignee)

Comment 10

4 years ago
Regarding comment 9, I think that we should try to keep the changes contained.  We can consider expanding scope in a follow-on.

I've done some investigation here and I think that we should add a control surface for selecting what signature algorithms we want to accept.

The hack I described might be OK if we need something quickly.  It's indirect and horrible, as Wan-Teh noted, but it should work.


For the control surface, I've a few API choices that I'd like to run by folks with more familiarity with the code.

A.1. Co-dependent settings for signature and hash.  e.g., 
SECStatus SSL_SignaturePrefSet(PRFileDesc *fd, SSLSignType sigAlg, SSLHashType hashAlg, PRBool enabled);
Noting that SSLHashType doesn't exist (see C).

A.2. Indepedendent settings:
SECStatus SSL_SignatureAlgPrefSet(PRFileDesc *fd, SSLSignType sigAlg, PRBool enabled);
SECStatus SSL_SignatureHashPrefSet(PRFileDesc *fd, SSLHashType hashAlg, PRBool enabled);

A.3. A job lot approach:
SECStatus SSL_SetSignatureAlgorithms(PRFileDesc *fd, SSLSignatureAlgorithm *algorithms, size_t count);


B.1. Include setters for the global defaults, like SSL_CipherPrefSetDefault does for cipher suites.

B.2. Don't.


C.1. A new type in sslt.h for SSLHashType.

C.2. Assuming A1, define a struct for signature algorithms:

typedef struct SSLSignatureAlgorithmStr {
  SSLSignType sign;
  SSLHashType hash;
} SSLSignatureAlgorithm;

C.3. Use the PKCS#11 definitions, such as SEC_OID_MD5.

C.4. Move the tls_hash_* definitions in ssl3prot.h into sslt.h


I'm leaning toward A.1, B.2, and C.1.
Martin, we are trying to assign this a security rating, but don't understand the implications of this bug. Can you advise? How easy is it to exploit?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 12

4 years ago
I'd say that this is probably sec-high.  Collisions on MD5 are relatively cheap.  If second preimage resistance is not sufficient (I'm inclined to trust Karthik on this point, since I'm short on time and haven't read the report) then it is relatively easy to perform server impersonation based on this.
Flags: needinfo?(martin.thomson)
Thank you, Martin.
Keywords: sec-high
(Assignee)

Comment 14

4 years ago
Posted patch bug1158489-1.patch (obsolete) — Splinter Review
Groundwork for larger patch.
(Assignee)

Comment 15

4 years ago
Posted patch bug1158489-2.patch (obsolete) — Splinter Review
A rework of how we handle signature algorithms.
(Assignee)

Comment 16

4 years ago
http://codereview.appspot.com/232480043 is available on request.
(Assignee)

Updated

4 years ago
Attachment #8606510 - Flags: review?(wtc)
(Assignee)

Updated

4 years ago
Attachment #8606511 - Flags: review?(wtc)

Updated

4 years ago
Target Milestone: 3.19.1 → 3.19.2
Wan-Teh, can you do the patch review here? This has been sitting around for more than a month and we need to get this fixed.
(Assignee)

Updated

4 years ago
Target Milestone: 3.19.2 → 3.20
(Reporter)

Comment 18

4 years ago
(In reply to Martin Thomson [:mt:] from comment #12)
> I'd say that this is probably sec-high.  Collisions on MD5 are relatively
> cheap.  If second preimage resistance is not sufficient (I'm inclined to
> trust Karthik on this point, since I'm short on time and haven't read the
> report) then it is relatively easy to perform server impersonation based on
> this.

I think server impersonation, based on this bug, is not really practical yet.
It requires either
(a) a server which chooses a predictable server random and repeats its (EC)DH key share
    [10% of servers reuse key shares but very very few seem to use predictable server randoms,
     but the latter is difficult to test, and may be plausible in virtualized scenarios.]

or

(b) an adversary capable of performing 2^64 queries, storing their results, and then 
peforming 2^64 MD5 hashes 
    [the last of these is practical, but the first two seem to be out of reach for most adversaries]

or

(c) a second preimage attack on MD5
    [this is still an open problem]

So, although I am all for this bug being fixed as soon as possible, I would in this case recommend a lower security rating than sec-high, even if it means no bounty :)
(Assignee)

Comment 19

4 years ago
Thanks Kathik, hopefully we can fix this before this becomes sec-high based on any of those criteria :)
Keywords: sec-highsec-moderate
Al, can you provide your assessment on whether this bug needs to be fixed in the upcoming ESR 38 release? Thanks!
Flags: needinfo?(abillings)
(In reply to Ritu Kothari (:ritu) from comment #20)
> Al, can you provide your assessment on whether this bug needs to be fixed in
> the upcoming ESR 38 release? Thanks!

Given that it is a external report affecting TLS, I'd suggest taking this on ESR38.
Flags: needinfo?(abillings)
If this is going into 40, it pretty much needs to get checked in today.
Flags: needinfo?(martin.thomson)

Comment 23

4 years ago
Tracking for ESR 38 given comment 21.
(Assignee)

Comment 24

4 years ago
This is not going in today.  See comment 18 regarding severity.  Also, the patch needs another round, which it isn't getting on such short notice.
Flags: needinfo?(martin.thomson)

Comment 25

4 years ago
I noticed that you nominated this bug for esr38 tracking ?, does this mean that you don't want the security team to fix it? In that case we can wontfix and untrack. Let me know!
Flags: needinfo?(abillings)
No, it means that I'd mark it tracking 41+ in the ESR38 tracking except 41+ doesn't exist yet.
Flags: needinfo?(abillings)
(Assignee)

Comment 27

4 years ago
Release notes draft for this change:

Bug 1158489 - NSS libssl incorrectly accepted signatures in TLS 1.2 using algorithms that it did not advertise in its signature_algorithms extension.  This resulted in libssl accepting MD5 hashes in signatures.  There are several functional changes as a result of this fix:
  - Support for MD5 hashes in signature algorithms has been removed.
  - Support for SHA-224 hashes in signature algorithms has been removed (SHA-224 was previously only partially supported).
  - The set of signature algorithms that libssl advertises and accepts can be configured, where this was previously hard-coded.
  - libssl now selects a signature algorithm to use based on the configured signature algorithms, rather than having a fixed preference order.
  - New API calls have been added:
    + SSL_SignaturePrefSet sets the signature algorithms that libssl will advertise or accept.  The order of this set is used to select the signature algorithm that is used; earlier values are preferred.  This configuration governs what value is advertised in the signature_algorithms extension or the supported_signature_algorithms field of the CertificateRequest message.  libssl rejects signatures in TLS 1.2 that identify a signature and hash algorithm that don't appear in this set. 
    + SSL_SignaturePrefGet retrieves the currently configured signature algorithms.
    + SSL_SignatureMaxCount gets the maximum number of signature algorithms that can be configured at the same time.
(Assignee)

Comment 28

4 years ago
Posted patch bug1158489.patch (obsolete) — Splinter Review
Also at https://codereview.appspot.com/256230044/
Attachment #8606510 - Attachment is obsolete: true
Attachment #8606511 - Attachment is obsolete: true
Attachment #8606510 - Flags: review?(wtc)
Attachment #8606511 - Flags: review?(wtc)
(Assignee)

Updated

4 years ago
Target Milestone: 3.20 → 3.21
(Assignee)

Comment 29

4 years ago
Updated based on ekr's comments, still awaiting final review.
Attachment #8643316 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
I just opened Bug 1195434.  This provides justification/cover for these changes going in, which will happen soon.  It also avoids one concern that ekr raised regarding a mismatch between the configuration in NSS and in mozilla::pkix.  Other users of NSS will have to deal with this on their own, I'm afraid.

(clearing n-i on wtc)
Flags: needinfo?(wtc)
Martin, since this is a FF41+ tracked bug. Will there be a fix for this issue in Beta41 time frame (1-2 more weeks left now)?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 33

4 years ago
I will ask what the NSS release schedule is like.  I think that the plan is to get a few more features in before we branch.

Updated

4 years ago
Group: core-security → crypto-core-security
(Assignee)

Comment 34

4 years ago
Discussed on today's call.  The decision was to delay inclusion to Firefox 44 and to miss the next ESR 38 build.  There are a few more related changes to land in NSS.  None of the fixes are particularly urgent and we could use the extra time to do some more comprehensive testing.
Flags: needinfo?(martin.thomson)
Based on comment 34, setting FF41 status to wontfix.
(Reporter)

Comment 36

4 years ago
Can we push this bug along? 
This bug will be publicly disclosed at Real World Crypto (Jan 6-8 2016).
The disclosure page (still work-in-progress) is here:
http://prosecco.gforge.inria.fr/vulnerabilities/sloth.html
(user: vulnerability, pass: prosecco)

To summarize the impact on NSS, an attacker who can make 2^X connections to www.microsoft.com 
and store the server signatures, can subsequently impersonate www.microsoft.com to NSS clients 
by performing 2^(128-X) MD5 hashes. If X = 64 is practical, then the effective security of the connection is brought down to 64 bits, even if the server is using a 2048 bit RSA key with TLS 1.2 ECDHE-RSA-AES-GCM-SHA384. This attack is still out of reach for academics, but may well be in reach for a powerful government-scale adversary.
(Assignee)

Comment 37

4 years ago
Ahh, this is already landed. I am reminded that we should request uplift of NSS 3.21 though.
(In reply to Martin Thomson [:mt:] from comment #37)
> Ahh, this is already landed. I am reminded that we should request uplift of
> NSS 3.21 though.

We're about to cut release candidates for Firefox 43 so you may have blown the dates there to make it into that release.
Flags: needinfo?(lhenry)
Martin, it could happen, but we're about to merge beta to mozilla-release and build the release candidate on Monday. If this is critical for the 43 release, we also have the option to do an RC2 in mid week.   It looks like you were planning to release a fix along with 44 and that more testing is needed. I don't think we will have time for that testing over the next week unless this is critical. 

Since this is rated sec-moderate, I wouldn't normally consider doing a second RC build for it. Do you think the planned disclosure in January changes how we should treat this bug?
Flags: needinfo?(lhenry) → needinfo?(martin.thomson)
(Assignee)

Comment 40

4 years ago
As noted, it's sec-moderate, though Comment 36 does make this marginally more serious than the earlier analysis in Comment 18.  Either way, I don't think that we need to create a special build for it.

My recommendation is that we opportunistically uplift as things become available.  That is, take it with the next ESR that goes out, and if there is a second RC, consider including it there.  We've been using NSS 3.21 in Nightly for a while now and I've not heard even a whisper about stability or compatibility issues, so I think that it's safe to uplift.

The upcoming announcement could come with another media splash and another round of panicking from certain people.  If we want to get ahead of it, then it would be wise to at least have a plan in place.  I think that this is the plan: ship NSS 3.21 as the opportunity presents itself.

Note: when uplifting, we need to uplift the changes in bug 1211568, not this one.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(martin.thomson)
Resolution: --- → FIXED
Since the changes in bug 1211568 are already in 44 I think we should uplift them to beta as well. If you could request uplift there we could get it into Monday's RC build. That doesn't allow for a lot of testing or room for error but it sounds best to give it a try. Thanks Martin.
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 42

4 years ago
I requested uplift for the three relevant bugs.
Flags: needinfo?(martin.thomson)
If you consider to uplift to the ESR 38.x update scheduled for next week, we need to discuss the best strategy.

ESR currently uses NSS 3.19.2.1. There have been a lot of changes between that and 3.21. The root CA list has been updated twice, and in the past, it was usually decided that regular CA updates should be excluded from the ESR branch (except in case of emergency CA updates).
OK. After some more discussion (thanks everyone for your advice!) let's not uplift this to 43. We still may need to do that, but if so, more planning and a bit more time would be better.
Suggestion for Firefox 43 (or 43.1) and for ESR 38.5 (or 38.5.1):

Use Martin's minimal fix from comment 6, which Wan-Teh has reviewed in comment 7.


If this minimal fix can be considered sufficiently secure for the next 2-3 months, it seems more appropriate for a last minute hotfix.

Red Hat would like to make some more time testing NSS 3.21, and is looking for a minimal fix that can be backported to NSS 3.19.2.1


And, if the minimal fix can be considered sufficient for ESR 38, it might also be sufficient for the limited lifetime of the Firefox 43 branch.


If you agree to this approach, my suggestion is:

ESR 38
======
Currently uses NSS 3.19.2.1

Apply the minimal fix from comment 6 on a mini-branch and release NSS 3.19.2.2

Upgrade the ESR branch to 3.19.2.2


Firefox 43
==========
Currently uses NSS 3.20.1

Apply the minimal fix from comment 6 on a mini-branch and release NSS 3.20.2

Upgrade Firefox 43 to 3.20.2


I suggest that we go ahead and create the NSS releases immediately, and you can decide if you want to respin 43/38.5, or if you want to do a followup and do 43.1/38.5.1


Thoughts?
Flags: needinfo?(martin.thomson)
(In reply to Kai Engert (:kaie) from comment #45)
> 
> I suggest that we go ahead and create the NSS releases immediately, and you
> can decide if you want to respin 43/38.5, or if you want to do a followup
> and do 43.1/38.5.1


Are we allowed to check in the minimal fix publicly on stable branches, or would that make it all too obvious that we found a security vulnerability?

If you're worried that it's too obvious, the alternative suggested timing is:

- on January 4, commit minimal hotfix to NSS, 
                create NSS branch releases,
                and update the 38.x and 43.x branches

- publish the updated 38.x and 43.x releases as soon after January 5 as you can.
Could you please comment, which suggestion and which timing you like best?

Thanks

Updated

4 years ago
Attachment #8696762 - Attachment description: Martin's minimal fix, suggested for ESR 38.x and 43.x, has r=wtc → Martin's minimal fix, has r=wtc, suggested for ESR 38.x and 43.x
(Reporter)

Comment 49

4 years ago
Hi,

If there's going to be a CVE number for this bug, do let me know so I can include it in our paper.

Many thanks,
Karthik

Comment 50

4 years ago
Comment on attachment 8696762 [details] [diff] [review]
Martin's minimal fix, has r=wtc, suggested for ESR 38.x and 43.x

Review of attachment 8696762 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

I inspected the NSS source code in mozilla-esr38. I agree with Karthikeyan's
analysis that in all other places NSS doesn't sign with MD5 and doesn't allow
MD5 signatures -- partly because NSS properly disallows MD5, and partly because
NSS is hardcoded to use SHA-256 or SHA-1 in CertificateVerify.
Attachment #8696762 - Flags: review+
Alias: CVE-2015-7209
(In reply to Karthikeyan Bhargavan from comment #49)
> Hi,
> 
> If there's going to be a CVE number for this bug, do let me know so I can
> include it in our paper.
> 
> Many thanks,
> Karthik

Karthik, Red Hat already assigned CVE-2015-7575 to this flaw (i mailed you about this earlier)

Al, Can we use the above CVE please if possible to allow duplication.

Thanks!
(Reporter)

Comment 52

4 years ago
Hi, I wasn't sure whether the Red Hat CVE was for openssl or NSS or both.

I think there is going to be a protocol-level CVE ("Don't use MD5 signatures in TLS 1.2")
asked for by the OpenSSL team.

In addition, maybe should be implementation-level CVEs specifically for those libraries
that accept MD5 signatures even though they are disabled; i.e. not offered in ClientHello/CertificateRequest and therefore not expected to be sent or accepted.
For example, NSS, GnuTLS, BouncyCastle have/had this bug.
(In reply to Karthikeyan Bhargavan from comment #52)
> Hi, I wasn't sure whether the Red Hat CVE was for openssl or NSS or both.
> 
> I think there is going to be a protocol-level CVE ("Don't use MD5 signatures
> in TLS 1.2")
> asked for by the OpenSSL team.
> 
Red Hat issued a "protocol-level" CVE

> In addition, maybe should be implementation-level CVEs specifically for
> those libraries
> that accept MD5 signatures even though they are disabled; i.e. not offered
> in ClientHello/CertificateRequest and therefore not expected to be sent or
> accepted.
> For example, NSS, GnuTLS, BouncyCastle have/had this bug.

In several previous cases, no implementation level CVEs were issues. Code is implements the protocol. In essence code is not flawed, so i dont think we should issue code level CVEs
(Reporter)

Comment 54

4 years ago
> Red Hat issued a "protocol-level" CVE

Ok, we will cite this CVE in our disclosure (hopefully other vendors will follow suit)

> In several previous cases, no implementation level CVEs were issues. Code is
> implements the protocol. In essence code is not flawed, so i dont think we
> should issue code level CVEs

I don't mind having only a protocol-level CVE; it simplifies things for disclosure.

Worth adding a word of warning though.  Say you're using Firefox/NSS and look at the SSL Labs Browser test, which says that your browser does not support RSA/MD5. Or you're using an old version of GnuTLS and you set your --priority to "NORMAL" which does not include MD5. You may expect that, as a user, you have now addressed the protocol-level CVE-2015-7575 by "disabling MD5". The bugs in these libraries (and in some others) is that disabling MD5 is not as simple as that. It may be worth adding specific wording to the protocol-level CVE to warn users and TLS implementations against this. 

Of course, we and others will probably have browser and server tests that will test for the vulnerability and provide the right feedback.
Alias: CVE-2015-7209 → CVE-2015-7575
By tomorrow, I must publicly commit the minimal fix to NSS backported branches, which might cause the issue to be more obvious, than the previous commit for this bug (which had used the text "Enable signature algorithm configuration").

I suggest we'll use the following text for the minimal fix:
  "Remove obsolete entry."

Regarding release notes, I suggest that we'll be vague in the NSS release notes document for now, and update them around after Karthikeyan has published the issue, with more details and credit.

I'd announce the updated NSS releases with vague information at the time Mozilla releases the first of {43.x,38.x} that contains the fix.
Clearing needinfo from comment 45, I talked to Liz and Sylvestre on IRC.

A Firefox 43.x release shall happen this week, and because of that Liz confirmed I should go ahead and prepare the NSS release for it.

ESR hasn't been decided yet, waiting for a decision, but I'll create an appropriate release for it, too.
Flags: needinfo?(martin.thomson)

Comment 57

4 years ago
Confirmed with Chrome folks that we're treating this as not requiring a chemspill; we'll likely pick this up with Chrome 49 going to NSS 3.21, which means sometime in late Feb/early March to hit stable, based on reading the release calendar tealeaves.
The only difference between the suggested NSS 3.20.2 and the 3.20.1 currently being used will be attachment 8696762 [details] [diff] [review] plus NSS version number changes.
Attachment #8698647 - Flags: approval-mozilla-release?
The only difference between the suggested NSS 3.19.2.2 and the 3.19.2.1 currently being used will be attachment 8696762 [details] [diff] [review] plus NSS version number changes.
Attachment #8698648 - Flags: approval-mozilla-esr38?
Group: crypto-core-security → core-security-release
OK, after some more discussion with Daniel and Kai, we are planning for Kai to land this on m-r tomorrow (which should be after I build 43.0.1).  We will aim this NSS update for 43.0.2, which we are planning to build and release early next week.   The goal here is to avoid the need for a 43.0.3 in early January.
Kai, given that we are doing this for ESR38 and FF43, should we also uplift to Beta44? Thanks!
Flags: needinfo?(kaie)
Actually I just realized that FF44 was upgraded to NSS 3.21 in Bug 1211568 so we may not need to do anything here for FF44. Should I just mark it as fixed?
correct, 44 and 45 are already fixed, because they use NSS 3.21
Comment on attachment 8698647 [details]
upgrade mozilla-release 43 to NSS 3.20.2

Approved for uplift to mozilla-release. We are set with the 43.0.1 build now, so this work will go into 43.0.2.
Attachment #8698647 - Flags: approval-mozilla-release? → approval-mozilla-release+
Comment on attachment 8696762 [details] [diff] [review]
Martin's minimal fix, has r=wtc, suggested for ESR 38.x and 43.x

BTW, the minimal patch has landed on stable NSS branches:

NSS_3_19_2_X_BRANCH for 3.19.2.2
https://hg.mozilla.org/projects/nss/rev/94e1157f3fbb

NSS_3_20_BRANCH for 3.20.2
https://hg.mozilla.org/projects/nss/rev/891676aa0d85

The commit to mozilla-release upgraded to 3.20.2
We are ready to push this with 43.0.2 release today.  Al I just realized, does this mean we should have a security advisory in place? Or is there one already as this upgrade is already released in 44 and onward?
Flags: needinfo?(abillings)
I'll just link to the 43 sec advisory page for now from the release notes.
Flags: needinfo?(abillings)
****

Was this bug / fix under embargo?



Al - can you post the text of what we released today here?
We had an advisory live for about an hour today on this bug before I was advised that it might be under embargo. I pulled the advisory down. Text of advisory was:

Security researcher Karthikeyan Bhargavan reported an issue
in Network Security Services (NSS) where MD5 signatures in the server signature within the TLS 1.2 ServerKeyExchange message are still accepted. This is an issue since NSS has officially disallowed the accepting MD5 as a hash algorithm in signatures since 2011. This issues exposes NSS based clients such as Firefox to theoretical collision-based forgery attacks. This issue was fixed in NSS version 3.20.2.
I mailed karthikeyan.bhargavan@inria.fr to ensure that he sees this.
Flags: needinfo?(karthikeyan.bhargavan)
(Reporter)

Comment 72

3 years ago
What is the procedure for the advisory? Can you add one later?

Otherwise, the text above looks more or less ok, since it refers only to server authentication (which is the harder attack, compared to client authentication.) Any mention of the CVE would definitely be premature. The main risk is that (a) someone will start wondering what servers support MD5 signatures, and the thing might leak before RHEL 6 and 7 release patches, and (b) someone will start wondering about client authentication, before Java releases patches.
Flags: needinfo?(karthikeyan.bhargavan)
Usually, we write them when we release the fix but we can hold them until later as well.
The advisory is already public, should we consider this issue public now?
(Reporter)

Comment 75

3 years ago
Public disclosure of CVE-2015-7575 is in January, and other vendors need the time to fix.
Let's please be circumspect about what we consider public till then. Like I said, I do not
mind if the short text referred to above has already been leaked, but please do not add any
more detail or refer to the CVE at this point. Conversely, please do refer to the CVE
once the bug is publicly disclosed.
(In reply to Huzaifa Sidhpurwala from comment #74)
> The advisory is already public, should we consider this issue public now?

The advisory was removed. While the text may have leaked, it is down right now and there are no public references to the CVE at this time.

The text I posted above will go back online (with the CVE from here) once the talk has occurred. If you don't want it live then, Karthikeyan, we will need a disclosure date from you. Otherwise, I expect to post it on January 7, based on the presentation being complete. It won't contain any more data than what we posted before then.

In the future, we need to have embargo dates explicitly communicated. I should have seen the presentation dates and put it together myself not to talk about it but no one ever clearly communicated to Mozilla not to talk about it until after a specific date.
Karthikeyan, at what time and timezone will your talk be given? Thanks.
Flags: needinfo?(karthikeyan.bhargavan)
I just found the answer myself.
Thursday, Jan 7, 16:30 pacific time, which means, early after midnight Jan 8 UTC.
Flags: needinfo?(karthikeyan.bhargavan)
(Reporter)

Comment 79

3 years ago
Hi, we just put sloth-attack.org online.
I'll be giving the talk tomorrow, but let's consider this public disclosure.
So the advisory with the text given in comment 70 (which has the CVE attached to this bug on it) can go live now?
Flags: needinfo?(karthikeyan.bhargavan)
(Reporter)

Comment 81

3 years ago
Yes. Please cite the CVE number as well.

Thanks!
Flags: needinfo?(karthikeyan.bhargavan)
The sec advisory says that we fixed this in 38.5.2 but I don't think that is the case.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #83)
> The sec advisory says that we fixed this in 38.5.2 but I don't think that is
> the case.

Al: Liz is right. Could you please fix the advisory from comment 82 and remove the statement about 38.x?
Flags: needinfo?(abillings)
Comment on attachment 8698648 [details]
upgrade mozilla-esr38 to NSS 3.19.2.2

Given that we've announced this fix in ESR (comment 83) and that the sec team thinks we should take this fix (comment 21, comment 40), let's get this into ESR 38.6.0.
Attachment #8698648 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Flags: needinfo?(abillings)
Whiteboard: [adv-main43+]
Whiteboard: [adv-main43+] → [adv-main43+][adv-esr38.6+]
(In reply to Kai Engert (:kaie) from comment #84)
> 
> Al: Liz is right. Could you please fix the advisory from comment 82 and
> remove the statement about 38.x?

What the hell? We didn't ship this?
(In reply to Al Billings [:abillings] from comment #87)
> (In reply to Kai Engert (:kaie) from comment #84)
> > Al: Liz is right. Could you please fix the advisory from comment 82 and
> > remove the statement about 38.x?
> 
> What the hell? We didn't ship this?

We have _now_: comment 84 was 6 weeks ago; 4 weeks ago we shipped Firefox 38.6 ("44+") with NSS 3.19.2.2 containing the fix.
Comment on attachment 8646029 [details] [diff] [review]
bug1158489.patch

Martin, only out of curiousity, was there a special reason why you added sha384/512 entries for rsa and ecdsa, but not for dsa, in this table? Or was it simply the thought they aren't necessary currently? Thanks in advance.

+static const SSLSignatureAndHashAlg defaultSignatureAlgorithms[] = {
+    {ssl_hash_sha256, ssl_sign_rsa},
+    {ssl_hash_sha384, ssl_sign_rsa},
+    {ssl_hash_sha512, ssl_sign_rsa},
+    {ssl_hash_sha1, ssl_sign_rsa},
+#ifndef NSS_DISABLE_ECC
+    {ssl_hash_sha256, ssl_sign_ecdsa},
+    {ssl_hash_sha384, ssl_sign_ecdsa},
+    {ssl_hash_sha512, ssl_sign_ecdsa},
+    {ssl_hash_sha1, ssl_sign_ecdsa},
+#endif
+    {ssl_hash_sha256, ssl_sign_dsa},
+    {ssl_hash_sha1, ssl_sign_dsa}
+};

Updated

3 years ago
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 90

3 years ago
Kai, I think that the SHA-256 version is an error.  See this from RFC 5246, Section 7.4.3:

   Because DSA signatures do not contain any secure indication of hash
   algorithm, there is a risk of hash substitution if multiple hashes
   may be used with any key.  Currently, DSA [DSS] may only be used with
   SHA-1.  Future revisions of DSS [DSS-3] are expected [...]
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 91

3 years ago
Oh, I realize now why it's in there.  Otherwise, we can't do DSA with TLS 1.2 because we advertise only SHA-256 paired with the asymmetric signature scheme in signature_algorithms.  It's a bit of a mess really.
Group: core-security-release

Comment 92

2 years ago
Comment on attachment 8696762 [details] [diff] [review]
Martin's minimal fix, has r=wtc, suggested for ESR 38.x and 43.x

goog
Flags: needinfo?(MAWAVANSMOOKEY)

Comment 93

2 years ago
Comment on attachment 8696762 [details] [diff] [review]
Martin's minimal fix, has r=wtc, suggested for ESR 38.x and 43.x

Review of attachment 8696762 [details] [diff] [review]:
-----------------------------------------------------------------

http:mtnuganda-cust.opera-mini.net:80/bugzilla/<html >/good

Comment 94

2 years ago
Comment on attachment 8696762 [details] [diff] [review]
Martin's minimal fix, has r=wtc, suggested for ESR 38.x and 43.x

good
Flags: needinfo?(MAWAVANSMOOKEY)
You need to log in before you can comment on or make changes to this bug.