Closed Bug 1519209 Opened 5 years ago Closed 5 years ago

Disable NSS_ALLOW_SSLKEYLOGFILE

Categories

(Firefox :: Security, defect)

65 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- fixed
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file)

Due to potential shutdown hangs we'd like to temporarily disable NSS_ALLOW_SSLKEYLOGFILE [1] by default in non-nightly releases.

[1] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/security/nss/lib/ssl/ssl.gyp#97

This disables NSS_ALLOW_SSLKEYLOGFILE in beta in release in order to avoid shutdown hangs until the NSS project has time to fix the root cause of the issue.

Nathan can you review this? It's really a build system change at this point. I have a [beta simulation running](https://treeherder.mozilla.org/#/jobs?repo=try&revision=837654f11970c3269bcf0d08ec8a68d3c30e94f5) to make sure it does the right thing.
Attachment #9035763 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

Review of attachment 9035763 [details] [diff] [review]:
-----------------------------------------------------------------

Note that we're trying to do this without changing NSS at all.  The Right Thing in some respect is to make this setting configurable at the NSS level, but since we're not taking that route (for now?), this was the best thing I could think of.  glandium might have strong opinions about this, and I thought up the idea, so I don't think I should be the person to review this.
Attachment #9035763 - Flags: review?(nfroyd) → review?(mh+mozilla)
Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

Review of attachment 9035763 [details] [diff] [review]:
-----------------------------------------------------------------

Considering how things work for defines and gyp, this might well be the only solution without touching NSS.

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +250,5 @@
>                  if '=' in define:
>                      name, value = define.split('=', 1)
> +                    # The NSS gyp file doesn't expose a way to override this
> +                    # currently, so we do so here.
> +                    if name == 'NSS_ALLOW_SSLKEYLOGFILE' and config.substs.get('RELEASE_OR_BETA', False):

You don't need that `, False`.
Attachment #9035763 - Flags: review?(mh+mozilla) → review+
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0801165e3175
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release. r=glandium

Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1188657

User impact if declined: This can cause random hangs in the networking threads when working with secure connections. This particularly shows up as shutdown hangs in crash stats.

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): We're just disabling the option to enable a feature which works around a bug in NSS related to checking if the feature is enabled.

String changes made/needed: N/A

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This disables a feature that can cause networking and shutdown hangs.

User impact if declined: This can cause random hangs in the networking threads when working with secure connections. This particularly shows up as shutdown hangs in crash stats.

Fix Landed on Version: 66

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We're just disabling the option to enable a feature which works around a bug in NSS related to checking if the feature is enabled.

String or UUID changes made by this patch: N/A

Attachment #9035763 - Flags: approval-mozilla-esr60?
Attachment #9035763 - Flags: approval-mozilla-beta?

(In reply to Eric Rahm [:erahm] from comment #1)

I have a [beta simulation
running](https://treeherder.mozilla.org/#/
jobs?repo=try&revision=837654f11970c3269bcf0d08ec8a68d3c30e94f5) to make
sure it does the right thing.

I've verified that the SSLKEYLOGFILE string used to check if an env var is set is no longer present in a simulated beta build by running:

strings libssl3.so | grep SSLKEYLOGFILE
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

[Triage Comment]
Enables NSS_ALLOW_SSLKEYLOGFILE on Nightly builds only to avoid shutdown hangs. Approved for 65.0b11 and 60.5.0esr.

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