Closed Bug 1434831 Opened 7 years ago Closed 7 years ago

Certificate chain is not shown in error pages

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 - wontfix
firefox59 + verified
firefox60 --- verified

People

(Reporter: emk, Assigned: keeler)

References

Details

(Keywords: regression, Whiteboard: [psm-assigned])

Attachments

(2 files)

bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain*
59 bytes, text/x-review-board-request
jcj
: review+
lizzard
: approval-mozilla-beta+
Details
patch for beta
9.09 KB, patch
Details | Diff | Splinter Review 
Steps to reproduce:
1. Browse <https://www.gpki.go.jp/>. SSL error page will be shown.
2. Click "Advanced".
3. Click "SEC_ERROR_UNKNOWN_ISSUER".

Actual result:
Nothing will be shown after "Certificate chain".

Expected result:
Certificate chain should be shown.

This makes troubleshooting nearly impossible in some bugs.
Can someone get a regression window with mozregression for this?
Keywords: regression, regressionwindow-wanted
 8:43.46 INFO: Last good revision: 8fe5c82c3dcd998eaa5f9b038d083abc764931af
 8:43.46 INFO: First bad revision: 51eaba841505a7b5178d6402cbf155ec6e336ce0
 8:43.46 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8fe5c82c3dcd998eaa5f9b038d083abc764931af&tochange=51eaba841505a7b5178d6402cbf155ec6e336ce0

 8:45.02 INFO: Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1406856
Blocks: 1406856
Keywords: regressionwindow-wanted
Mark, any idea what went wrong here? Or maybe we just need to fix the frontend?
Flags: needinfo?(mgoodwin)
In security/manager/ssl/SSLServerCertVerification.cpp:

1488:
   if (rv != Success) {
    // Certificate validation failed; store the peer certificate chain on
    // infoObject so it can be used for error reporting.
    infoObject->SetFailedCertChain(Move(certList));
    PR_SetError(MapResultToPRErrorCode(rv), 0);
  }

`certList` should be `peerCertChain`, probably
Assignee: nobody → dkeeler
Flags: needinfo?(mgoodwin)
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment on attachment 8947605 [details]
bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain*

https://reviewboard.mozilla.org/r/217294/#review223116

Good catch, good test. This probably needs to be uplifted too, I imagine.
Attachment #8947605 - Flags: review?(jjones) → review+
Thanks for the review. Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9efc225ab7cf (except for the l10n builds, but I assume that's an unrelated problem)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9d88b0c947776c30f585303be532864ae95a85dd -d fce4efb62720: rebasing 445460:9d88b0c94777 "bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain* r=jcj" (tip)
merging security/manager/ssl/SSLServerCertVerification.cpp
warning: conflicts while merging security/manager/ssl/SSLServerCertVerification.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0690bf69410a
ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain* r=jcj
https://hg.mozilla.org/mozilla-central/rev/0690bf69410a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8947605 [details]
bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain*

Approval Request Comment
[Feature/Bug causing the regression]: bug 1406856
[User impact if declined]: extra debugging information in certificate error pages won't be available (harder for us to debug what's going wrong for users)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes:
1. go to https://untrusted-root.badssl.com/
2. click "advanced"
3. click "SEC_ERROR_UNKNOWN_ISSUER"
If the bug is fixed, there should be text below "Certificate chain:" like

