1042889 - mozilla::pkix, cannot override sec_error_ca_cert_invalid with version 1 certificate, and other scenarios (with or without pkix)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[marnie]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Firefox updated to 31 and I went to our plesk login for our server. 


Actual results:

I got an error:
Secure Connection Failed

An error occurred during a connection to **iplink to plesk**. Issuer certificate is invalid. (Error code: sec_error_ca_cert_invalid)

    The page you are trying to view cannot be shown because the authenticity of the received data could not be verified.
    Please contact the website owners to inform them of this problem. Alternatively, use the command found in the help menu to report this broken site.


Expected results:

In the past there was an option to confirm a security exception with this type of link, now there is no option. I am assuming it is pertaining to the update and use of the new mozpkix usage but I don't know how to fix it. I am able to access the login from Chrome. I also updated firefox on our windows 7 machine and I was able to login even after the upgrade, however the windows 8.1 machines were unable to access it after the firefox update.

Comment 1

[marnie]

I tried a solution posted here: https://support.mozilla.org/en-US/questions/1012036#answer-607025 and it worked. I renamed the cert8.db to cert8.db.old and then restarted firefox and I am now able to get to the login page for our plesk install.

Comment 2

[Guillaume Beaudoin]

User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

I have the same problem as reported by [e-mail]. Renaming cert8.db to cert8.db.old doesn't not change anything.

Comment 3

[Vladimir]

Similar problem is discussed in Bug 403220
I think that mechanism of certificate validation in mozilla::pkix is too strong, but without notification of which part of certificate is invalid it is impossible to say why this happens.
It is not a simply "regression". It is CRITICAL regression.

For developers of mozilla::pkix: please give somewhere detailed report why there is sec_error_ca_cert_invalid somewhere in console (which field of certificate is "invalid") at least in nightly. 
I have few internal corporate web-sites with one internal CA certificate and generated certificate for every site (mail, databases...). For some of them i have no any problems, but some are not accepted by only firefox. The only reason i've found is that CA certificate is V1 without extended fields, and "untrusted" sites have V3 certificates with extended attribute describing additional domain names.

Comment 4

[Guillaume Beaudoin]

Could someone confirm they have the same issue as myself? Please try to connect to http://bit.ly/1s42vQi

