Closed Bug 1601035 Opened 4 years ago Closed 4 years ago

new certificate viewer doesn't handle certificates with an AuthorityKeyIdentifier without a keyIdentifier

Categories

(Firefox :: Security, defect, P2)

Product:
Firefox
Component:
Security
Version:
71 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 74
Tracking Flags:
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)

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.
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:

  1. Visit an HTTPS site with a certificate not issued by a builtin CA, for instance https://mail.tghost.co.uk/.

  2. 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.

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.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Security: PSM
Product: Firefox → Core

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.

Component: Security: PSM → Security
Product: Core → Firefox
Summary: New Certificate Viewer can't show certificates from non-builtin issuers → new certificate viewer doesn't handle certificates with an AuthorityKeyIdentifier without a keyIdentifier
Blocks: cert-viewer
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Attached patch bug_1601035.patch (obsolete) — — Splinter Review

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.

That patch solves it for me with the few example websites I have tested. Thanks very much.

Flags: needinfo?(jhofmann)

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!

Flags: needinfo?(jhofmann) → needinfo?(camporter1)

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.

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.

Flags: needinfo?(camporter1) → needinfo?(jhofmann)
Assignee: nobody → camporter1
Status: NEW → ASSIGNED

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.

Flags: needinfo?(jhofmann) → needinfo?(camporter1)

(Happy to help if you have any questions)

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!

Flags: needinfo?(camporter1)
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

https://hg.mozilla.org/mozilla-central/rev/b7fdc1dd49d6

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

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
Attachment #9119561 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Attachment #9118259 - Attachment is obsolete: true
QA Whiteboard: [qa-triaged]

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.

Attachment #9119561 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Status: RESOLVED → VERIFIED
status-firefox73: fixed → verified
status-firefox74: fixed → verified
Flags: qe-verify+

Out of curiosity, why is this marked WONTFIX for firefox72? Isn't this a somewhat serious regression, not being able to verify certificates?

(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.

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.

You need to log in before you can comment on or make changes to this bug.