758314 - Allow end-user to override error when MD5 cert is encountered

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kathleen Wilson]

Add the user interface to allow the end-user to click through (over-ride) the error when an MD5 certificate is encountered. This cert override mechanism means "allow this one specific certificate to work for this one specific website, until the certificate expires."

We would like to add this capability so that end-users will not be tempted to set security.enable_md5_signatures to true, because that would mean effectively "enable MD5 signatures for every website forever."

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Treat SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED like SEC_ERROR_UNTRUSTED_ISSUER

I developed this patch by searching for every place in Gecko that mentioned SEC_ERROR_UNTRUSTED_ISSUER, and then adding equivalent logic for SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED.

We a week away from having the automated testing infrastructure set up to be able to test this with an automated test, so we'll have to use a litmus test for now.

I verified this using the steps that Kai outlined in bug 590364 comment 7:

Test case:

- set security.enable_md5_signatures = false

- use a separate test profile, please

- install Kai's test CA from http://kuix.de/ca/nss-test-ca.php
  (check the boxes, OK)

- go to https://kuix.de:9450

- you should see an error page

- you should be able to add an override and connect to the page

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 629063 [details] [diff] [review]
Treat SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED like SEC_ERROR_UNTRUSTED_ISSUER

r- but you should be close.



If you're modifying the nsIX509Cert interface, you should use a new UUID:

-[scriptable, uuid(f0980f60-ee3d-11d4-998b-00b0d02354a0)]
+[scriptable, uuid(c5501cf9-36f3-4c11-a14f-1ff2bef427f6)]


If you introduce new constant nsIX509Cert::SIGNATURE_ALGORITHM_DISABLED, then you should also introduce appropriate handling in all places, where other existing constants are being handled.

You already searched for SEC_ERROR_UNTRUSTED_ISSUER, but please also search for ISSUER_NOT_TRUSTED.

One place I found is nsNSSCertificate::VerifyForUsage, add:

+      case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
+        *verificationResult = SIGNATURE_ALGORITHM_DISABLED;
+        break;
+
       case SEC_ERROR_CERT_USAGES_INVALID:


(There's one more place, but we might skip function nsCertTree::GetCellText, as we no longer use a "purposecol" in cert manager UI (it turned out to be unacceptably slow when we started to enabled OCSP). We could consider to file a bug and get the code for purposecol removed.)

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Treat SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED like SEC_ERROR_UNTRUSTED_ISSUER [v2]

Kai, I made the changes you requested, except for changing the UUID of the interface. Since we are only adding a constant, and that doesn't affect the ABI of the interface, it is better to avoid changing the UUID so that binary extensions do not have to be recompiled just for this change.

BUT, note that the certificate details dialog box WILL NOT show this custom error message for certificates with MD5-based signatures, because CERT_VerifyCertificate does NOT return the SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED error for them when we call it from nsUsageArrayHelper::GetUsagesArray. Instead, CERT_VerifyCertificate returns SEC_ERROR_INADEQUATE_CERT_TYPE. I believe this is because we're asking it to verify the cert for every possible usage, and CERT_VerifyCertificate first detects SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED, and then later detects SEC_ERROR_INADEQUATE_CERT_TYPE, and decides to return the latter error code instead of the former.

Consequently, the error message in the certificate details dialog box will not be very helpful; it will say "Could not verify certificate for unknown reasons."

Also, when you have libpkix enabled, you also get the "Could not verify certificate for unknown reasons" because of a known bug, bug 672811.

Comment 4

[Kai Engert [:KaiE:]]

Thanks for the additional changes. We should file a new bug for the issue you identified (if there isn't one yet).

One last thing with your patch, you are changing the wording of string ID
  addExceptionUnverifiedLong

In the past I was told, "whenever you change the wording of a string, drop the old string ID, introduce a new string ID, and change all usages of the old string ID to use the new string ID".

So, I'd like to ask, please either drop your change to the wording of addExceptionUnverifiedLong, or use a new string ID and change everything accordingly.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Treat SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED like SEC_ERROR_UNTRUSTED_ISSUER [v3]

I made the change you suggested, and I filed bug 765133 about the certificate error dialog box.

I also changed the id of the addExceptionUnverifiedShort string even though I did not change its text, in order to keep its name consistent with addExceptionUnverifiedLong.

I also corrected the "an signature algorithm" typo in nsserrors.properties.

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 633394 [details] [diff] [review]
Treat SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED like SEC_ERROR_UNTRUSTED_ISSUER [v3]

Thanks, r=kaie

Comment 7

[Daniel Veditz [:dveditz]]

We're running short on time to get this into Firefox 16, just a couple of weeks more and we'll miss it. Can we land this and bug 650355 now?

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> We're running short on time to get this into Firefox 16, just a couple of
> weeks more and we'll miss it. Can we land this and bug 650355 now?

I am going to write automated tests for these patches in the next couple of days. If I have trouble with that then we'll have to define a Litmus test for it.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It is important to have an automated test for this, but I cannot get that test done in time for the next merge. Instead, let's use Kai's testcase as a litmus test.

I filed bug 773477 to add automated tests.

I added qawanted because that's the best way I can think of to indicate to QA that I want them to create a litmus test. (QA: Please educate me as to the proper way of doing this.)

> Test case:
> 
> - create a separate test profile, because having Kai's test CA
>   certificate installed is not safe for general use.
>
> - set security.enable_md5_signatures = false
> 
> - install Kai's test CA from http://kuix.de/ca/nss-test-ca.php
>   (check the boxes, OK)
> 
> - go to https://kuix.de:9450
> 
> - you should see an error page
> 
> - you should be able to add an override and connect to the page

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/097fc01c52a4

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/097fc01c52a4