1589883 - The new about:certificates incorrectly reports the OCSP Must-Staple extension

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[Eddie Carswell]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

1. Browse to a site with the OCSP Must-Staple extension in the x509 certificate.
2. Check the site's certificate in about:certificates, and look for the OCSP Stapling section.

Example: Compare the about:certificates view of dm4productions.com to the parsed certificate from Censys.

Actual results:

The about:certificates page incorrectly reports that OCSP stapling is not required.

Expected results:

The about:certificates page should report that OCSP stapling is required if the tls_feature extension is present with the value of status_request (Unparsed extension: 1.3.6.1.5.5.7.1.24 = DER:30:03:02:01:05).

Comment 1

[Johann Hofmann [:johannh]]

April, can you take a look at this? Is this different from Certainly Something?

Comment 2

[April King [:April]]

Certainly Something correctly shows that OCSP Stapling is required on dm4productions.com. It's a regression from my code.

Comment 3

[Johann Hofmann [:johannh]]

Carolina, is this something you'd have time to look into?

Comment 4

[Carolina Jimenez Gomez]

Hey, I could take a look after November 4, is that OK? If yes, please assign this bug to me!

Comment 5

[Johann Hofmann [:johannh]]

Hey, that seems okay, considering that nobody else is likely to have time for it. Considering that Certainly Something supports it, it would be great if we could manage an uplift into 71.

Comment 6

[Carolina Jimenez Gomez]

Taking a look right now! I'll try to get a patch for this asap.

Comment 7

[Carolina Jimenez Gomez]

Attachment: Bug 1589883 - Fixes OCSP section.r=johannh

Comment 8

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]: Correctness bug in new security UI feature rolling out in 71

Comment 9

[Carolina Jimenez Gomez]

I see the keyword checkin-needed is not available, hope checkin-needed-tb does the same!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bae5c13eea9
Fixes OCSP section.r=johannh

Comment 11

[Natalia Csoregi [:nataliaCs]]

(In reply to Carolina Jimenez Gomez from comment #9)

I see the keyword checkin-needed is not available, hope checkin-needed-tb does the same!

Hi Carolina! The checkin-needed tag is no longer available in Bugzilla.
If you would like to have a patch landed, please put the tag directly in Phabricator- from Edit Revision put Check-in Needed in the Tag field.

Comment 12

[Carolina Jimenez Gomez]

(In reply to Natalia Csoregi [:nataliaCs] from comment #11)

Hi Carolina! The checkin-needed tag is no longer available in Bugzilla.
If you would like to have a patch landed, please put the tag directly in Phabricator- from Edit Revision put Check-in Needed in the Tag field.

Thank you! Do you know how can I nominate the patch for beta uplift? Johann asked me to do so but I don't really know how (I don't see the option in the tags) and he is on PTO until December

Thank you!

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1bae5c13eea9

Comment 14

[Carolina Jimenez Gomez]

Comment on attachment 9106337 [details]
Bug 1589883 - Fixes OCSP section.r=johannh

Beta/Release Uplift Approval Request

• User impact if declined: The certificate viewer was reporting a wrong OCSP
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I think this patch is not risky since I changed an if statement
• String changes made/needed: None.

Comment 15

[Carolina Jimenez Gomez]

Apologies for the "I don't know" responses, is my first time nominating a bug for beta uplift and really don't know how to verify those

Comment 16

[:Gijs (he/him)]

(In reply to Carolina Jimenez Gomez from comment #15)

Apologies for the "I don't know" responses, is my first time nominating a bug for beta uplift and really don't know how to verify those

No worries, I took the liberty of editing your response for you -

Has the fix been verified in Nightly?

This would be true if someone from QE had tested that the fix worked on Nightly. Nobody has done so here (or there'd be comments and the bug status would have been changed to "VERIFIED" instead of "RESOLVED".

Needs manual test from QE?

Because there are automated tests, I don't think this needs additional manual testing.

String changes made/needed

This question is about whether you modified localizable strings (in .dtd, .properties or .ftl files). Your patch didn't do that, so "No" is the right answer. The question is here because if we change localizable strings, that creates work for localizers, and we try to avoid doing that while things are already in beta. In this case, although you changed when some strings get displayed, there's no work for localizers because the actual strings themselves did not change.

Comment 17

[Carolina Jimenez Gomez]

Thank you very much!!!

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9106337 [details]
Bug 1589883 - Fixes OCSP section.r=johannh

P1 fix for an upcoming 71 feature, patch has tests and looks safe, uplift approved for 71 beta 9, thanks.

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8ffcf4d49c7