Closed Bug 1589883 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Security, defect, P1)

Desktop
All
defect

Tracking

()

RESOLVED FIXED
Firefox 72
Tracking Status
firefox70 --- disabled
firefox71 + fixed
firefox72 --- fixed

People

(Reporter: eddiecarswell13, Assigned: carolina.jimenez.g)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

Component: Untriaged → Security
OS: Unspecified → All
Hardware: Unspecified → Desktop

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

Blocks: cert-viewer
Flags: needinfo?(april)

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

Flags: needinfo?(april)

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

Flags: needinfo?(carolina.jimenez.g)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2

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

Flags: needinfo?(carolina.jimenez.g)

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.

Assignee: nobody → carolina.jimenez.g
Status: NEW → ASSIGNED

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

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

Priority: P2 → P1

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

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

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72

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.
Attachment #9106337 - Flags: approval-mozilla-beta?

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

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

Thank you very much!!!

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.

Attachment #9106337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: