Closed Bug 683883 Opened 13 years ago Closed 13 years ago

Improve DigitNotarGate handling in PSM

Categories

(Core :: Security: PSM, defect)

6 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox6 --- .2-fixed
firefox7 --- fixed
firefox8 --- fixed
firefox9 --- fixed
status1.9.2 --- .22-fixed

People

(Reporter: KaiE, Assigned: KaiE)

Details

(Keywords: verified-beta, verified1.9.2, Whiteboard: [commit hint: comment 15],[qa+])

Attachments

(1 file, 5 obsolete files)

This is a follow-up bug to the PSM level changes done in bug 682927. See bug 682927 comment 102 (and surrounding comments) for information why this is needed.
Attached patch Patch v1 (obsolete) — Splinter Review
With Bug 683261, NSS checks for known DigiNotar certs, and will report them as untrusted. This patch attempts to do the following in addition, but note, this is done ONLY FOR SSL connections, not for other uses of certs. if NSS said "good", then call PSM_SSL_BlacklistDigiNotar else == NSS said "bad or knockout cert" then call PSM_SSL_DigiNotarErrorUpgradeToRevoked PSM_SSL_DigiNotarErrorUpgradeToRevoked -------------------------------------- We look at all certs of the chain. Two conditions must be true: - at least one cert in the chain must be issued by "CN=DigiNotar Root CA" - at least one of the certs in the chain must be issued >= 2011-07-01 If both conditions are true, then we upgrade the error to "revoked". PSM_SSL_BlacklistDigiNotar -------------------------- Remember this is only called after NSS decides the chain looks good - and was not knocked out. We look at all certs of the chain. If the bottom-most entry in the chain is one of the two whitelisted Dutch gov. roots, then we continue with the good state from NSS. Else, and at least one cert in the chain was issued by someone named like "CN=DigiNotar", then we'll change NSS's good decision into a "bad cert" decision. We'll say at least "untrusted". However, in this case we'll also call into PSM_SSL_DigiNotarErrorUpgradeToRevoked, and potentially upgrade the error to revoked. I believe this implements what was originally intended by bug 682927.
Assignee: nobody → kaie
Comment on attachment 557489 [details] [diff] [review] Patch v1 this needs some fixes
Attachment #557489 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Only comments have changed in this updated patch.
Comment on attachment 557490 [details] [diff] [review] Patch v2 >+// Call this if a cert has been reported by NSS as INVALID -> // Call this only if a cert has been reported by NSS as INVALID >+PRErrorCode >+PSM_SSL_DigiNotarErrorUpgradeToRevoked(CERTCertificate * serverCert, >+ CERTCertList * serverCertChain) >+{ >+ // If cert was issued by DigiNotar, >+ // and any involved cert was issued after 01-JULL-2011, Nit: JUL >+ if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) { Are we sure this is going to catch everything, including certs chaining up via cross-signing, which do not chain up to the DigiNotar Root CA? >+// Call this if a cert has been reported by NSS as VALID Again, add the "only". >+ // By request of the Dutch government Please change this comment; it's misleading people. I suggest simply removing it. Gerv
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to Gervase Markham [:gerv] from comment #4) > > >+// Call this if a cert has been reported by NSS as INVALID > > -> // Call this only if a cert has been reported by NSS as INVALID Actually, this function is called from two places. Now I think the following would be a clearer comment: // Call this if we have already decided that a cert should be treated as INVALID, // in order to check if we to upgrade the error to REVOKED. > >+ // and any involved cert was issued after 01-JULL-2011, > > Nit: JUL ok fixed. > >+ if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) { > > Are we sure this is going to catch everything, including certs chaining up > via cross-signing, which do not chain up to the DigiNotar Root CA? No, this doesn't catch everything. This is my attempt to do the same as we had initially intended. I'd be OK to extend this to any CA certs from DigiNotar, including the cross-signed intermediates. If we want to do so (still excluding Dutch Gov.), we can shorten the comparison string to "CN=DigiNotar" Because of your question, I presume you're in favor of this, so I'll make this change. > >+// Call this if a cert has been reported by NSS as VALID > > Again, add the "only". Ok, added. > >+ // By request of the Dutch government > > Please change this comment; it's misleading people. I suggest simply > removing it. removed
Attachment #557490 - Attachment is obsolete: true
Attachment #557506 - Flags: review?(bsmith)
> >+ if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) { > Are we sure this is going to catch everything, including certs chaining up via > cross-signing, which do not chain up to the DigiNotar Root CA? The stated goal of the original patch was to do this test for cert only under the DigiNotar Root CA. This code should work even in the presence of cross-signed certs for the Root CA. It would not elevate certs issued after July 1 in the other DigiNotar intermediates. I'm OK with either, as long as we know what we are trying to elevate.
Comment on attachment 557506 [details] [diff] [review] patch v3 r+ rrelyea.. Two optiona changes: + if (CERT_GetCertTimes(serverCert, &notBefore, &notAfter) != SECSuccess || + notBefore >= cutoff) { + onOrAfter20110701 = PR_TRUE; + } serverCert is passed in, and isn't changed by the loop, so you can simply check it outside the loop. If it's not before the cutoff, you can simply return 0 without looking at the loop. This means you can return SEC_ERROR_REVOKED_CERTIFICATE inside the loop, since you can only get to the loop if the cert has been issued after Jul 1. My original description was assuming you only had the chain and you didn't know the chain order. + CERT_LIST_END(CERT_LIST_NEXT(node), serverCertChain)) { This appears to be verifying that one of the Dutch CA's is the root cert that issued this chain. The CERT_LIST_END() check is correct only if serverCertChain is ordered leaf to root. I believe that is the case, but a less brittle check would be to see if the cert is selfsigned SECITEM_ItemsAreEqual(&node->cert->derIssuer,&node->cert->derSubject) NOTE: This patch implements the semantic that any certificate in the DigiNotar infrastructure issued after Jul 1 would have the elevated error code. That's a mozilla policy decision whether that is the correct semantic. bob
Attachment #557506 - Flags: review+
OK, I'll attach an updated patch in a couple of minutes.
Attached patch Patch v4 (obsolete) — Splinter Review
This patch addresses Bob's proposed changes.
Attachment #557592 - Flags: review?(rrelyea)
Comment on attachment 557592 [details] [diff] [review] Patch v4 r+ rrelyea
Attachment #557592 - Flags: review?(rrelyea) → review+
Attachment #557506 - Attachment is obsolete: true
Attachment #557506 - Flags: review?(bsmith)
Comment on attachment 557592 [details] [diff] [review] Patch v4 Asking for additional feedback, as suggested during NSS conf call.
Attachment #557592 - Flags: review?(wtc)
Attachment #557592 - Flags: feedback?(dveditz)
(In reply to Robert Relyea from comment #7) > > NOTE: This patch implements the semantic that any certificate in the > DigiNotar infrastructure issued after Jul 1 would have the elevated error > code. That's a mozilla policy decision whether that is the correct semantic. Yes, any certificate in the DigiNotar infrastructure, except the ones that the Dutch Government requested to exclude. The code will change all such certificates, issued July 1 or afterwards, to be reported as "revoked", not overridable by the user. Our earlier patch had already done this for everything chaining up to the "DigiNotar Root CA". We know that Verizon and Entrust have revoked the respective intermediates. I think we want to treat certs from any potentially existing, but not yet reported intermediates, in the very same way. (with the exception of Staat der Nederlanden). I believe Mozilla wants this behaviour. That's how I understand Gerv's question in comment 4.
(In reply to Kai Engert (:kaie) from comment #12) > I think we want to treat certs from any potentially existing, but not yet > reported intermediates, in the very same way. (with the exception of Staat > der Nederlanden). I think a good idea to hold this patch until a final decision by the Dutch government or to review the statement about to keep the DigiNotar intermediates under the root of Staat der Nederlanden. The Dutch government recently advised in preparation for the possible outcomes of the Fox-IT report to "Identify what the consequences are if PKIoverheid DigiNotar certificates can not be trusted." It looks like they tried (with success) to keep the DigiNotar PKI Overheid root online as long as possible. This e-mail confirms that they are definitely not sure that the intermediate root is not compromised. http://dewinter.com/2011/09/01/logius-adviseert-om-gevolgen-uitval-pki-overheidscertificaten-te-benoemen/
Paul, please don't misunderstand this patch. This patch is a code fix. It introduces better/stricter handling for everything except the Dutch Government CAs. The handling for the Dutch Gov certs was added elsewhere. As you can see, this patch simply moves that code around, for code correctness purposes. I don't see a need to hold this patch, because it doesn't change the current handling of the Dutch Government CA certificates.
Note to people who will land the patch in this bug. You may have to revert the patch from bug 683449, to make the patch in this bug apply cleanly.
Whiteboard: [commit hint: comment 15]
Comment on attachment 557592 [details] [diff] [review] Patch v4 Just to be prepared, if Mozilla should decide to stop excluding the PKIoverheid CAs from the blacklist, I would land a modified patch, that excludes the following block of code. >+ // If it's issued by one of the "Staat der Nederlanden Root"s, >+ // then don't blacklist. Compare names, and ensure it's a self-signed root. >+ if ((!strcmp(node->cert->issuerName, >+ "CN=Staat der Nederlanden Root CA,O=Staat der Nederlanden,C=NL") || >+ !strcmp(node->cert->issuerName, >+ "CN=Staat der Nederlanden Root CA - G2,O=Staat der Nederlanden,C=NL")) && >+ SECITEM_ItemsAreEqual(&node->cert->derIssuer,&node->cert->derSubject) >+ ) { >+ // keep as valid >+ return 0; >+ } Bob, if needed, would you be OK with this modification?
> Bob, if needed, would you be OK with this modification? Yes, sounds good. This will definately stop whitelisting the Overheid certificates. bob
Comment on attachment 557592 [details] [diff] [review] Patch v4 r=wtc. I suggest some changes. >+// Call this if we have already decided that a cert should be treated as INVALID, >+// in order to check if we to upgrade the error to REVOKED. Nit: I find "upgrade the error to REVOKED" a little hard to understand. "Upgrade" usually means upgrading to something better, such as from economy class to business class. But REVOKED is the worse certificate error. >+ // If cert was issued by DigiNotar, >+ // and any involved cert was issued after 01-JUL-2011, IMPORTANT: This comment does not match your code. To match your code, the comment should say: // If any involved cert was issued by DigiNotar, // and serverCert was issued after 01-JUL-2011, >+ if (status != PR_SUCCESS) { >+ NS_ASSERTION(status == PR_SUCCESS, "PR_ParseTimeString failed"); >+ // be safe, assume it's afterwards, keep going >+ } >+ else { Nit: put '}' and 'else {' on the same line? >+ // no upgrade for certs older than the cutoff date Nit: change "older than" to "issued before". >+ if (!node->cert->issuerName) >+ continue; >+ if (strstr(node->cert->issuerName, "CN=DigiNotar")) { >+ return SEC_ERROR_REVOKED_CERTIFICATE; Nit: you can combine these two if statements into one: if (node->cert->issuerName && strstr(node->cert->issuerName, "CN=DigiNotar")) { return SEC_ERROR_REVOKED_CERTIFICATE; } >+ // If it's issued by one of the "Staat der Nederlanden Root"s, >+ // then don't blacklist. Compare names, and ensure it's a self-signed root. >+ if ((!strcmp(node->cert->issuerName, >+ "CN=Staat der Nederlanden Root CA,O=Staat der Nederlanden,C=NL") || >+ !strcmp(node->cert->issuerName, >+ "CN=Staat der Nederlanden Root CA - G2,O=Staat der Nederlanden,C=NL")) && >+ SECITEM_ItemsAreEqual(&node->cert->derIssuer,&node->cert->derSubject) >+ ) { >+ // keep as valid >+ return 0; >+ } The comment is slightly inaccurate because node->cert is one of the roots, but the comment is talking about the issuer. I would change the comment to say: // If it's one of the "Staat der Nederlanden Root"s, // then don't blacklist. Compare names, and ensure it's a self-signed root. i.e., remove "issued by". >+ certList = CERT_GetCertChainFromCert(serverCert, PR_Now(), certUsageSSLCA); >+ if (!certList) { >+ rv = SECFailure; >+ } >+ else { Nit: put '}' and 'else {' on the same line? >+ PRErrorCode blacklistErrorCode; >+ if (rv == SECSuccess) { // PSM_SSL_PKIX_AuthCertificate said "valid cert" >+ blacklistErrorCode = PSM_SSL_BlacklistDigiNotar(serverCert, certList); >+ } else { // PSM_SSL_PKIX_AuthCertificate said "invalid cert" >+ blacklistErrorCode = PSM_SSL_DigiNotarErrorUpgradeToRevoked(serverCert, certList); >+ } IMPORTANT: in the 'else' case, you may need to save the original error code, so that you can restore it if blacklistErrorCode is 0. The reason is that some of the NSS functions called by PSM_SSL_DigiNotarErrorUpgradeToRevoked internally may change the error code. You may be able to inspect PSM_SSL_DigiNotarErrorUpgradeToRevoked to prove that it won't change the error code if it returns 0, but it may be more robust to save the original error code.
Attachment #557592 - Flags: review?(wtc) → review+
(In reply to Wan-Teh Chang from comment #18) > > IMPORTANT: in the 'else' case, you may need to save the original > error code, so that you can restore it if blacklistErrorCode is 0. > The reason is that some of the NSS functions called by > PSM_SSL_DigiNotarErrorUpgradeToRevoked internally may change > the error code. Thanks for catching this. I indeed just ran into this bug. I will address your comments and attach an improved patch soon.
Attached patch Patch v5 (obsolete) — Splinter Review
(In reply to Wan-Teh Chang from comment #18) > > Nit: I find "upgrade the error to REVOKED" a little hard to understand. > "Upgrade" usually means upgrading to something better, such as from > economy class to business class. But REVOKED is the worse certificate > error. Ok, I'm changing comments and variables to use "worsen" instead of upgrade. > >+ } else { // PSM_SSL_PKIX_AuthCertificate said "invalid cert" > >+ blacklistErrorCode = PSM_SSL_DigiNotarErrorUpgradeToRevoked(serverCert, certList); > >+ } > > IMPORTANT: in the 'else' case, you may need to save the original > error code, so that you can restore it if blacklistErrorCode is 0. I've changed it to PRErrorCode revokedCode = PSM_SSL_DigiNotarTreatAsRevoked(serverCert, certList); if (revokedCode != 0) blacklistErrorCode = revokedCode; This patch addresses all the changes that Wan-Teh requested, and I fixed the single bug (I have a test case for this), therefore I'm carrying forward the r+.
Attachment #557592 - Attachment is obsolete: true
Attachment #557592 - Flags: feedback?(dveditz)
Attachment #557831 - Flags: review+
Comment on attachment 557831 [details] [diff] [review] Patch v5 No, although my test case worked, I din't fix the bug correctly.
Attachment #557831 - Flags: review+ → review-
Attached patch Patch v6Splinter Review
Ok, most of this patch has r=rrelyea and r=wtc, except for the following piece: + PRErrorCode blacklistErrorCode; + if (rv == SECSuccess) { // PSM_SSL_PKIX_AuthCertificate said "valid cert" + blacklistErrorCode = PSM_SSL_BlacklistDigiNotar(serverCert, certList); + } else { // PSM_SSL_PKIX_AuthCertificate said "invalid cert" + PRErrorCode savedErrorCode = PORT_GetError(); + // Check if we want to worsen the error code to "revoked". + blacklistErrorCode = PSM_SSL_DigiNotarTreatAsRevoked(serverCert, certList); + if (blacklistErrorCode == 0) { + // we don't worsen the code, let's keep the original error code from NSS + PORT_SetError(savedErrorCode); + } + } Wan-Teh, could you please review this section, to make sure I fixed it in the way you prefer? Thanks!
Attachment #557831 - Attachment is obsolete: true
Attachment #557837 - Flags: review?(wtc)
We'll wanted this landed everywhere once it is reviewed and tested: * mozilla-central (default branch) * releases/mozilla-aurora (default branch) * releases/mozilla-beta (default branch) * releases/mozilla-release (default branch) * releases/mozilla-1.9.2 (default branch) I can take care of transplanting to whatever relbranches we need. Thanks!
Comment on attachment 557837 [details] [diff] [review] Patch v6 r+ rrelyea
Attachment #557837 - Flags: review+
Comment on attachment 557837 [details] [diff] [review] Patch v6 r=wtc. >+// Call this if we have already decided that a cert should be treated as INVALID, >+// in order to check if we to worsen the error to REVOKED. Nit: change "we to" to "we have to" or "we need to". Nit: "worsen" is correct but sounds awkward. I'll shut up now :-) > if (strstr(node->cert->issuerName, "CN=DigiNotar")) { > isDigiNotarIssuedCert = PR_TRUE; Nit: Add a break statement. You can break out of the 'for' loop as soon as you set isDigiNotarIssuedCert to true.
Attachment #557837 - Flags: review?(wtc) → review+
Target Milestone: --- → mozilla9
QA would like to verify this behavior prior to signing off, but it's not 100% clear what we should be using as testcases and what the final behavior should be. Any hints would be very appreciated.
See bug 683261 for verification steps. Gerv
Hey, thanks tons for the followup. Sounds like we picked the right strategy (much of our test matrix are the URLs you pointed out) but the confirmation makes me breathe easier.
QA verififed this against 3.6.22(build2), 6.0.2(build2, and 7.0b4(build2) as well as the latest nightly. We used URLs that should now not be trusted as well as trusted ones and marked our results in the following spreadsheet: https://docs.google.com/spreadsheet/ccc?key=0AnEJ45MnstYjdGVLc0hQV3M0RE1ENDlZOTJqX204ZXc&hl=en_US#gid=0
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
QA tracking for verification in Aurora.
Whiteboard: [commit hint: comment 15] → [commit hint: comment 15],[qa+]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: