new certificate viewer doesn't handle certificates with an AuthorityKeyIdentifier without a keyIdentifier
Categories
(Firefox :: Security, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox72 | --- | wontfix |
firefox73 | --- | verified |
firefox74 | --- | verified |
People
(Reporter: luke, Assigned: camporter1)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0
Steps to reproduce:
-
Visit an HTTPS site with a certificate not issued by a builtin CA, for instance https://mail.tghost.co.uk/.
-
Try to view the certificate using the new certificate viewer in Firefox 71.
Note 1: The same behaviour happens whether or not the issuer is currently trusted by adding the issuer certificate to the certificate store.
Note 2: Since this stops you viewing the certificate on an "untrusted certificate!" warning page, I consider to be to security flaw.
Actual results:
The about:certificate tab is opened, but it contains:
"Certificate
Something went wrong.
We were unable to find the certificate information, or the certificate is corrupted. Please try again."
Expected results:
The certificate and its issuer should have been shown.
Reporter | ||
Comment 1•4 years ago
|
||
I forgot to add, the address of the about:certificate tab from the example above (https://mail.tghost.co.uk/) looks valid and just like the other sites which work fine:
about:certificate?cert=MIIGdjCCBF6gAwIBAgIBJjANBgkqhkiG9w0BAQsFADB0MQswCQYDVQQGEwJKRTEPMA0GA1UECAwGSmVyc2V5MRUwEwYDVQQKDAx0Z2hvc3QuY28udWsxFTATBgNVBAMMDHRnaG9zdC5jby51azEmMCQGCSqGSIb3DQEJARYXcG9zdG1hc3RlckB0Z2hvc3QuY28udWswHhcNMTkxMTE4MjE1NjAwWhcNMjAxMTE3MjE1NjAwWjBRMQswCQYDVQQGEwJKRTEPMA0GA1UECAwGSmVyc2V5MRUwEwYDVQQKDAx0Z2hvc3QuY28udWsxGjAYBgNVBAMMEW1haWwudGdob3N0LmNvLnVrMIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEA0CS0dR6hyjv4o6s4%2FI2VT43DCfylcaq%2F6%2B5tUn6xMDtf0tYMXPjG%2FGjtlXQk2Jq7wa%2FlRR%2Fzs8c6037BZ7wzu8Wx2i6tvRLZZSlzM2mRHxaRRDYTeHyovanhCFzKqnvi30bxKugQ3lETBK5eP7ppVpcHz58lx%2B%2BFovPOBx6SRaz4EiceUybZwFpKxUJmlhivT2MJTKM5NHF4n27vszM0dyu2qXLzhpy5dgklMEEhdnHFCT%2F9YPN7uLCvzJnCo9N%2FeWf84odJ%2FjTiEKm5NG6SGDuOZ%2FbY9KwQ8nAk26zeTkw%2BMfMXD2Rl76ZFg%2B07bpckSTiaCtLGGhbgLrqEdgNGTpz1Pu3uFjhR4QsfUsDAaSvgns0PT6eSA152cgOtin2SYo%2B4UVjVdM5d%2ByB0JgEIGHmiCRQkDdCD9%2FNZBhrnG2Xwooe64QzD5OjNS8yQSCPvjfivbSk1ulcIGjgxC0z1Ot4hc%2ByuU8aRE0ekIOf5EUEjZfJ%2FnO4EXT3J5en0CYurswJM7miOsVEiO1OIut022T00MoAF%2F%2FWP73qVW3Jbv6X0pzOBYDY3osyje4n6JRqujhPEOR0MNiaZxfz4JFNPAzi0Tq6qORbXbLgGBqPFvKh2KKIMGysfRqLlETI1lpqsHyhMxgllQW9IK33kwK9y0HgluEZvc58dvle8hOS4NK8CAwEAAaOCATQwggEwMAkGA1UdEwQCMAAwHQYDVR0OBBYEFORJOggtlruQ1S6vzucFiAS86CadMIGQBgNVHSMEgYgwgYWheKR2MHQxCzAJBgNVBAYTAkpFMQ8wDQYDVQQIDAZKZXJzZXkxFTATBgNVBAoMDHRnaG9zdC5jby51azEVMBMGA1UEAwwMdGdob3N0LmNvLnVrMSYwJAYJKoZIhvcNAQkBFhdwb3N0bWFzdGVyQHRnaG9zdC5jby51a4IJAMeNizC6cTabMDMGA1UdHwQsMCowKKAmoCSGImh0dHA6Ly93d3cudGdob3N0LmNvLnVrL2NhL2NybC5wZW0wCwYDVR0PBAQDAgP4MBEGCWCGSAGG%2BEIBAQQEAwIE8DAcBgNVHREEFTATghFtYWlsLnRnaG9zdC5jby51azANBgkqhkiG9w0BAQsFAAOCAgEAKZC9pqgU9MPssbNeJhKeGcCoH49BhfhrJNPkWqlJ%2FVbUB32m8p%2F7exaqFEe%2Fk2YwV8ILMA%2FCvQLgeV36Y9rHcWKqaA8lrklhRkK6%2BfYkAkpw37vVbm8qWsTt4OMa0Ajh%2F7QBy%2FUqak8Gaarl3LkJKLKB3Pj%2Bd2BFNxNm5W3TeFia2s%2B5y0O0eKqSQwRPqlQo3qhNQU1fDimTq8yI3fH9AOByWXHePjSc5h45GHxWoXPqQKtuqCINou6pKeZa1KoTCefB2oYJqamkneL2%2B21JhwkSVkGUfseGc2hiROVLejc%2Fgzq7mpUqudOsyN672BwB6W81w6%2FHCt2cZmCTWAAT5E3b6bHqqTTn9O2uljx2zTxao5Xov4L9EZ3hdccsLJGXHYgGDqY33e6Rrf86Jc%2BV%2F4%2FL8vhGf%2B70ASKLkibUuGwFJ0%2BamQHoHnVXLZ5rnbV3ArXOQcsfegTifmEPn7pGyHm7r5%2BkJ2JIh8wCyl5B%2FUxvNwIfAFEiieU3aqRAgbjnezYXwoLoLJBKSRMXdIfZJldduIkoPaeO0zWWaJSMon3qTAeDuboM4EfYG9Ma3UWdznXVkmgiXFo%2Fz6k%2Bg4jNzaQknI8jNHbp9G5K7M9KPcvUja8hjeGG4RIULKDdkauZUe1hX6m3NrzrJLD3MJPD8YTFeAf%2Fe3HCGwl4stKWHg4%3D&cert=MIIFZDCCA0wCCQDHjYswunE2mzANBgkqhkiG9w0BAQsFADB0MQswCQYDVQQGEwJKRTEPMA0GA1UECAwGSmVyc2V5MRUwEwYDVQQKDAx0Z2hvc3QuY28udWsxFTATBgNVBAMMDHRnaG9zdC5jby51azEmMCQGCSqGSIb3DQEJARYXcG9zdG1hc3RlckB0Z2hvc3QuY28udWswHhcNMTgwNDA4MTIyMjU0WhcNMjMwNDA4MTIyMjU0WjB0MQswCQYDVQQGEwJKRTEPMA0GA1UECAwGSmVyc2V5MRUwEwYDVQQKDAx0Z2hvc3QuY28udWsxFTATBgNVBAMMDHRnaG9zdC5jby51azEmMCQGCSqGSIb3DQEJARYXcG9zdG1hc3RlckB0Z2hvc3QuY28udWswggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQCoFbHy44GNOQSymk%2BwAmZmllcAATSM6DrsW7NZgX8vHeP5OAlRaGaOgVmozKcbKSB2KSIvDvY1%2BKEKtfr5meIaeA6r%2BILeUYuNxQomnW3Oy%2B%2B7RFE53pS7R7QIt%2FLVf3k60mdZDLXF1wyFv950noGNrFZfvBvND1%2F88JeC4Gt7dVnJMy9CXOr7V0O%2F7sa5z%2Bvfm2Acufd1RYA3W%2FECbt%2F6z8KV%2FQthAgcYoabQgkmh8AxQ5cG7tBmoEAs%2BNwU%2BR8eMq1f%2B6TBCJiMa07jA1%2BQ663d7YONlk4fTSmvo3lEhXtrR%2BmrkxZAMiAD6NJs3VakcuAED9e1dS3G3%2FmPcNrKsa73aXEC5uvL9y4mKCZLypuSYjlW8cP5JpCG6vtmwL%2FATbcCtwpDwrKLcRWCrZdX0FOJirjWfKL%2BhyIWayQ5u7awdhH430h8rj03fXSMBhtTn73rohSGIANBnqVftvDDPnEQb5p17giaVbJybfA2tdMvJTZg6pEAx9%2B3zHXRECFpWOtaJDjWbmIExPMgiCSFkBPP3MEDVb7VdHKZs0CvBQg6GmpXg9M4OHZylzS6PXXO8qBCtBdKOwdFSZWbeD9E652rkvb5FuQ8laGHcxgMc2YFlVZaqVowXEkVkkld12d4sBipeRpDWNjA2kLfeUmwsXWtugG8c8Jm9IoZY8oeUkwIDAQABMA0GCSqGSIb3DQEBCwUAA4ICAQCVj6R9C5pE9hkBx7CCr9l4TLoolaEZlKngJHBRRSy5U50MndQ24Jeis2OTxEeYX7DJ%2BOy6QZdaph6FLwALxUWPfQavBcgauKR%2FCdqLQxZc267TMFkRIZvOtHUJ4obJ5qvQAWzNnmRq18b5plznfBfn9b3F2AmHgfsNX6WryQ4geR786VObcUhKTaofcaX%2FE4J9AVzL864fJ%2FDB%2FPSqhYrtskiG6PT9Ngu20ejvgAVk5aJ%2F%2F5Swsjyg6EBl%2BdwJepq2xzI313ZkwCg2pp6oqLDlxf%2Fh585Bsqke39Ax%2B42NWNM14cethcJ7jgyZoEaL3T%2BQ3O%2Bj9XEe5FbvkKtSgMIA6f0FneA4vbF8%2FECPMEi7k3k04gU1gZSR%2FGA63GPEK%2FCLqeh%2BYVgBGKDPinnMEV%2FuBrTUDQFBlDEHlELk7F9o93lxQNs6WOGQSfybfHhySla3nqoYV2phyeZZsJ%2BxMovWWIydJhVN%2F37v0fYTZhnlquGenXHvC5MHrMq2vjUY0Uy637AzXWVNWveNyQmmn9tL4jygELHQu%2Bint4GRybH7rThcslv63D5s4zZza7cITIFjmqZ%2BPrTSVKL%2BlnQLjEMpSdfB86Y2JnMimaOyv%2BbZp1QMB5fYh0PTsjYATi9We%2BDFD42c5H7Su0vFAwCTP11AiKzfbj2oBKVVwwJj3FmkYQ%3D%3D
That is the correct certificate chain, and OpenSSL will happily decode both of them once split up, URL decoded, and formatted as PEM files.
Comment 2•4 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
With a bit of logging added in, I get TypeError: "aKID.keyIdentifier is undefined"
from certDecoder.js:225
. Looks like RFC 5280 says the keyIdentifier
field of AuthorityKeyIdentifier
is optional, so we can't rely on it being defined.
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
I was running into this issue recently, so I took a quick stab at a patch that might help with specific cases where certificates are either missing a keyIdentifier
, or are missing extensions
altogether. It seems like PKI.js would include the default of an empty array for extensions, but doesn't when returned from toJSON
. I'm a newcomer, so feel free to ignore/change the patch.
Reporter | ||
Comment 5•4 years ago
|
||
That patch solves it for me with the few example websites I have tested. Thanks very much.
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Hi camporter1, thank you for the patch, that seems good to me. Would you like to submit the patch for "proper" review in Phabricator, our code review tool? If so, please follow this set-up guide https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
If you don't have time for that please let me know and I'll submit it :)
Thanks!
Assignee | ||
Comment 8•4 years ago
|
||
When viewing certificates with certificateviewer that have an authority x509 extension
but do not have an authority key identifier, parsing the certificate fails.
Since the authority key identifier is optional in RFC 5280,
check if it exists before trying to set the key identifier.
Also check that x509 extensions are defined before setting up the critical and unsupported extensions.
Assignee | ||
Comment 9•4 years ago
|
||
Hello :johannh, thank you for the info and patience. I've opened a phabricator review, but I'm most likely missing some things on it.
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Thank you for the patch, would you be up for writing a new test that ensures that we're not regressing this again? It should be fairly straightforward, you can take on of the existing tests, e.g. https://searchfox.org/mozilla-central/source/toolkit/components/certviewer/tests/browser/browser_checkOCSP.js and change the about:certficate URL to one that was affected by this bug, then maybe simplify the assertions to only ensure that we're correctly loading the certificate.
The test can go in the same patch or a new patch, as you prefer.
Comment 11•4 years ago
|
||
(Happy to help if you have any questions)
Assignee | ||
Comment 12•4 years ago
|
||
Updated with tests around the authority key section and making sure that the cert viewer still loads other sections if no authority key is set. Let me know if there's anything that doesn't look right!
Comment 13•4 years ago
|
||
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b7fdc1dd49d6 Check for the key identifier in an authority x509 extension before trying to use it since it is optional for some certificates. Check that there are x509 extensions before trying to get critical or unsupported extensions. r=johannh
Comment 14•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Comment on attachment 9119561 [details]
Bug 1601035 - Check for the key identifier in an authority x509 extension before trying to use it since it is optional for some certificates. Check that there are x509 extensions before trying to get critical or unsupported extensions.
Beta/Release Uplift Approval Request
- User impact if declined: about:certificate is not correctly displaying some certificates
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: See comment 0
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Simple patch that guards on a field being undefined, has tests
- String changes made/needed: None
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 16•4 years ago
|
||
Comment on attachment 9119561 [details]
Bug 1601035 - Check for the key identifier in an authority x509 extension before trying to use it since it is optional for some certificates. Check that there are x509 extensions before trying to get critical or unsupported extensions.
Fixes some situations where about:certificate shows incorrect information. Approved for 73.0b5.
Comment 17•4 years ago
|
||
bugherder uplift |
Comment 18•4 years ago
|
||
I was able to reproduce this bug using the steps from comment 0, on an affected build (Beta 73.0b4).
The issue is verified as fixed on latest Nightly 74.0a1, Beta 73.0b5 across OSes: Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64.
Comment 19•4 years ago
|
||
Out of curiosity, why is this marked WONTFIX for firefox72? Isn't this a somewhat serious regression, not being able to verify certificates?
Comment 20•4 years ago
|
||
(In reply to Christian Kujau from comment #19)
Out of curiosity, why is this marked WONTFIX for firefox72? Isn't this a somewhat serious regression, not being able to verify certificates?
It only applies to some certificates and 73 is going out to release very soon. This is not worth making a dot release for, in our opinion.
Comment 21•4 years ago
|
||
I created my self-signed SSL certificate using default options of openssl, and was hit with this bug. So I would say the "some certificates that are hit with this bug" are actually "most self-signed certificates". And, according to https://wiki.mozilla.org/Release_Management/Calendar Firefox 73 will be released on Feb 11, which is about 20 days from now. Please re-consider shipping the fix sooner. Thanks.
Description
•