Comment 7

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> [Tracking Requested - why for this release]:
> Some websites that use CAs that are not trusted by default are inaccessible
> in Firefox 31, whereas previously one could add a cert error override.
> 
> (In reply to Camilo Viecco (:cviecco) from comment #5)
> > The site [CENSORED] uses a v1 root certificate. Please
> > use a v3 certificate with the basic constraints bit isCA set to true. (you
> > can reuse your key)
> 
> What Camilo suggested is a good idea if it is feasible for you.
> 
> But, also it would be reasonable to allow the user to add an cert error
> override for non-trust-anchor self-signed v1 certificates. After all, if the
> user were to add the root certificate as a trust anchor, then we could
> accept the certificate without error. So, this is no different than any
> other SEC_ERROR_UNKNOWN_ISSUER as far as the security impact of allowing a
> cert error override is concerned.

Comment 5 and comment 6 were hidden at the request of the reporter, who doesn't want us to mention the URL of the website of the affected website.

Comment 8

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

More information from the reporter:

> Yes, it's Novell ZENworks. It's a software to manage workstations (à la
> Micorosoft SCCM System Center Configuration Manager).
> 
> Even more troublesome for us, the same certificate is used as the 
> Internal Certificate Authority » to authenticate the managed workstation
> to the ZENworks server.

It seems like these certificates are going to be common because they are generated (semi-)automatically using some commercial, closed-source product.

Comment 9

[Matt Hughes]

Having this issue as well.

We have a self-signed test CA in our labs.  We all import that test CA as a trusted authority in our browsers.  I was able to fix the problem by deleting the certs8.db.

What is odd is some sites work and some don't; they're all signed by the same test ca.  I can't really see what is different.
 
Here is the cert info:
Certificate:
    Data:
        Version: 1 (0x0)
        Serial Number:
            e1:88:6b:11:20:7f:6e:44
        Signature Algorithm: sha1WithRSAEncryption
        Issuer: --- OU=Test CA
        Validity
            Not Before: Dec 23 03:04:43 2011 GMT
            Not After : Dec 15 03:04:43 2041 GMT
        Subject: ----
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
            RSA Public Key: (2048 bit)
                Modulus (2048 bit):
                    00:e9:ff:45:da:b4:cb:99:c3:de:87:46:32:a9:06:
                    32:c4:b4:6f:52:b8:bc:71:69:3d:16:77:f5:54:37:
                    d6:a8:9a:f7:b4:06:ec:7d:7b:1c:36:c0:b5:60:42:
                    e5:12:3f:3b:fc:ac:e4:c4:ab:7f:4e:65:47:8d:75:
                    9c:02:c7:d8:77:a4:ca:48:05:4b:7c:74:e3:07:1e:
                    db:b8:6c:17:5f:3f:7e:d5:dd:c8:22:6c:d3:e0:f6:
                    25:44:93:7a:2c:0b:c2:86:72:ea:07:c3:13:6d:b2:
                    9a:4d:b3:6b:f9:bb:1a:3d:68:70:0a:ff:71:78:0b:
                    8b:f4:3b:44:c6:b7:2e:c4:a2:02:96:96:9c:48:86:
                    e4:5c:9d:4c:ba:b0:27:4e:ac:6f:88:43:af:a9:5d:
                    20:57:78:d7:d5:b2:df:9f:ed:41:a5:5d:52:d6:93:
                    9d:0b:bf:f5:8b:e6:fc:bf:5d:02:75:5e:1c:6b:d4:
                    87:ce:ee:aa:3d:c1:c2:24:f0:e0:66:c8:3e:67:af:
                    66:9d:7b:88:bb:cb:c8:ed:5b:47:d6:ae:56:a7:54:
                    3e:57:95:e3:32:ef:63:b8:6d:e0:10:06:ca:2b:9f:
                    78:38:c7:94:65:23:fa:b1:f4:d4:e6:b4:97:0c:c4:
                    33:a3:91:89:89:91:f0:48:3d:2c:fb:38:ce:02:75:
                    f2:8f
                Exponent: 65537 (0x10001)
    Signature Algorithm: sha1WithRSAEncryption
        42:04:97:68:68:b9:91:06:8b:9d:2b:76:0f:b0:ca:02:8c:7f:
        91:73:04:0c:4c:bb:3c:73:42:d7:7e:47:08:a3:dc:ce:ad:c4:
        0f:b1:d0:0a:19:5d:95:62:2d:59:b9:c4:b4:a4:3d:11:98:fc:
        ac:1a:04:63:09:6b:85:ff:85:fe:de:f9:16:66:fe:68:51:fe:
        5d:88:f0:e2:b9:69:97:59:8c:62:d5:7b:d4:70:e2:13:6f:cd:
        db:ad:da:cb:bd:43:64:22:11:58:cd:7c:df:f7:ee:51:6f:cc:
        6e:fd:53:9e:4b:6b:cf:c0:eb:d6:e3:78:1b:b0:48:ab:48:ac:
        c0:16:32:f9:da:6b:7f:10:72:92:db:93:28:61:6e:ec:77:cf:
        04:aa:2d:c4:9d:be:40:9f:78:56:99:cf:cb:6e:94:3b:05:e8:
        c3:54:71:27:b0:93:2e:15:14:69:0a:de:40:a6:95:4d:a4:25:
        35:3d:b2:e8:e4:13:b8:55:48:9a:a1:de:f5:ce:d7:5b:10:54:
        75:c8:31:09:e5:b8:f1:41:2a:14:f9:cc:db:b8:bd:66:f0:4c:
        39:99:a7:75:72:09:6a:c0:57:a1:12:77:8c:12:ff:43:54:c6:
        4c:79:37:f9:8f:6b:2c:5c:7a:dc:43:8a:a9:9e:e7:1a:99:1b:
        22:a2:6d:70

Looking above, it seems like another suggested workaround is to make this a v3 cert.  Yet that doesn't explain why some certs signed by this test ca work and others don't.  AND why, when I start with a fresh cert8.db, everything works normally.

Comment 10

[Matt Hughes]

Some difference about the actual certs (signed by same CA):

Works:
key usage: Encrypt, Verify, Wrap, Derive
Extended key usage: Critical: False


Doesn't work:
key usage: Encrypt, Verify, Derive
Extended key usage: Critical: True
key usage extension: Critical: true
  Digital Signature, Key Encipherment

Comment 11

[Matt Hughes]

Also, kind of a tangent, but it's very frustrating that FF presents an error, but doesn't let you actually look at the certificate.  It should have the same behavior as an untrusted certificate; you should at least be able to click on the pad lock in the url bar to view details.  How else are clients supposed to debug the issue?

Comment 12

[Sylvestre Ledru [:Sylvestre]]

We probably won't release a new 31 for this but we might take a patch for ESR 31.

Comment 13

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

http://www-01.ibm.com/support/docview.wss?uid=swg21680147&myns=swglotus&mynp=OCSSKTMJ&mync=R

Comment 14

[Marek Raida]

The same problem with self-signed certificates, and it is even increasing.
Started after upgrade to 31. On Windows 7, mostly.
Deleting them (internal certificates) helped for a week (we were able to confirm security exception), but then, week later, the very same problem appeared again, on the same computers; it affects whole company and people are beginning to distrust Firefox, what is really really sad :-((
Please, fix it somehow...

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking 32 as won't fix as it's now too late to accept a fix.

Brian - Can/should we fix this in 33? If so, can that be done this week before 33 uplifts to beta?

Comment 16

[Danilo Treffiletti]

Just reporting another website that is no more accessible with Firefox 31.0: https://www.wind.it/ (It's the website of an Italian mobile phone provider).

The certificate is correctly granted by GTE CyberTrust Solutions, Inc.

$ openssl s_client -connect www.wind.it:443 < /dev/null
CONNECTED(00000003)
depth=2 C = US, O = GTE Corporation, OU = "GTE CyberTrust Solutions, Inc.", CN = GTE CyberTrust Global Root
verify return:1
depth=1 O = Cybertrust Inc, CN = Cybertrust SureServer Standard Validation CA
verify return:1
depth=0 C = IT, ST = Italia, L = Milano, O = WIND SPA, OU = TN.IT.OP.SI., CN = www.wind.it
verify return:1
---
Certificate chain
 0 s:/C=IT/ST=Italia/L=Milano/O=WIND SPA/OU=TN.IT.OP.SI./CN=www.wind.it
   i:/O=Cybertrust Inc/CN=Cybertrust SureServer Standard Validation CA
 1 s:/O=Cybertrust Inc/CN=Cybertrust SureServer Standard Validation CA
   i:/C=US/O=GTE Corporation/OU=GTE CyberTrust Solutions, Inc./CN=GTE CyberTrust Global Root
 2 s:/C=US/O=GTE Corporation/OU=GTE CyberTrust Solutions, Inc./CN=GTE CyberTrust Global Root
   i:/C=US/O=GTE Corporation/OU=GTE CyberTrust Solutions, Inc./CN=GTE CyberTrust Global Root
---
Server certificate
-----BEGIN CERTIFICATE-----
MIIEKzCCAxOgAwIBAgIOAQAAAAABNlh4HSZd7FUwDQYJKoZIhvcNAQEFBQAwUDEX
MBUGA1UEChMOQ3liZXJ0cnVzdCBJbmMxNTAzBgNVBAMTLEN5YmVydHJ1c3QgU3Vy
ZVNlcnZlciBTdGFuZGFyZCBWYWxpZGF0aW9uIENBMB4XDTEyMDMyODA4NTEyN1oX
DTE1MDMyODA4NTEyN1owbzELMAkGA1UEBhMCSVQxDzANBgNVBAgTBkl0YWxpYTEP
MA0GA1UEBxMGTWlsYW5vMREwDwYDVQQKEwhXSU5EIFNQQTEVMBMGA1UECxMMVE4u
SVQuT1AuU0kuMRQwEgYDVQQDEwt3d3cud2luZC5pdDCCASIwDQYJKoZIhvcNAQEB
BQADggEPADCCAQoCggEBAJ0tDMcy74qFd5SoaGxux4N13Pq3cPBl/iZvF+JyaTft
PtWL2i7B2yRNDF2NBYaK2/rECIMB3Zwwhh4YEe6RxIScO8p7rRAjlEfBIC9ABlC7
XRYVIB6btn89SjC1DR2kTSiSmitOTfY3M1GEx4pNLM5wOuR29y5AwKHDVje/5DEA
NQut3OxBRskBjXvjNlafZfQDnlN0ee8Ag4WoI1owxYZNBiiADTfSYnIvhFkGW21X
fl3W3iJ98+c5WNTj4QCesENmrSNMHWKQ/pYsHcyAieiXhfyYieoZ0ashHKiqHwVE
KtOGq6NAO3bH2xyPEGBl2qC7Erv8d5SAWb6yi/gyydsCAwEAAaOB4zCB4DAfBgNV
HSMEGDAWgBTNOpafrm4PQFwcSPhLLbhxAeuJ2jA5BgNVHR8EMjAwMC6gLKAqhiho
dHRwOi8vY3JsLm9tbmlyb290LmNvbS9TdXJlU2VydmVyRzIuY3JsMB0GA1UdDgQW
BBTlhQ+N10kt1x+cp3Gl3YCYHh+WGDAJBgNVHRMEAjAAMA4GA1UdDwEB/wQEAwIF
oDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwEQYJYIZIAYb4QgEBBAQD
AgbAMBYGA1UdEQQPMA2CC3d3dy53aW5kLml0MA0GCSqGSIb3DQEBBQUAA4IBAQAy
ws4HEpACbpn5fTbLPOJqiyfUoijnlncG2myjygEhybPds9G0y752H6pRNWaNifmg
1M2+js3gesT4C+rpQWi43FDu0/ixZs4G9k+lLnOn32SEGqjzcwd/nS1FnnT8MoAQ
q332uQaayKQ8QNVV3jC+bV1rrW3iObhBsYlAX5VzOsTBqiYzxTcwSW5B7EHlwnqB
Z13+lXzQnati9MVITjYQVfrMN9YgrhRrC6dM8n/1CG6R0klKFeEXDDRUU3pRlJyt
3NPGt5LVy9X2zv5A28Yi3aGhRO2tObSj+vauHG49XdjDHjrz+d82ch8HUqcEG3pz                                                                                                                          
phiE3exGSCkDgPX5NvEU                                                                                                                                                                      
-----END CERTIFICATE-----                                                                                                                                                                 
subject=/C=IT/ST=Italia/L=Milano/O=WIND SPA/OU=TN.IT.OP.SI./CN=www.wind.it                                                                                                                
issuer=/O=Cybertrust Inc/CN=Cybertrust SureServer Standard Validation CA                                                                                                                  
---                                                                                                                                                                                       
No client certificate CA names sent                                                                                                                                                       
---                                                                                                                                                                                       
SSL handshake has read 3075 bytes and written 634 bytes                                                                                                                                   
---                                                                                                                                                                                       
New, TLSv1/SSLv3, Cipher is AES256-SHA                                                                                                                                                    
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
SSL-Session:
    Protocol  : TLSv1
    Cipher    : AES256-SHA
    Session-ID: C9F85D51F2A5A3365BA59A4584C2102BC8403F9F723C67812E8C4F5F059B3291
    Session-ID-ctx: 
    Master-Key: F1581ED4E3297E7DD8B46C0E3E65AEA2C9D43C05B20CA7B6661EFC20D3C7A3F99A8D038516E865843E8F0C83D2A71A5F
    Key-Arg   : None
    PSK identity: None
    PSK identity hint: None
    SRP username: None
    TLS session ticket:
    0000 - e9 aa 14 c8 b0 0e df aa-1d b0 9f 2b 32 db 5e 05   ...........+2.^.
    0010 - 6a 0d 87 7c a0 dc b2 b3-91 11 b3 63 cb 73 a6 38   j..|.......c.s.8
    0020 - 6c 48 bd d8 d8 0a e4 77-2f 8f c9 07 6b 25 87 54   lH.....w/...k%.T
    0030 - 5a 95 6d 14 3a 5c e7 06-93 7c b7 ac ba a1 62 20   Z.m.:\...|....b 
    0040 - e4 0a c8 c4 5c f9 62 50-6b da db 3f 26 43 7d f3   ....\.bPk..?&C}.
    0050 - 66 b2 82 d5 5f 26 1d 25-2c ae 90 ef 98 e3 51 d8   f..._&.%,.....Q.
    0060 - 57 50 ff 50 88 6a 06 78-0c 5a 16 a2 9c 0b 79 42   WP.P.j.x.Z....yB
    0070 - 55 a5 b0 69 0a c2 65 ff-93 97 a6 8e cc 8b 70 ea   U..i..e.......p.
    0080 - fa b9 ff f7 a0 f5 37 00-a3 22 d6 96 5d 07 9f 19   ......7.."..]...
    0090 - 25 65 60 29 d1 31 9c 66-1d 1e c3 5b 9f b5 aa 28   %e`).1.f...[...(

    Start Time: 1409077457
    Timeout   : 300 (sec)
    Verify return code: 0 (ok)
---
DONE

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks, urban. That's a different issue that we'll track in bug 1058812.

Comment 18

[Andrei Petcu]

Ubuntu 14.04, Firefox 31. Self signed internal company certificate. I tried renaming cert8.db but it does not work.

Comment 19

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

(In reply to Lawrence Mandel [:lmandel] from comment #15)
> Brian - Can/should we fix this in 33? If so, can that be done this week
> before 33 uplifts to beta?

Yes.

Comment 20

[Alexei Yuzhakov]

Guys, millions of Plesk users may stop using Firefox due to this problem. Plesk goes with default autogenerated SSL certificate. But majority of providers does not change it and continues to use the autogenerated certificates. End-users of these providers will be facing with this problem.

Comment 21

[Kai Engert (:KaiE:)]

Should we resolve this as a duplicate of bug 1063315 ?

Comment 22

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #21)
> Should we resolve this as a duplicate of bug 1063315 ?

Maybe not. In bug 1063315, I see that a md2 signature is involved in the root CA certificate.

I cannot see md2 involved in the certificates from comment 5.

Potentially the issue in this bug is related to the "version 1" certificate (instead of the expected "version 3").

I'm able to override this error with mozilla::pkix disabled, so it's caused by that.

Comment 23

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

(In reply to Kai Engert (:kaie) from comment #21)
> Should we resolve this as a duplicate of bug 1063315 ?

No. The certificate in bug 1063315 has many problems, as you noted.

Comment 24

[Kai Engert (:KaiE:)]

firefox 32 + 33: still cannot override

(This makes it different from bug 1063315, which apparently has been fixed in firefox 33.)

Comment 25

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

(In reply to Kai Engert (:kaie) from comment #24)
> firefox 32 + 33: still cannot override

What about Nightly?

> (This makes it different from bug 1063315, which apparently has been fixed
> in firefox 33.)

Again, most likely Firefox 33 works due to the side effect of removing some 1024-bit roots.

Comment 26

[Kai Engert (:KaiE:)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #25)
> (In reply to Kai Engert (:kaie) from comment #24)
> > firefox 32 + 33: still cannot override
> 
> What about Nightly?

Cannot override with nightly ff 35.

I confirmed that Firefox 24 allows to override this error.


> Again, most likely Firefox 33 works due to the side effect of removing some
> 1024-bit roots.

I don't think so.

Comment 27

[Kai Engert (:KaiE:)]

Bug 1063315 has a patch, when applied, Firefox allows to override this error.

Comment 28

[Kai Engert (:KaiE:)]

test build of ESR-31 branch with patch v1 included: 
  https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-934c922473bc/

Comment 29

[Raj Bhaskar]

(In reply to Kai Engert (:kaie) from comment #28)
> test build of ESR-31 branch with patch v1 included: 
>  
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 934c922473bc/

Using this build, I can see the 'I understand the risks' message again, but when I press the 'Add exception' button, the 'Confirm security exception' button (and 'Permanently store this exception' checkbox) are greyed out and I can't click them.

This is for an internal self-signed instance of Oracle Enterprise Manager Cloud Control.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Kai, any chance we could get a fix for 33? Thanks

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I maintain that it would be best to in general not allow certificate exceptions for certificates that appear to be issued from certificates lacking CA:TRUE in the basic constraints extension. This would be similar to allowing exceptions for invalid signatures. Instead, since mozilla::pkix does allow v1 certificates that are already trust anchors to essentially omit the basic constraints extension, anyone with this problem should import the root certificate of the site they're attempting to access using the certificate manager: Preferences -> Advanced -> Certificates -> View Certificates -> Authorities -> Import -> (import the root certificate) -> Check "Trust this CA to identify websites".

I'm resolving this a "INVALID", which really means "this works as intended".

Comment 33

[SuD]

Use case:
- I visit a not-very-trusted website i didn't knew before (https://suspicious-site.com).
- It has a bogus certificate, but I want to add an exception because it want to have a peek at the site. I will probably never come back anyway.

Before:
- I add an exception for certificate on that site only.

After:
- I have to install the certificate as a root CA.
- Now the suspicious site can impersonate ANY certificate on my browser.

Am I right this is a serious security problem?

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Updating the tracking flags to match comment #32.

Comment 35

[peter gervai]

I have several old units (embedded) with fixed https (no way to connect to http), self signed certs and no root cert offered. How should I "import" the root? How should I connect to these units?

Comment 36

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Peter, if there's no root certificate, that would be a different issue. Please file a new bug so we can figure out what's wrong and find a solution.

Comment 37

[Kai Engert (:KaiE:)]

FYI, there seem to be many people who are affected by the problem that they can no longer override the error, and are unable to connect to their sites.

For example, see the many comments on this site:
http://blog.dob.sk/2014/07/23/firefox-31-self-signed-certificate-sec_error_ca_cert_invalid/

David, I would like to suggest that you to reconsider your decision not to allow an override.

Comment 38

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I may have thought of a workaround that will be acceptable: In mozilla::pkix, continue to only treat an x509v1 certificate as a CA if it is a trust anchor. In NSSCertDBTrustDomain, when checking potential issuers, if an issuer is an x509v1 certificate that isn't a trust anchor, don't attempt to build a path with it. This will (in most cases) result in an "unknown issuer" error that is overridable by the user. This way, we don't have to allow overrides in all cases where a non-CA certificate is attempting to act as a CA.

Comment 39

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

David, I would suggest that when filtering errors for cert overrides in SSLServerCertVerification, for this error and maybe other errors, check if the end-entity certificate is self-signed and, if so, translate the error to Result::ERROR_UNKNOWN_ISSUER and calculate the override bits on that error.

Comment 40

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I don't think that will fix the issue. My understanding is that the end-entity certificate is not self-signed, and the issuer is an x509v1 certificate that is not a trust anchor and does not have a basic constraints extension. Consequently, mozilla::pkix sees this as a case where we can't safely say that the x509v1 certificate may issue other certificates, and returns Result::ERROR_CA_CERT_INVALID.

Comment 41

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

David, search this bug for "self-signed" and you will see that many or even most of the instances of this problem are for self-signed certificates, including the link that Kai provided. I think it may be acceptable to just allow the override for the self-signed case.

Comment 42

[Kai Engert (:KaiE:)]

I think you won't be able to fully enumerate all the way in which invalid certificates have been produced.

We're talking about the scenario, where you have successfully detected that things are bad, have warned the user, but the user needs a way to proceed anyway.

This happens when people must connect to devices with embedded certificates, where the creator of the certificate hadn't done their homework to create a correct certificate.

People are stuck with such hardware, without being able to replace them.

If you want to support users, allow them to override, no matter what caused the cert to be treated as bad.

I agree that we shouldn't allow to override known revoked certs, and we shouldn't allow to override known blacklisted certs. But for all other bad certs it should be possible to override.

Comment 43

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #41)
> David, search this bug for "self-signed" and you will see that many or even
> most of the instances of this problem are for self-signed certificates,
> including the link that Kai provided.

Comment 3, comment 5 (hidden), and comment 9 all describe having issues with non-self-signed end-entity certificates.

> I think it may be acceptable to just allow the override for the self-signed case.

This actually already works (at least in 35). I did some limited testing, and I was able to add an override for a self-signed x509v1 certificate with no basic constraints extension.

Comment 44

[me.moz]

(In reply to Kai Engert (:kaie) from comment #37)

> For example, see the many comments on this site:
> http://blog.dob.sk/2014/07/23/firefox-31-self-signed-certificate-
> sec_error_ca_cert_invalid/
> 

FYI: this post is getting 300-400 visits/day (since FF31 release), what is min. 5000 new uniques/month. So there seems to be many people affected by this change - and the question how many simply switched to other browser instead of looking for workarounds.

Comment 45

[me.moz]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #43)
> (In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment
> #41)
> > David, search this bug for "self-signed" and you will see that many or even
> > most of the instances of this problem are for self-signed certificates,
> > including the link that Kai provided.
> 
> Comment 3, comment 5 (hidden), and comment 9 all describe having issues with
> non-self-signed end-entity certificates.
> 
> > I think it may be acceptable to just allow the override for the self-signed case.
> 
> This actually already works (at least in 35). I did some limited testing,
> and I was able to add an override for a self-signed x509v1 certificate with
> no basic constraints extension.

My 'now' problematic certificates have been created by pkitool from easy-rsa - and IMHO many users are creating certificates also using this scripts.

Problem is that pkitool creates server certificate with CA:TRUE in extension section.

I think, there should be always an override allowed - you might only disallow storing permanent exception for really invalid certificates (ie. CA without CA:TRUE) - that all to make sites, that have been accessible before FF31, accessible again (and most of them are users own intranet sites).

Comment 46

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

(In reply to me.moz from comment #45)
> Problem is that pkitool creates server certificate with CA:TRUE in extension
> section.

Are they self-signed? If so, then the work that was already done for Firefox 33 should fix things.

If not, then pkitool is exposing your infrastructure to a huge security issue.

Either way, pkitool is open source and it should be very easy to write a patch to pkitool and contribute it upstream, to make pkitool generate end-entity certificates without basicConstrapints (equivalent to basicConstraints.cA == false).

> I think, there should be always an override allowed - you might only
> disallow storing permanent exception for really invalid certificates (ie. CA
> without CA:TRUE) - that all to make sites, that have been accessible before
> FF31, accessible again (and most of them are users own intranet sites).

It depends on what Firefox's goals are. If Firefox goal is to offer a reasonable UI that protects non-experts (i.e. almost everybody) people from MitM attacks, then it makes a lot of sense for Firefox to become stricter and stricter about disallowing cert error overrides.

Comment 47

[Kai Engert (:KaiE:)]

In case it helps, I have setup multiple test services, with various incorrect certificates.
If you know of another kind of bad certificate that we need to test, please tell me what properties it should have (or send me an example certificate), and I'll try to set it up.

https://kuix.de:9451/
https://kuix.de:9452/
https://kuix.de:9453/
https://kuix.de:9454/

I tried to setup the example given in comment 45, self-signed server cert with basic constraints extension set to true (is ca), but that seems to be overridable by firefox 31.

(In reply to me.moz from comment #45)
> Problem is that pkitool creates server certificate with CA:TRUE in extension
> section.

Could you please send me a copy of the cert?
Because I failed to produce a cert that is non-overridable by firefox.

Comment 48

[Kai Engert (:KaiE:)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #46)
> It depends on what Firefox's goals are. If Firefox goal is to offer a
> reasonable UI that protects non-experts (i.e. almost everybody) people from
> MitM attacks, then it makes a lot of sense for Firefox to become stricter
> and stricter about disallowing cert error overrides.

My goal is that Firefox is a usable browser for the real world.

As long as the real world produces broken devices that people have paid for and are required to use, we must allow them to override.

Make it as scary as you want, and make it as difficult to override as you want, but make it overridable.

This is exactly why we had worked hard in the past to require the multi-step process to add an override, which is much more difficult than most other browsers. But we still allowed it, because users require it.

It's painful that we have to have this argument again and again!

Comment 49

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #48)
> > It depends on what Firefox's goals are. If Firefox goal is to offer a
> > reasonable UI that protects non-experts (i.e. almost everybody) people from
> > MitM attacks, then it makes a lot of sense for Firefox to become stricter
> > and stricter about disallowing cert error overrides.

Brian, yes we DO protect users by DEFAULT, by rejecting the certs, and by requiring them to go through a multi-step override process, which gives them the chance to reconsider if they really want to do it. Read the warnings that are shown, the warnings make it clear, that users shouldn't do it.

But if users must override, allow them to.

Comment 50

[me.moz]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #46)
> (In reply to me.moz from comment #45)
> > Problem is that pkitool creates server certificate with CA:TRUE in extension
> > section.
> 
> Are they self-signed? If so, then the work that was already done for Firefox
> 33 should fix things.
> 
> If not, then pkitool is exposing your infrastructure to a huge security
> issue.

Yes, they are self-signed. And even if not, signing such a certificate with some public CA is IMHO unreal, they would not allow it.

> Either way, pkitool is open source and it should be very easy to write a
> patch to pkitool and contribute it upstream, to make pkitool generate
> end-entity certificates without basicConstrapints (equivalent to
> basicConstraints.cA == false).
> 
> > I think, there should be always an override allowed - you might only
> > disallow storing permanent exception for really invalid certificates (ie. CA
> > without CA:TRUE) - that all to make sites, that have been accessible before
> > FF31, accessible again (and most of them are users own intranet sites).
> 
> It depends on what Firefox's goals are. If Firefox goal is to offer a
> reasonable UI that protects non-experts (i.e. almost everybody) people from
> MitM attacks, then it makes a lot of sense for Firefox to become stricter
> and stricter about disallowing cert error overrides.

The biggest problem (for me) is that it stopped suddenly, without reasonable problem description and other browser have no problem on the same sites = whatever the problem is any non-expert will see such a change as a FF bug and make a switch. Power users (loving FF ;-) will try to find workaround, and make a switch when there is none - like in FF33+.

Comment 51

[Kai Engert (:KaiE:)]

(In reply to me.moz from comment #50)
> Yes, they are self-signed. And even if not, signing such a certificate with
> some public CA is IMHO unreal, they would not allow it.

FYI, in the scenario you have given to me, it's NOT self signed.

A self signed server certificate means, your server sends only one cert, and that one is self-signed.

What you are using is:
- your server certificate has been signed/issued by your private CA
- you have your own self-signed CA certificate
- you have your own CA certificate imported and marked as trusted

(In your scenario, if the certificate is available (imported) but untrusted, then Firefox allows to override.)


This means we have identified one scenario where users experience problems:

It affects users, who are using a private CA, for example a corporate CA, where the CA certificates or entity certificates contain flaws.

When the users upgrade their browser, they continue to use their old profiles, with old CA certificates, which are still marked as trusted. And they won't be able to override any more.


I don't have permission to share the link to the site, but I will attempt to reproduce equivalent certificates and setup a test service on my server.


Regardless, I've attached a patch to bug 1063315, and I've provided a link to a test build of Firefox 31 with that patch attached.

Using that build, I can successfully connect to the site mentioned above, with the configuration mentioned above.


You might ask why I'm adding more error codes in that patch: It's because I'm suggesting to restore the equivalent override capability that was used in the past.

Comment 52

[Kai Engert (:KaiE:)]

Attachment: a test CA certificate, must be imported and marked as trusted, prior to testing https://kuix.de:9455 (use a separate test profile!)

Comment 53

[Kai Engert (:KaiE:)]

I've setup a test service, which is equivalent to the nonworking web service, which is operated by the owner of the blog post, from comment 44 and 45.

In order to reproduce the failure, you can use Firefox 31 or Firefox 32.

Create a separate profile (e.g. using firefox -P)

Import the test CA certificate, attachment 8500981 [details], into the test profile, and mark it as trusted for web sites.

Visit https://kuix.de:9455

With the released Firefox 31/32, you get ca_cert_invalid, impossible to override.

Using the test build mentioned in bug 1063315 comment 41, it's possible to override.

Comment 54

[Kai Engert (:KaiE:)]

Regarding Firefox 33 beta and most recent Firefox 35 development version:

This test site still produces ca_cert_invalid, impossible to override:
  https://kuix.de:9452

Comment 55

[Johnathan Nightingale [:johnath]]

(In reply to Kai Engert (:kaie) from comment #48)
> (In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment
> #46)
> > It depends on what Firefox's goals are. If Firefox goal is to offer a
> > reasonable UI that protects non-experts (i.e. almost everybody) people from
> > MitM attacks, then it makes a lot of sense for Firefox to become stricter
> > and stricter about disallowing cert error overrides.
> 
> My goal is that Firefox is a usable browser for the real world.
> 
> As long as the real world produces broken devices that people have paid for
> and are required to use, we must allow them to override.
> 
> Make it as scary as you want, and make it as difficult to override as you
> want, but make it overridable.

I could scarcely have said it better than this. Interdict the load, but allow overrides.

> It's painful that we have to have this argument again and again!

You and I have both experienced that pain, but I understand that new issues can cause us to re-assess. I'm not faulting smart people for wanting to explore the problem before closing on a solution.

Having said that - Firefox's intent is that an override be possible, and I am satisfied that the warnings we put up are suitably effective at protecting novices from MitM. How quickly can we get to a fix that accomplishes that intent, and how safe will that fix be for uplift?

Comment 56

[Kai Engert (:KaiE:)]

Attachment: Patch for Mozilla 31 ESR branch

Comment 57

[Kai Engert (:KaiE:)]

Attachment: Patch for Mozilla 33 branch

(I assume we don't want to work on Firefox 32, because that branch will be obsoleted by Firefox 31 next week anyway.)

Using this patch, Firefox 33 allows to override all scenarios I'm aware of.


The previous attachment, patch for Firefox 31, also fixes all scenarios I'm aware of.


While most reports are about sec_error_ca_cert_invalid, other errors codes that were overridable in the past, had been removed from the overridable list. Both patches bring back all the error codes I saw in the past.


I will work on patches for the newer branches next. I need to build each branch on my local machine for testing. I hope I'll have locally tested patches for Firefox 34 and 35 by the end of the day.

Comment 58

[Kai Engert (:KaiE:)]

Attachment: Patch for Mozilla 34 and 35

tested, allows to override https://kuix.de:9452

Comment 59

[Kai Engert (:KaiE:)]

The situation is more complicated.

For the first time, I was given access to a test server, which:
- allows to override with mozillapkix true
- doesn't allow to override with mozillapkix false

In other words, even with mozillapkix disabled, some servers cannot be accessed.
My current patch doesn't fix those sites.

I'm starting to investigate.

Comment 60

[Kai Engert (:KaiE:)]

This is a test site, which cannot be overriden when mozillapkix is disabled (preference value set to false): https://kuix.de:9457
(And my current patch is insufficient.)

We're facing the very inconvenient situation, that we can no longer assume a standard configuration setting.

Some users have alraedy flipped the pref to one or the other value.

Because we had (falsely) assumed that setting the pref back to false were a good solution to restore all previous behaviour, we had already shipped the changed default "false" to some customers. And because it doesn't work for some, they have flipped it back to true.

I'm afraid, the only good solution for the Firefox 31 Enterprise Support Branch, avoiding all further confusion, is to make all scenarios overridable, regardless of the preference value.

Comment 61

[Kai Engert (:KaiE:)]

Attachment: Patch v3 for Mozilla 31

I found the additional place, where the error code needs to be added, in order to allow overrides with the classic (non-pkix) code path.

I'll give this additional testing this evening, and will request review later today.

Feel free to give early feedback.

Comment 62

[Kai Engert (:KaiE:)]

This comment contains complete testing instructions, for testing both modes of operation (pkix = true and pkix = false).

Please prepare two new, fresh Firefox profiles that will be used during testing.
Run Firefox from a command line, for easier selection of profile name and private browsing selection.
Switch (cd) to the directory where your firefox binary is stored.

We will use private browsing mode, to avoid setting permanent certificate exceptions, which allows for easier repeated testing.

Create two Firefox profiles:
  ./firefox -no-remote -CreateProfile test31-pkix
  ./firefox -no-remote -CreateProfile test31-notpkix
(both commands will exit immediately)

Disable pkix in the notpkix profile:
  ./firefox -no-remote -P test31-notpkix
  enter address: about:config
  type "pkix" to filter
  doubleclick security.use_mozillapkix_verification (which sets it to "false")
  exit firefox

In both profiles, load a root CA certificate, which will be used for one of the tests (the test on port 9455).
  ./firefox -no-remote -P test31-notpkix
  https://bugzilla.mozilla.org/attachment.cgi?id=8500981
  Do you want to trust Kai's Test CA?
  click (enable) the first checkbox "Trust ... websites", and OK
  exit firefox

  Run the pkix profile and repeat the above steps:
  ./firefox -no-remote -P test31-pkix
  https://bugzilla.mozilla.org/attachment.cgi?id=8500981
  Do you want to trust Kai's Test CA?
  click (enable) the first checkbox "Trust ... websites", and OK
  exit firefox

Now you are done preparing the test profiles. You only need to do the above once.
In the future, you can reuse both profiles for your tests, regardless which Firefox 31 builds you test (released or experimental).

(If you test other Firefox versions, you might consider to use separate profiles, 
e.g. instead of using test31-pkix, you could create and use test33-pkix etc.)


Now let's test.

We'll be using four test services:
https://kuix.de:9451
https://kuix.de:9452
https://kuix.de:9455
https://kuix.de:9457

They only differ in the last digit, so I'll use those digits as a shortcut to refer to them below.

Here are the tests that I've performed using the current most recent Firefox 31 build, which is 31.1.1

First, let's test the Firefox default configuration, mozillapkix enabled (set to true).
  ./firefox -no-remote -P test31-pkix -private
  https://kuix.de:9451
    failed, adding exception not offered
  https://kuix.de:9452
    failed, adding exception not offered
  https://kuix.de:9455
    failed, adding exception not offered
  https://kuix.de:9457
    untrusted, I understand, add exception, confirm, page loads

Second, let's test the nonstandard configuration, mozillapkix disabled (set to false).
  ./firefox -no-remote -P test31-notpkix -private
  https://kuix.de:9451
    untrusted, I understand, add exception, confirm, page loads
  https://kuix.de:9452
    untrusted, I understand, add exception, confirm, page loads
  https://kuix.de:9455
    page loads immediately
  https://kuix.de:9457
    failed, adding exception not offered

As you can see, with current Firefox 31, with each configuration we have at least one scenario, where adding an override is impossible.

The intention is that both configurations should allow to override for all test scenarios.

A build is fixed, if you get the result
  untrusted, I understand, add exception, confirm, works
for all of the above four test services (9451, 2, 5, 7).
It's also OK if test 9455 with notpkix just works, without failure, without need to add exception.

You're done.
(In case you want to be extra thorough, you could also test https://kuix.de:9453 and https://kuix.de:9454 and https://kuix.de:9456 which use a few other, slightly different configurations.)

Comment 63

[Kai Engert (:KaiE:)]

Comment on attachment 8501858 [details] [diff] [review]
Patch v3 for Mozilla 31

Firefox 31 ESR, try build with this patch, running at:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd7acb5a1437

Comment 64

[Kai Engert (:KaiE:)]

Comment on attachment 8501858 [details] [diff] [review]
Patch v3 for Mozilla 31

Just to clarify, with my local build and this patch attached, I get the expected results, as described in the previous comment (can add exception for each URL and access).

The test build I have mentioned in the previous comment should allow you to repeat my testing.

Comment 65

[Kai Engert (:KaiE:)]

Comment on attachment 8501058 [details] [diff] [review]
Patch for Mozilla 33 branch

With Firefox 33, which no longer allows to switch between pkix/not-pkix,
I see the following behaviour related to the above four sites:

  https://kuix.de:9451
    untrusted, I understand, add exception, confirm, page loads
  https://kuix.de:9452
    failed, adding exception not offered
  https://kuix.de:9455
    untrusted, I understand, add exception, confirm, page loads
  https://kuix.de:9457
    untrusted, I understand, add exception, confirm, page loads

My earlier patch for Firefox 33 was already sufficient (because Firefox 33 no longer contains the legacy code path that I fixed in patch v3.)

A try build with this patch runs here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4e29372acbc

Comment 66

[Kai Engert (:KaiE:)]

Comment on attachment 8501188 [details] [diff] [review]
Patch for Mozilla 34 and 35

The statements from comment 65 about Firefox 33, they also apply to Firefox 34.

The patch is slightly different, because in Firefox 34, you had already fixed one additional scenario (expired issuer cert).

The patch for Firefox 34 (aurora) also applies cleanly to Firefox 35 (mozilla-central).

A try build with this patch runs here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84dd78e13022

Comment 67

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

Comment on attachment 8501188 [details] [diff] [review]
Patch for Mozilla 34 and 35

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

The cert error override functionality is tested with the tests in security/manager/ssl/tests/unit/test_cert_overrides.js. When you add a new error to the list of errors that are allowed to be overridden, it is important to add the relevant tests to that file so that we know exactly which types of certificate problems cert error overrides are supported for. Otherwise, it will be nearly impossible for us to avoid adding regressions later.

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +136,5 @@
>      // Overridable errors.
>      case SEC_ERROR_UNKNOWN_ISSUER:
>      case SEC_ERROR_UNTRUSTED_ISSUER:
>      case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE:
>      case SEC_ERROR_UNTRUSTED_CERT:

SEC_ERROR_UNTRUSTED_CERT and SEC_ERROR_UNTRUSTED_ISSUER should be removed. More generally, NSSErrorsService.cpp and SSLServerCertVerification.cpp should be refactored to keep them in sync.

Also, IIRC there is third place that needs to be updated, because somebody copy/pasted this logic into somewhere else, but I can't remember where. I think David filed a bug about it recently.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +326,5 @@
>    // Assumes the error prioritization described in mozilla::pkix's
>    // BuildForward function. Also assumes that CERT_VerifyCertName was only
>    // called if CertVerifier::VerifyCert succeeded.
>    switch (defaultErrorCodeToReport) {
> +    case SEC_ERROR_INADEQUATE_KEY_USAGE:

This is the wrong bug for this change. This bug is about  sec_error_ca_cert_invalid.

@@ +333,4 @@
>      case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE:
>      case SEC_ERROR_UNKNOWN_ISSUER:
> +    case SEC_ERROR_UNTRUSTED_ISSUER:
> +    case SEC_ERROR_UNTRUSTED_CERT:

UNTRUSTED_ISSUER and UNTRUSTED_CERT are used for actively-distrusted certificates, for which overrides are intentionally not allowed (just like you requested).

I think that the old NSS-based verification may return those errors for different reasons, but mozilla::pkix does not. In Firefox 33 and later, mozilla::pkix is the only supported certificate verifier, so this part of the patch doesn't make sense.

Comment 68

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

Comment on attachment 8501858 [details] [diff] [review]
Patch v3 for Mozilla 31

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +339,3 @@
>      case SEC_ERROR_UNKNOWN_ISSUER:
> +    case SEC_ERROR_UNTRUSTED_ISSUER:
> +    case SEC_ERROR_UNTRUSTED_CERT:

At least hwne mozilla::pkix is being used, these two errors shouldn't be overridable, because they are used reporting actively distrusted certificates.

I think that the NSS-based verification is ambiguous and PSM cannot tell if the cert was rejected with one of these errors because the cert was actively distrusted or because of some less serious issue. If so, then it is NSS that needs to be changed, not PSM.

Comment 69

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #67)
> The cert error override functionality is tested with the tests in
> security/manager/ssl/tests/unit/test_cert_overrides.js. When you add a new
> error to the list of errors that are allowed to be overridden, it is
> important to add the relevant tests to that file so that we know exactly
> which types of certificate problems cert error overrides are supported for.
> Otherwise, it will be nearly impossible for us to avoid adding regressions
> later.

See also security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp and security/manager/ssl/tests/unit/tlsserver/generate_certs.sh, which will also need to be modified for additional test-cases.

> ::: security/manager/ssl/src/NSSErrorsService.cpp
> @@ +136,5 @@
> Also, IIRC there is third place that needs to be updated, because somebody
> copy/pasted this logic into somewhere else, but I can't remember where. I
> think David filed a bug about it recently.

Yes - bug 1057497. The file is dom/browser-element/BrowserElementChildPreload.js

Comment 70

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8501058 [details] [diff] [review]
Patch for Mozilla 33 branch

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +332,3 @@
>      case SEC_ERROR_UNKNOWN_ISSUER:
> +    case SEC_ERROR_UNTRUSTED_ISSUER:
> +    case SEC_ERROR_UNTRUSTED_CERT:

These added values would also need to be handled in MapCertErrorToProbeValue, above this function.

Comment 71

[Kai Engert (:KaiE:)]

Brian,

the intention is to make absolutely sure, that all kind of certificate errors can be overriden, regardless what there individual error codes are. The set of error codes is based on what had been used in the past.

We can change the subject of this bug to cover all scenarios.

I don't want to work on one error code after the other, in separate bugs, because this is the strategy you had used so far, and it hasn't worked (refer to the fact that you have incrementally add more error codes in Firefox 32, 33, 34.)

Firefox should implement code, that allows any kind of rejected certificate to be overriden, unless it's a fatal failure (revoked or actively distrusted).

Comment 72

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8501858 [details] [diff] [review]
Patch v3 for Mozilla 31

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

Clearing review for now, since these patches all need similar changes.

Comment 73

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

(In reply to Kai Engert (:kaie) from comment #71)
> I don't want to work on one error code after the other, in separate bugs,
> because this is the strategy you had used so far, and it hasn't worked
> (refer to the fact that you have incrementally add more error codes in
> Firefox 32, 33, 34.)

This was largely due to miscommunication in the expected vs. actual testing procedures during compatibility testing with mozilla::pkix, combined with too small of a dataset for the compatibility testing. (In particular, if a test failed with mozilla::pkix and the test also failed with NSS-based certificate verification, we considered that a match, even if the reported errors were different, which was a mistake.) Note that the overal approach for deciding on whether to allow a cert override or not did not change; PSM is still doing things nearly exactly the same way it has been for years.

> Firefox should implement code, that allows any kind of rejected certificate
> to be overriden, unless it's a fatal failure (revoked or actively
> distrusted).

Firefox cannot tell if a certificate has been revoked if it doesn't have a valid CA certificate to check the OCSP response signature with. Consequently, the more one wants to make sure you get the "revoked or actively distrusted" part correct, the more strict one needs to be about overrides for untrusted issuers.

Anyway, please re-read my comments on your patches. Your patch allows overrides for actively distrusted certificates too, so it doesn't do what you intended. I am pretty sure that if you were to run "mach xpcshell-test security/manager/ssl/tests/unit" with your patch applied then the tests regarding actively distrusted certificates would fail, to help you understand this.

Comment 74

[Kai Engert (:KaiE:)]

Thanks for suggesting that the "untrusted" error are used for the actively distrusted scenarios. If true, I'm OK not to add them back for pkix.

I will look at the other comments in more detail tomorrow.

Comment 75

[Patrick Cain]

This issue is frustrating for us as our corporate users now can't use Oracle Enterprise Manager, Symantec OpsCenter and various other web interfaces. These apps generate a local certificate for the webserver just so the traffic is secure over the local network. Initially we were able to get around it by putting security.use_mozillapkix_verification = false in about:config but this no longer works.

We don't have time to mess around with editing auto-generated certificates or wait for a FF fix for this so have had to install Google Chrome for 3000 staff. It seems likely that the next step will be to remove FF as it's too much admin overhead to maintain IE (needed by many apps), FF & Chrome.

Comment 76

[Marek Raida]

Unfortunately, I agree that it is still causing us some problems in our Alma corporate as well :-((
Not so massive, but still...

Comment 77

[Kai Engert (:KaiE:)]

Given the huge frustration this is apparently giving users, I propose that we try to get Firefox 31 and Firefox 33 hotfixed, for the release on October 14, and sort out the "right way" for Firefox 34/35 after that.

Comment 78

[Sylvestre Ledru [:Sylvestre]]

Kai, what do you mean by hotfixed? The 33 release has been built already and AFAIK, there is no driver for a new build. Thanks

Comment 79

[Kai Engert (:KaiE:)]

I did more testing, and setup a test case for sec_error_inadequate_cert_type at https://kuix.de:9458

It seems it wasn't possible to override this error with past releases. I tested Firefox 6, 17 and 24, and none of them allowed to override. So maybe it's OK to omit this error code.

Comment 80

[Kai Engert (:KaiE:)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #68)
> Patch v3 for Mozilla 31
> 
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +339,3 @@
> >      case SEC_ERROR_UNKNOWN_ISSUER:
> > +    case SEC_ERROR_UNTRUSTED_ISSUER:
> > +    case SEC_ERROR_UNTRUSTED_CERT:
> 
> At least hwne mozilla::pkix is being used, these two errors shouldn't be
> overridable, because they are used reporting actively distrusted
> certificates.

We need these overridable in Firefox 31, for the not-pkix scenario.
The test case for https://kuix.de:9452/ produces sec_error_untrusted_issuer, and that isn't an actively distrusted scenario.

Comment 81

[Sylvestre Ledru [:Sylvestre]]

Brian, David, I am considering the impact on the ESR release. Do you think that Kai's patches could be merged soon in case we want to ship ESR 31.2.0 with it? Thanks

Comment 82

[Kai Engert (:KaiE:)]

Attachment: Patch v4 for Mozilla 31

I haven't tested this patch yet.
My local build is still running.
And a try build is ongoing here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=891cdb566b5b

Comment 83

[Kai Engert (:KaiE:)]

Comment on attachment 8502608 [details] [diff] [review]
Patch v4 for Mozilla 31

(In reply to Kai Engert (:kaie) from comment #82)
> I haven't tested this patch yet.
> My local build is still running.

Local test completed.
All my manual tests passed.

Sorry, at this time, I don't have time to learn how your new automated tests work.
I already spend a large amount of time this week to track down this issue, and with creating all the test cases that I have made publicly available.

If you consider the automated tests a blocker for getting the ESR31 branch fixed, then I'd like to ask that you please help to get the tests done.

Comment 84

[Kai Engert (:KaiE:)]

Attachment: Patch v4 for Mozilla 33

merged, equivalent to the moz-31 patch, untested

Comment 85

[Kai Engert (:KaiE:)]

Attachment: Patch v4 for Mozilla 34 and 35

merged, equivalent to the moz-31 patch, untested

Comment 86

[Kai Engert (:KaiE:)]

Comment on attachment 8502635 [details] [diff] [review]
Patch v4 for Mozilla 33

My local testing of this patch with the beta moz 33 branch was successful.

Comment 87

[Benjamin Kerensa [:bkerensa]]

As ESR owner I just wanted to chime in and say we will take a patch fixing this.

Comment 88

[Kai Engert (:KaiE:)]

Attachment: Patch v5 for Mozilla 31

My previous try build for esr-31 failed, because the existing tests had explicitly tested that ca_cert_invalid is non-overridable, in my understanding.

This is an updated patch, which should silences those failures, by changing the existing tests against ca_cert_invalid into expected overridable tests.

With this patch applied, and locally running
  mach xpcshell-test --sequential security/manager/ssl/tests/unit
the unit tests pass on my system.

I've started an additional try build:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=42f598d7057d

Comment 89

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8502745 [details] [diff] [review]
Patch v5 for Mozilla 31

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

My understanding is that we're doing this because on esr-31, if mozilla::pkix is disabled, some certificate errors that should be overridable aren't. I think this will work fine for esr-31, but I maintain that we should do something like what I outlined in comment 38 for mozilla-central. That fix can then be uplifted to beta and aurora. With classic verification, in general we don't have a great idea of what conditions lead to which errors. In mozilla::pkix, we have much better control over this, which means we can come up with a more targeted solution that solves the issue for those affected by it without exposing the majority of our users to unnecessary risk.

r=me for esr-31 with comments addressed.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +306,5 @@
>      case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:  return  8;
>      case SSL_ERROR_BAD_CERT_DOMAIN:                    return  9;
>      case SEC_ERROR_EXPIRED_CERTIFICATE:                return 10;
> +                    // 11 is already used in newer mozilla branches
> +    case SEC_ERROR_CA_CERT_INVALID:                    return 12;

This used to be 3, so please use that value (consequently, it would be nice if this line were between SEC_ERROR_UNKNOWN_ISSUER and SEC_ERROR_UNTRUSTED_ISSUER).

@@ +335,5 @@
>    // called if CertVerifier::VerifyCert succeeded.
>    switch (defaultErrorCodeToReport) {
>      case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
> +    case SEC_ERROR_CA_CERT_INVALID:
> +    case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE:

mozilla::pkix doesn't return SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE in esr31, so this case shouldn't be here.

Comment 90

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8502608 [details] [diff] [review]
Patch v4 for Mozilla 31

My understanding is this patch is obsoleted by the one I just reviewed.

Comment 91

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #81)
> Brian, David, I am considering the impact on the ESR release. Do you think
> that Kai's patches could be merged soon in case we want to ship ESR 31.2.0
> with it? Thanks

Once my review comments are addressed, I believe this will be good to go for ESR 31.2.0 (provided the try run went well - it looks like our build infrastructure is struggling at the moment).

Comment 92

[Kai Engert (:KaiE:)]

Thank you David. I'll make a new patch. I've already resubmitted the try build because of the test infrastructure failures:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5f9bd640a047

Comment 93

[Kai Engert (:KaiE:)]

Attachment: again... Patch v5 for Mozilla 31

This patch addresses David's change requests from comment 89.
Carrying forward r+.

Comment 94

[Kai Engert (:KaiE:)]

Comment on attachment 8502884 [details] [diff] [review]
again... Patch v5 for Mozilla 31

sorry, attached wrong file

Comment 95

[Kai Engert (:KaiE:)]

Attachment: Patch v6 for Mozilla 31

correct file, fixed as David requested, carrying forward r+, requesting esr31 approval.

Comment 96

[Kai Engert (:KaiE:)]

FYI, the following tests can be ignored, as I've been told by RyanVM:
- Windows Mn 
- Web platform (WR, W1, etc.)
- M-e10s

Comment 97

[Kai Engert (:KaiE:)]

Attachment: Patch v6 for Mozilla 33

This is the patch merged to the Mozilla 33 beta branch, in case you decide you want to fix this bux in the Mozilla 33.0 release, too.

Comment 98

[Benjamin Kerensa [:bkerensa]]

Comment on attachment 8502886 [details] [diff] [review]
Patch v6 for Mozilla 31

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

Approved

Comment 99

[Kai Engert (:KaiE:)]

Comment on attachment 8502886 [details] [diff] [review]
Patch v6 for Mozilla 31

checked in:
https://hg.mozilla.org/releases/mozilla-esr31/rev/878a9dd89f82

Comment 100

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

Clearing NEEDINFO. David is managing this.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

Backed out from esr31 for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-esr31/rev/0fdf9fee4314

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4698&repo=mozilla-esr31

Comment 102

[Kai Engert (:KaiE:)]

Attachment: Patch v7 for Mozilla 31 (is v6 with bustage fix)

I understand the test bustage, which was caused by a failed check of expectations.

The expected number of telemetry reports for ca_cert_invalid is no longer "zero". Because we added two overrides for that error, the expected number is now "two".

Because all builds failed with the same error, I did a single test run with the bustage fix, adjusting the expected number to "two".

The try run has succeeded:
https://tbpl.mozilla.org/?tree=Try&rev=73feb2ec915c

I've relanded this updated patch which includes the bustage fix:
https://hg.mozilla.org/releases/mozilla-esr31/rev/83d6db7c3de1

Comment 103

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
renominating for tracking because this seems to be causing some installations significant pain.

Comment 104

[Lawrence Mandel [:lmandel] (use needinfo)]

Sylvestre tells me that we're going to build for Firefox 33.0 again tomorrow morning. Given that we are building again, we probably should take this. I'm somewhat uncomfortable with taking a patch that has not been well tested. However, if the final patch is what is attached (or very similar), it is not new code paths that have been added but rather a new way to invoke existing code paths. This makes me somewhat more comfortable. In order to take this we're going to need approval requests indicating that the patches are ready and we're going to need someone to land the patches as we will not likely have sheriff coverage over the weekend.

I think that we should land this change across the board (central, aurora, beta) and then rip it out of whatever release we can implement the proper solution.

Kai - Does that make sense to you?

Comment 105

[Kai Engert (:KaiE:)]

It makes sense to me, because I personally recommended that a fix should be immediately rolled out to all users of Firefox - not limited to ESR 31 - because too many users are affected.

However, based on previous comments, my understanding is that David Keeler doesn't like the idea to use my current patch for Firefox 33 or more recent branches, because he would like to implement something that is cleaner. See his comment 89, which indicates that his r+ is valid only for Firefox 31.

On the other hand, we don't have that cleaner implementation yet, and I won't be able to help with it.

My personal recommendation is to land patches into Firefox 33, 34, 35, immediately, based on the patch I made for Firefox 31, because my manual testing suggests that the approach solves the problem. Afterwards the PSM team can work to clean this up, and add automated tests, in the way they prefer later.

Can we get David to accept this plan?

The patches for 33/34/35 will look slightly different, because some context has slightly changed.

I'm willing to make the required patches. There is already a patch for Firefox 33 attached to this bug (Patch v6 for Mozilla 33), which should still work. It shouldn't need the test silencing I made for Firefox 31, because there aren't any tests in Firefox 33 that test for override scenarios with the ca_cert_invalid error.

So, you could go ahead immediately with landing Firefox 33.
I can work on the adjusted patches for 34 and later during the weekend.

Comment 106

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Bug 991177 made the change that disallows overrides for the error SEC_ERROR_CA_CERT_INVALID. This landed in 31, so we have already shipped 2 versions where overriding this error is not possible. Speaking as PSM module owner, I think it would be more risky to rush a fix out for 33 than to land a more targeted approach in mozilla-central, do some compatibility testing, and uplift as appropriate. I realize this means shipping another version with the same issue. However, in the meantime, users who are effected by it can use esr-31.

Comment 107

[Kai Engert (:KaiE:)]

David, I don't understand where you see a risk.

This patch clearly adds override capability for the mentioned error, only.
It doesn't change anything else.

At worst, someone would be able to override ca_cert_invalid, if the code produced it for a really weird scenario.
I don't think that's a risk, because it's still a warning, and the user has the chance to stop.

Comment 108

[Lawrence Mandel [:lmandel] (use needinfo)]

For the benefit of those people not awake and reading irc, I asked keeler whether it is possible to enable the override capability via an add-on. The short answer is no. We're going to have to make a call on this bug for Firefox 33. The default is clearly to ship as we did with Firefox 31 and 32. I don't know that that is the best option for our users. (Again, I really don't know how many users we expect this change to impact and if things will be significantly worse than Firefox 32.)

Gavin - As this impacts Firefox, I thought you might be able to help with an assessment and opinion here as well.

Comment 109

[Matt Wobensmith [:mwobensmith][:matt:]]

I confirmed the previous behavior on Fx31.1.1, exactly as documented.
I verified that all errors are now overridable in the release candidate of Fx31.2.0esr.
So, I'm able to mark the ESR build as verified.

Obviously we are still waiting on decisions for Fx33.

On a personal note, thank you Kai for providing such excellent test instructions in comment 62. It is very much appreciated.

Comment 110

[Sylvestre Ledru [:Sylvestre]]

I didn't take it for 33 for the following arguments:
* it has been introduced in 31. We shipped 2 releases with these bugs...
 
* this didn't get enough testing.

* both patches have been done at the last minute and might have side effects. In case something goes wrong, the cost of doing a new ESR is far less than the release.

* I have the feeling that it impacts more our enterprise users than our regular users.

* 33.1 is not far away. If aurora+beta+esr 31.2.0 are fine with these changes, we could take both patches (therefor, I am going to keep the tracking on this).

Comment 111

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 1/3: add error for untrusted v1 CA certs

Brian, this adds an error specifically in the case where we encounter a non-trust-anchor x509v1 certificate without a basic constraints extension that is being used as a CA. The idea is to allow overrides for only this case, because while it appears that people are encountering this in the wild, we still don't want to allow overrides for the general case where, for example, an x509v3 certificate either doesn't have a basic constraints extension or has one where CA=FALSE.

Comment 112

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 3/3: tests

Comment 113

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8504397 [details] [diff] [review]
patch 3/3: tests

Whoops - this is patch with the tests.

Comment 114

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 2/3: allow overrides for the new error

Comment 115

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8504396 [details] [diff] [review]
patch 1/3: add error for untrusted v1 CA certs

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

::: security/pkix/lib/pkixcheck.cpp
@@ +357,5 @@
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA && version == der::Version::v1) {
> +      if (trustLevel == TrustLevel::TrustAnchor) {
> +        isCA = true;
> +      } else {
> +        return Result::ERROR_UNTRUSTED_V1_CERT_USED_AS_CA;

The comment around line 379 needs an update

// End-entity certificates are not allowed to act as CA certs.

Comment 116

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8504397 [details] [diff] [review]
patch 3/3: tests

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

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +129,5 @@
>                           getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY));
> +
> +  add_cert_override_test("end-entity-issued-by-untrusted-v1-cert.example.com",
> +                         Ci.nsICertOverrideService.ERROR_UNTRUSTED,
> +                         getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_UNTRUSTED_V1_CERT_USED_AS_CA));

whitespace

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +415,5 @@
> +  check_cert_err(cert_from_file('v1_ee-v1_int_bc-v3_ca_missing_bc.der'), expected_error);
> +  check_cert_err(cert_from_file('v1_bc_ee-v1_int_bc-v3_ca_missing_bc.der'), expected_error);
> +  check_cert_err(cert_from_file('v2_ee-v1_int_bc-v3_ca_missing_bc.der'), expected_error);
> +  check_cert_err(cert_from_file('v2_bc_ee-v1_int_bc-v3_ca_missing_bc.der'), expected_error);
> +  check_cert_err(cert_from_file('v3_missing_bc_ee-v1_int_bc-v3_ca_missing_bc.der'), expected_error);

over 80 here and below and above

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +154,5 @@
> +  if [ $ALREADY_EXISTS -eq 1 ]; then
> +    echo "cert \"$NICKNAME\" already exists - not regenerating it (use --clobber to force regeneration)"
> +    return
> +  fi
> +

It probably deserves a comment that this is being verified in the context of being a CA and is v1 and etc...

Comment 117

[:Cykesiopka]

Comment on attachment 8504399 [details] [diff] [review]
patch 2/3: allow overrides for the new error

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +95,5 @@
>      case SSL_ERROR_BAD_CERT_DOMAIN:
>      case SEC_ERROR_EXPIRED_CERTIFICATE:
>      case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
>      case MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY:
> +    case MOZILLA_PKIX_ERROR_UNTRUSTED_V1_CERT_USED_AS_CA:

Doesn't MOZILLA_PKIX_ERROR_UNTRUSTED_V1_CERT_USED_AS_CA need to be defined higher up in this file?

Comment 118

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

Comment on attachment 8504396 [details] [diff] [review]
patch 1/3: add error for untrusted v1 CA certs

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

::: security/pkix/lib/pkixcheck.cpp
@@ +345,5 @@
>      //  asserted, then the certified public key MUST NOT be used to verify
>      //  certificate signatures."
>      //
>      // For compatibility, we must accept v1 trust anchors without basic
>      // constraints as CAs.

Add a blank comment line here.

@@ +350,5 @@
> +    // There are devices with v1 certificates that are unlikely to be trust
> +    // anchors. In order to allow access to them but not allow overrides for all
> +    // cases where an issuing certificate is missing the basic constraints
> +    // extension, we use a specific error in this case that is overridable. The
> +    // more general error is not overridable.

I would replace the latter two sentences with "In order to allow applications to treat this conditional differently from other basic constraint violations (e.g. allowing cert error overrides only for this case), we return a different error code." This minimizes the Firefox-specific nature of the comment, which is probably good if y'all are still planning to put mozilla::pkix into NSS.

@@ +357,5 @@
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA && version == der::Version::v1) {
> +      if (trustLevel == TrustLevel::TrustAnchor) {
> +        isCA = true;
> +      } else {
> +        return Result::ERROR_UNTRUSTED_V1_CERT_USED_AS_CA;

I would avoid the word "UNTRUSTED" because that is used in other error codes to mean "distrusted". "ERROR_UNTRUSTED_V1_CERT_USED_AS_CA" seems good.

::: security/pkix/lib/pkixnss.cpp
@@ +245,5 @@
> +      "to establish a secure connection." },
> +    { "MOZILLA_PKIX_ERROR_UNTRUSTED_V1_CERT_USED_AS_CA",
> +      "An X509 version 1 certificate that is not a trust anchor was used to "
> +      "issue the server's certificate. X509 version 1 certificates are "
> +      "deprecated and should not be used to sign other certificates." },

s/X509/X.509/

Comment 119

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

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #118)
> I would avoid the word "UNTRUSTED" because that is used in other error codes
> to mean "distrusted". "ERROR_UNTRUSTED_V1_CERT_USED_AS_CA" seems good.

Er, "ERROR_V1_CERT_USED_AS_CA" seems good, I mean.

Comment 120

[TG2]

rant/

I really don't want to be labeled a d**k here ... but all of you that have done this really have to get on board with the ALWAYS ALLOW an override.

One issue you *ards seem to also have not gotten through.. you "allow" for overrides thinking its a multi-step process and that you're being secure.. but ID10T error is that you DEFAULTED the "permanently store" to checked!

you can not keep doing this to the users. EITHER part.. I'm into 50+ sites in a day.. that depending on them being internally ip'd (RFC1918) or externally IP'd that I've got to go into about:config ... search for your f**ked up PKI and change it from False to True.. and vise versa all depending on IP with broken certs.. 

these are certs end users, EVEN PROFESSIONAL TECHNICIANS HAVE ABSOLUTELY NO F**KING CONTROL OVER AND ARE REQUIRED TO USE THE SITES FOR ACCESS.. and yet EVERY F**KING ONE OF YOU, except mabye Kai thinks that its acceptible to be the **ONLY** browser causing this much of a disruption to people's work with this work-around bs..

the answer is simple.. REGARDLESS OF THE ERROR .. You allow a person to temporarily *or* permanently add an exception to the site. and THAT is the END of it!

You've had this f**ked up since 31 and here two revs later.. STILL screwing the users.. WTF dev team?

I DARE you to take any one of the sites that *IS* a problem, and go to IE, or Chrome and see how EVERYONE ELSE can get to their broken https sites with those OTHER browsers.. but not your precious Firefox.

A do apologize that this post is probably inflamitory, but some of you super smart people need a swift kick in the a** to realize you've REALLY tarnished what was a decent browser with this BS and the longer it continues, the more of us that relied upon you to allow us to do our jobs.. are going to switch to something else.. even downgrade and stay because you REALLY don't seem to get the f**king clues how much you've destroyed OUR day.

/rant

Comment 121

[:Gavin Sharp [email: [e-mail]]]

(In reply to Lawrence Mandel [:lmandel] from comment #108)
> Gavin - As this impacts Firefox, I thought you might be able to help with an
> assessment and opinion here as well.

Sounds like the decision was already made here, but I agree with Sylvestre's rationale in comment 110.

Comment 122

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thank you for the reviews, everyone. I believe I've either addressed all of your comments or we've resolved them in person (particularly for comment 115 and comment 116).

Here's some try runs:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4e5b4954ed2
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fcc21c05374b

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48ab37c74639
https://hg.mozilla.org/integration/mozilla-inbound/rev/3127d128a14d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4dc550b3f0

I'll attach the updated patches shortly.

Comment 123

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 1/3v2: add error for untrusted v1 CA certs

Comment 124

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 2/3v2: allow overrides for the new error

Comment 125

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 3/3v2: tests

Comment 126

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48ab37c74639
https://hg.mozilla.org/mozilla-central/rev/3127d128a14d
https://hg.mozilla.org/mozilla-central/rev/fc4dc550b3f0

Comment 128

[avatar11]

(In reply to Carsten Book [:Tomcat] from comment #126)
> https://hg.mozilla.org/mozilla-central/rev/48ab37c74639
> https://hg.mozilla.org/mozilla-central/rev/3127d128a14d
> https://hg.mozilla.org/mozilla-central/rev/fc4dc550b3f0

You did not fix this.

Comment 129

[Kai Engert (:KaiE:)]

I confirm that test case https://kuix.de:9451 still fails with the latest FF36 nightly.

Comment 130

[Sylvestre Ledru [:Sylvestre]]

David, it seems that it didn't cause regression in ESR or aurora/beta (not sure that they are affected by the issue in comment #129). I think I am going to take this bug in 33.1. Could you fill the uplift request for mozilla-release? thanks

Comment 131

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #130)
> David, it seems that it didn't cause regression in ESR or aurora/beta (not
> sure that they are affected by the issue in comment #129). I think I am
> going to take this bug in 33.1. Could you fill the uplift request for
> mozilla-release? thanks

Just so we're all on the same page, you're saying you want the fix that went into ESR to be uplifted to mozilla-release? If so, Kai should probably drive that, since he wrote/landed that patch.

Comment 132

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Kai Engert (:kaie) from comment #129)
> I confirm that test case https://kuix.de:9451 still fails with the latest
> FF36 nightly.

Looks like that involves an x509v3 certificate with no basic constraints attempting to sign a certificate. I don't think that's something we should allow overrides for. In any case, we're going to gather some more data on how often what sorts of errors happen in bug 1085506 and bug 1085509. Hopefully with some more data we can make a more informed decision about what the right balance between safety and usability is, given the resources we have and how Firefox handles certificate errors.

Comment 133

[Sylvestre Ledru [:Sylvestre]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #131)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #130)
> Just so we're all on the same page, you're saying you want the fix that went
> into ESR to be uplifted to mozilla-release? If so, Kai should probably drive
> that, since he wrote/landed that patch.

Yes, that is what I meant :)

Comment 134

[Ken]

Well,

To add another large group, HP Gen 7 servers you cannot access the ILO for management in FF33.
I realize that Firefox is trying to greatly improve web security, but for embedded devices that are using ssl only to prevent snooping on the wire, this breaks many of them ... and the thought of generating 1000's of new certs and installing one by one ... I'll switch browsers before I do that ...

 
(In reply to Sylvestre Ledru [:sylvestre] from comment #133)
> (In reply to David Keeler (:keeler) [use needinfo?] from comment #131)
> > (In reply to Sylvestre Ledru [:sylvestre] from comment #130)
> > Just so we're all on the same page, you're saying you want the fix that went
> > into ESR to be uplifted to mozilla-release? If so, Kai should probably drive
> > that, since he wrote/landed that patch.
> 
> Yes, that is what I meant :)

Comment 135

[TG2]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #132)
> (In reply to Kai Engert (:kaie) from comment #129)
> > I confirm that test case https://kuix.de:9451 still fails with the latest
> > FF36 nightly.
> 
> Looks like that involves an x509v3 certificate with no basic constraints
> attempting to sign a certificate. I don't think that's something we should
> allow overrides for. In any case, we're going to gather some more data on
> how often what sorts of errors happen in bug 1085506 and bug 1085509.
> Hopefully with some more data we can make a more informed decision about
> what the right balance between safety and usability is, given the resources
> we have and how Firefox handles certificate errors.

Again, arrogant thinking that you know what is best for everyone.
Let the PEOPLE decide.  If you really wanted to make this issue better, you would provide immediate details on the cause for concern, you would allow people to make an exception.. **AND** on the exception page the *DEFAULT* would not be to PERMANENTLY STORE THE EXCEPTION!

Why does no one get this concept?

you (meaning mozilla / firefox) allow exceptions, but instead of making people *actively select* the permanent exception for a security bypass, you've defaulted it to permanent for them!?

**ALL** errors need the ability to be bypassed (exceptions allowed for, by the user).  You want to create exception after exception after exception .. stop wasting your time .. and leave the exceptions up to the USERS for the most part.  What Firefox SHOULD be doing, is tracking the exceptions that users make .. perhaps a prompt "would you like to have this exception shared with the Developer Teams?" so that users coming across something they think shouldn't require steps to evade hapless nonsensical security may at some point get the facts directly to the Dev Team, rather than making people come to the bugzilla and post!

again.. these are concept 101 ideas why are they not sinking in to you people?

Comment 137

[Richard Barnes [:rbarnes]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #132)
> (In reply to Kai Engert (:kaie) from comment #129)
> > I confirm that test case https://kuix.de:9451 still fails with the latest
> > FF36 nightly.
> 
> Looks like that involves an x509v3 certificate with no basic constraints
> attempting to sign a certificate. I don't think that's something we should
> allow overrides for. In any case, we're going to gather some more data on
> how often what sorts of errors happen in bug 1085506 and bug 1085509.
> Hopefully with some more data we can make a more informed decision about
> what the right balance between safety and usability is, given the resources
> we have and how Firefox handles certificate errors.

There's been some further discussion on how to strike the right balance here.  I would propose the following plan:

1. For certs that chain to an installed trust anchor, apply strict checks as we do today
2. For certs with an unknown root, always report sec_error_unknown_issuer (which is overrideable)

Does that seem like it would address the use cases that are driving this bug?

Comment 138

[IU]

(In reply to Richard Barnes [:rbarnes] from comment #137)
> There's been some further discussion on how to strike the right balance
> here.  I would propose the following plan: ...

How about also, for these particular cases that Mozilla is concerned about, also leaving as unchecked "Permanently store this exception"?  That would be another reasonable mitigation.

Comment 139

[Ken]

I believe you balance would work nicely.

As for "Permanently store this exception", I believe that should be off by default. If the user is overriding security to begin with, let them choose to make it permanent exception. With HP equipment I've had issues of duplicate serial numbers. I use a plugin to turn off by default making the exception permanent.

Comment 140

[Kai Engert (:KaiE:)]

(In reply to Richard Barnes [:rbarnes] from comment #137)
> 2. For certs with an unknown root, always report sec_error_unknown_issuer
> (which is overrideable)

I don't know if that's sufficient.

You should make sure bad certs are overridable, regardless of the reason why the cert is interpreted as bad (as long as it's non-revoked).

I'm not sure I understand your correctly. If your plan is to change the code to never return sec_error_ca_cert_invalid, but instead return sec_error_unknown_issuer for those bad certs - then I don't see the benefit of this change, and I'd just allow ca_cert_invalid to be overridable.

Comment 141

[Kai Engert (:KaiE:)]

(In reply to Ken from comment #139)
> As for "Permanently store this exception", I believe that should be off by
> default. If the user is overriding security to begin with, let them choose
> to make it permanent exception. With HP equipment I've had issues of
> duplicate serial numbers. I use a plugin to turn off by default making the
> exception permanent.

I don't disagree, but I think that's a separate issue.
I suggest to file a separate bug "change the override dialog to default to non-permanent".
I'd personally be in favor of that change (but I'm not making that decision).

Comment 142

[Richard Barnes [:rbarnes]]

(In reply to Kai Engert (:kaie) from comment #141)
> (In reply to Ken from comment #139)
> > As for "Permanently store this exception", I believe that should be off by
> > default. If the user is overriding security to begin with, let them choose
> > to make it permanent exception. With HP equipment I've had issues of
> > duplicate serial numbers. I use a plugin to turn off by default making the
> > exception permanent.
> 
> I don't disagree, but I think that's a separate issue.
> I suggest to file a separate bug "change the override dialog to default to
> non-permanent".
> I'd personally be in favor of that change (but I'm not making that decision).

This has been tried, and the UX guys have a pretty plausible story for why it is the way it is:
https://bugzilla.mozilla.org/show_bug.cgi?id=536197#c1

Comment 143

[Richard Barnes [:rbarnes]]

(In reply to Kai Engert (:kaie) from comment #140)
> (In reply to Richard Barnes [:rbarnes] from comment #137)
> > 2. For certs with an unknown root, always report sec_error_unknown_issuer
> > (which is overrideable)
> 
> I don't know if that's sufficient.
> 
> You should make sure bad certs are overridable, regardless of the reason why
> the cert is interpreted as bad (as long as it's non-revoked).

This is not something we are going to agree to in general.  In the case where the site is using a "real PKI" cert from a root in our program, we should be strict.

In other words: You use our PKI, you play by our rules.  Use your own PKI, and you get to play by your rules, but you have to get users to opt in.


> I'm not sure I understand your correctly. If your plan is to change the code
> to never return sec_error_ca_cert_invalid, but instead return
> sec_error_unknown_issuer for those bad certs - then I don't see the benefit
> of this change, and I'd just allow ca_cert_invalid to be overridable.

Without getting into the details, the idea would be to make any error overrideable if we can't find a path to a real CA.

Comment 144

[Kai Engert (:KaiE:)]

(In reply to Richard Barnes [:rbarnes] from comment #143)
> > You should make sure bad certs are overridable, regardless of the reason why
> > the cert is interpreted as bad (as long as it's non-revoked).
> 
> This is not something we are going to agree to in general.  In the case
> where the site is using a "real PKI" cert from a root in our program, we
> should be strict.
> 
> In other words: You use our PKI, you play by our rules.  Use your own PKI,
> and you get to play by your rules, but you have to get users to opt in.

Sorry, but I don't understand what this means in practice.

Comment 145

[me.moz]

Just an idea:
What about adding an about:config option, that makes everything overridable. Standard user will not probably use it/need it/ever look for it - and power users, dealing with dummy equipments or whatever will IMHO easily find how to enable it...

Comment 146

[Ray Satiro]

(In reply to Richard Barnes [:rbarnes] from comment #143)
> (In reply to Kai Engert (:kaie) from comment #140)
> > (In reply to Richard Barnes [:rbarnes] from comment #137)
> > > 2. For certs with an unknown root, always report sec_error_unknown_issuer
> > > (which is overrideable)
> > 
> > I don't know if that's sufficient.
> > 
> > You should make sure bad certs are overridable, regardless of the reason why
> > the cert is interpreted as bad (as long as it's non-revoked).
> 
> This is not something we are going to agree to in general.  In the case
> where the site is using a "real PKI" cert from a root in our program, we
> should be strict.
> 
> In other words: You use our PKI, you play by our rules.  Use your own PKI,
> and you get to play by your rules, but you have to get users to opt in.

I really would like to be able to override anything, I don't particularly care why it's failing. I'll go even further than Kai and say I don't even care if it's revoked. I want to be able to override anything. I need access to the information and I am smart enough to make the determination. I think something that is often overlooked in discussions like this is that the user may not even be using HTTPS for something that needs to be secure, they may be forced into HTTPS for some other reason. Let the (advanced) user be the judge. Here's a problem I've encountered occasionally:

https://bugzilla.mozilla.org/show_bug.cgi?id=403220#c46

Comment 147

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

(In reply to Richard Barnes [:rbarnes] from comment #137)
> There's been some further discussion on how to strike the right balance
> here.  I would propose the following plan:
> 
> 1. For certs that chain to an installed trust anchor, apply strict checks as
> we do today
> 2. For certs with an unknown root, always report sec_error_unknown_issuer
> (which is overrideable)

This would not work that well. As soon as we see a problem with an intermediate cert, we stop building a chain using that intermediate, and instead try to find a better intermediate. It is possible (even likely) that we'll find a better intermediate; if so, any time that we spend trying to see if the bad intermediate chained to a trust anchor would be wasted. And, this would add otherwise-unnecessary complication to mozilla::pkix.

Please consider this scenerio:

       --- CA1(v1, not expired) <--- Root1
      /                             
EE <--                               
      \                             
       --- CA1(v3, expired) <--- Untrusted self-signed cert

In this scenerio, mozilla::pkix will return ERROR_UNKNOWN_ISSUER, which is overridable, because one path results in ERROR_V1_CERT_USED_AS_CA (or whatever) and the other path results  in ERROR_EXPIRED_ISSUER_CERT. Thus, the cert error would be customizable.

Now, the question is whether to allow a cert error override for the case where mozilla::pkix only finds the top chain. IMO, the presence or absence of the bottom chain shouldn't affect that decision. Consequently, absent a plan for removing "unknown issuer" cert error overrides in general (which I think is a very good idea), I think it is OK to allow the cert error override for the ERROR_V1_CERT_USED_AS_CA case.

To be clear, I disagree with Johnathan that the cert error override mechanism that Firefox and other browsers have is sufficient for protecting users. However, absent a plan to actually solve that general problem, simpler solutions to this problem (i.e. the one that Kai already implemented) are better than more complication solutions that have negative performance consequences.

Comment 148

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

The determination that a certificate does not come from a root in our program doesn't have to be implemented in mozilla::pkix. (I already tried, and you're right - it introduces needless complexity.) If the code that calls mozilla::pkix sees that a trusted chain couldn't be built, it could do a simple path traversal by issuer. If it can't find a path that ends in a trust anchor, we can guess that the certificate isn't part of the PKI that we have requirements for, and therefore the most we can say about it is ERROR_UNKNOWN_ISSUER (which is overridable). Note that since we're already on an error path in this case, speed isn't particularly important.

Comment 149

[nelsonjfr]

I am not programmer in this project, but it seems to me you are trying to protect the many while being pesky with the few :-)
In particular I do no care if I need to go to about:config and change a setting to allow me access to that site I know is safe. Chrome allows me in. There are plenty of issues with intranets and Firefox, all revolving around certificates. People around me is ditching Firefox because of it.

Comment 150

[mobilesvenvd]

I too have this problem. I can't for example access HP ILO anymore and other intranet sites. I just want an option to override regardless of why a certificate or the chain was bad.

Comment 151

[f0rhum]

I added a case here : https://bugzilla.mozilla.org/show_bug.cgi?id=1084606 (FF33/34beta/Aurora35 Windows NT 5.1)

Comment 152

[me.moz]

FYI, I see a rise of complain comments on my blog post (http://blog.dob.sk/2014/07/23/firefox-31-self-signed-certificate-sec_error_ca_cert_invalid/) regarding latest Firefox 33 handling of certificates - mostly from admin guys (usually saying they have to switch to IE !)

Are you really sure that planning this fix for release 36 is soon enough ?

Comment 153

[Kai Engert (:KaiE:)]

Comment on attachment 8502916 [details] [diff] [review]
Patch v6 for Mozilla 33

This patch should work on Firefox 33.x, if you want to take it.

Comment 154

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8502916 [details] [diff] [review]
Patch v6 for Mozilla 33

OK. I do think that it is a pain for users and we received a lot of complains on this. AFAIK, the patch that we took in esr31 didn't caused any regression.
I am taking it for 33.1.

Comment 155

[Sylvestre Ledru [:Sylvestre]]

David, do you think we should take your patch for aurora & beta or Kai's hotfix? Thanks

Comment 156

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Since that patch didn't appear to cause problems on ESR and since we're taking it on release, let's go ahead and take it for aurora and beta as well. I'll pursue the scheme outlined in comment 137 in another bug.

Comment 157

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/efd4bca5ac0d

Comment 158

[Kai Engert (:KaiE:)]

Attachment: Patch v6 merged to Mozilla 34/35 (applies on both branches)

Comment 159

[Kai Engert (:KaiE:)]

Comment on attachment 8513661 [details] [diff] [review]
Patch v6 merged to Mozilla 34/35 (applies on both branches)

Approval Request Comment
[Feature/regressing bug #]: ???

[User impact if declined]:
Many people will quit using Firefox, and use other browsers, because Firefox no longer allows them to manage their equipment with built-in unchangable certificates.
See the complaints in the blog post mentioned in e.g. comment 152.

[Describe test coverage new/current, TBPL]:
Same patch has worked on Firefox 31.2 ESR without known regressions.
Only manual testing. Full test instructions available, see comment 62.

[Risks and why]:
Very little risk. Patch is very small, only adds scenarios to existing code. It allows the user to take the "override" code path, that's already being executed for many other scenarios (and was used in the past for this scenario, too).

[String/UUID change made/needed]: none

Comment 160

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8513661 [details] [diff] [review]
Patch v6 merged to Mozilla 34/35 (applies on both branches)

As noted, we've already shipped this in ESR and taken in on release. 

Beta+
Aurora+

Comment 161

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1472a5e80ec7
https://hg.mozilla.org/releases/mozilla-beta/rev/a03fc45643ef

Comment 162

[xattr]

Just noticed I cant access HP ILO sites anymore with FF33 because the workaround for FF31 doesnt work anymore.... LOL!! This is such a joke! incredible! This realy makes me mad. MAD!! This browser is getting worse with every version! To whoever at mozilla invented this "feature": WHAT THE **** ARE YOU SMOKING!?!?!?! you know what? SHOVE YOUR BROWSER UP YOUR BUM!!! Im done with this!!! For admin stuff I use chrome now, even that thing has an override button. you guys realy failed on this. good bye!

Comment 163

[SuD]

I confirm that ESR-31.2.0 allows override on HP ILO sites. Thank you so much for understanding our needs.

Comment 164

[Sylvestre Ledru [:Sylvestre]]

(In reply to SuD from comment #163)
> I confirm that ESR-31.2.0 allows override on HP ILO sites. Thank you so much
> for understanding our needs.
Welcome. 33.1 will have the fix too.

Comment 165

[kaka5mail]

hope fix it soon.    This's so inconvenient

Comment 166

[will69]

Thanks for fixing this in Firefox Beta. We can use HP ILO again with Firefox Beta, which we prefer to FF ESR or MS IE.

It doesn't work in Firefox Nightly, though. Is this expected or are there more problems down the road?

Comment 167

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to will69 from comment #166)
> Thanks for fixing this in Firefox Beta. We can use HP ILO again with Firefox
> Beta, which we prefer to FF ESR or MS IE.
> 
> It doesn't work in Firefox Nightly, though. Is this expected or are there
> more problems down the road?

Bug 1091778 is a more general solution to this problem, and will hopefully be fixed this cycle.

Comment 169

[kaka5mail]

Since upgrade to firefox 37.   It's happened agin.   
Out company  inner site  with slef certy CA , cannot be open . and It's error code is sec_error_ca_cert_invalid

It's should reopen!

Comment 170

[kaka5mail]

sorry, It's  firefox 36.0 Mac os

Comment 171

[Kai Engert (:KaiE:)]

Thanks for your report. Because this version tracked the issue in older versions of Firefox, I've filed a new bug report, bug 1138332, to track the regression in Firefox 36. Please let's discuss the Firefox 36 issue in bug 1138332.

Comment 172

[Guillaume Beaudoin]

This bug is reappearing in the recent Firefox 36 and 36.0.1 for me. I'm stuck with a certficiation error without any options.

Comment 173

[Matt Wobensmith [:mwobensmith][:matt:]]

Guillaume - do you have a URL for us to look at?

Comment 174

[Guillaume Beaudoin]

Hello Matt,

The certificate itself is in bug 1140090. The web site itself is internal and unavailable from the Internet.

Much like comment 8, it's a proprietary software that self generate a self sign certificate. This time it's Avaya Configuration and Orchestration Manager (http://www.avaya.com/ca-en/product/configuration-and-orchestration-manager/).

Comment 175

[Kai Engert (:KaiE:)]

I've updated bug 1140090 with text dumps of the certificates.

It's probably the lack of the basic constraint extension.

This is what I have described in new bug 1138332, as I said in comment 171.

Comment 176

[Guillaume Beaudoin]

Thanks you Kai, bug 1140090 is probably a duplicate of bug 1138332 then. I wrongly assumed that since it was self-sign it was a different scenario. My bad for the extra works to everyone involved.