-----BEGIN CERTIFICATE-----
MIIFSzCCBDOgAwIBAgIQSueVSfqavj8QDxekeOFpCTANBgkqhkiG9w0BAQsFADCB
kDELMAkGA1UEBhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4G
A1UEBxMHU2FsZm9yZDEaMBgGA1UEChMRQ09NT0RPIENBIExpbWl0ZWQxNjA0BgNV
BAMTLUNPTU9ETyBSU0EgRG9tYWluIFZhbGlkYXRpb24gU2VjdXJlIFNlcnZlciBD
QTAeFw0xNTA0MDkwMDAwMDBaFw0xNTA0MTIyMzU5NTlaMFkxITAfBgNVBAsTGERv
bWFpbiBDb250cm9sIFZhbGlkYXRlZDEdMBsGA1UECxMUUG9zaXRpdmVTU0wgV2ls
ZGNhcmQxFTATBgNVBAMUDCouYmFkc3NsLmNvbTCCASIwDQYJKoZIhvcNAQEBBQAD
ggEPADCCAQoCggEBAMIE7PiM7gTCs9hQ1XBYzJMY61yoaEmwIrX5lZ6xKyx2PmzA
S2BMTOqytMAPgLaw+XLJhgL5XEFdEyt/ccRLvOmULlA3pmccYYz2QULFRtMWhyef
dOsKnRFSJiFzbIRMeVXk0WvoBj1IFVKtsyjbqv9u/2CVSndrOfEk0TG23U3AxPxT
uW1CrbV8/q71FdIzSOciccfCFHpsKOo3St/qbLVytH5aohbcabFXRNsKEqveww9H
dFxBIuGa+RuT5q0iBikusbpJHAwnnqP7i/dAcgCskgjZjFeEU4EFy+b+a1SYQCeF
xxC7c3DvaRhBB0VVfPlkPz0sw6l865MaTIbRyoUCAwEAAaOCAdUwggHRMB8GA1Ud
IwQYMBaAFJCvajqUWgvYkOoSVnPfQ7Q6KNrnMB0GA1UdDgQWBBSd7sF7gQs6R2lx
GH0RN5O8pRs/+zAOBgNVHQ8BAf8EBAMCBaAwDAYDVR0TAQH/BAIwADAdBgNVHSUE
FjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwTwYDVR0gBEgwRjA6BgsrBgEEAbIxAQIC
BzArMCkGCCsGAQUFBwIBFh1odHRwczovL3NlY3VyZS5jb21vZG8uY29tL0NQUzAI
BgZngQwBAgEwVAYDVR0fBE0wSzBJoEegRYZDaHR0cDovL2NybC5jb21vZG9jYS5j
b20vQ09NT0RPUlNBRG9tYWluVmFsaWRhdGlvblNlY3VyZVNlcnZlckNBLmNybDCB
hQYIKwYBBQUHAQEEeTB3ME8GCCsGAQUFBzAChkNodHRwOi8vY3J0LmNvbW9kb2Nh
LmNvbS9DT01PRE9SU0FEb21haW5WYWxpZGF0aW9uU2VjdXJlU2VydmVyQ0EuY3J0
MCQGCCsGAQUFBzABhhhodHRwOi8vb2NzcC5jb21vZG9jYS5jb20wIwYDVR0RBBww
GoIMKi5iYWRzc2wuY29tggpiYWRzc2wuY29tMA0GCSqGSIb3DQEBCwUAA4IBAQBq
evHa/wMHcnjFZqFPRkMOXxQhjHUa6zbgH6QQFezaMyV8O7UKxwE4PSf9WNnM6i1p
OXy+l+8L1gtY54x/v7NMHfO3kICmNnwUW+wHLQI+G1tjWxWrAPofOxkt3+IjEBEH
fnJ/4r+3ABuYLyw/zoWaJ4wQIghBK4o+gk783SHGVnRwpDTysUCeK1iiWQ8dSO/r
ET7BSp68ZVVtxqPv1dSWzfGuJ/ekVxQ8lEEFeouhN0fX9X3c+s5vMaKwjOrMEpsi
8TRwz311SotoKQwe6Zaoz7ASH1wq7mcvf71z81oBIgxw+s1F73hczg36TuHvzmWf
RwxPuzZEaFZcVlmtqoq8
-----END CERTIFICATE-----

(it won't be exactly that, but you get the idea)

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's pretty clear what went wrong in the original patch, and the fix is minimal. This patch also adds an additional test.
[String changes made/needed]: none
Attachment #8947605 - Flags: approval-mozilla-beta?
Comment on attachment 8947605 [details]
bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain*

Great, thanks for the new test, fixes an issue with 59.

Do you think this may be worth uplifting to m-r as a ridealong for a 58.0.2 release?
Flags: needinfo?(dkeeler)
Attachment #8947605 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Sure, this would be nice to have as a ridealong.
Flags: needinfo?(dkeeler)
Flags: qe-verify?
Needs a rebased patch for Beta uplift.
Flags: needinfo?(dkeeler)
Attached patch patch for beta — — Splinter Review 
Flags: needinfo?(dkeeler)

    
    

    
  
(the beta patch seems to work on release as well)

    
    

    
  
Ritu, fyi, here is another for your list for potential 58.0.2 issues.

          tracking-firefox58:
          --- → ?

          tracking-firefox59:
          --- → +
Flags: needinfo?(rkothari)

    
    

    
  
This seems more like a patch that gives us more diagnostics. When I decided to gtb 58.0.2, I wasn't convinced this would totally risk-free and ready to be included in 58.0.2. Unless we have data to prove this helps diagnose problems from Nightly/Beta end-users, I'd let this one ride the 59 train to release.
Flags: needinfo?(rkothari)
Flags: qe-verify? → qe-verify+

    
    

    
  
I reproduced this issue on Nightly 60.0a1 (2018-01-31) under Windows 10 x64.

The issue is no longer reproducible on the latest Nightly 60.0a1 (2018-02-09) and Beta 59.0b8, under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32.
Status: RESOLVED → VERIFIED

          status-firefox59:
          fixed → verified

          status-firefox60:
          fixed → verified
Flags: qe-verify+

    
    

    
  
As near as I can tell, it only seems to affected self-signed certs. If I attempt to use it on:

https://wrong.host.badssl.com/

In 58, I'm able to get the complete chain.

    
    

    
  
At this stage I believe this is wontfix for 58, there are no plans for a 58.0.3.

          status-firefox58:
          affected → wontfix

          tracking-firefox58:
          ? → -

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: