HSTS Not Clearing on Shutdown since FF63

VERIFIED FIXED in Firefox 66

Status

()

defect
P1
normal
VERIFIED FIXED
6 months ago
6 months ago

People

(Reporter: h6sjml+esk3z2w69qt00, Assigned: keeler)

Tracking

({regression})

65 Branch
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 wontfix, firefox66+ verified, firefox67 verified)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (iPad; CPU OS 12_1_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1

Steps to reproduce:

  1. Enable clear history on shutdown with every option checked (including site settings).

  2. Shutdown browser.

Actual results:

SiteSecurityServiceState.txt not clearing.

Note: The file is cleared if sanitization used while browser is running. (ctrl-shift-del).

Expected results:

HSTS entries stored in SiteSecurityServiceState.txt should be cleared on shutdown.

Component: Untriaged → Data Sanitization
Product: Firefox → Toolkit

Strange, I can reproduce. No idea what's going wrong here. We should figure it out.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1252f83a5b4222f493fb979058098e4f3dd09bd8&tochange=7afeaceba53d9c52a0f0319a3d44f1ac6e5b36c3

Regressed by:
7afeaceba53d David Keeler — bug 1470918 - use only one thread for all DataStorage instances r=franziskus,froydnj

:keeler,
Your patch seems to cause the regression.
Can you please look into this?

Blocks: 1470918
Flags: needinfo?(dkeeler)

I have a solution but it is too large to fit in the margin (I'll get a patch together hopefully tomorrow).

Assignee: nobody → dkeeler
Component: Data Sanitization → Security: PSM
Flags: needinfo?(dkeeler)
Priority: P2 → P1
Product: Toolkit → Core
Whiteboard: [psm-assigned]

In bug 1470918, a shared thread was introduced that did the reading/writing work
for all DataStorage instances. To ensure all state was written out at shutdown,
the original patch implemented a two stage strategy that in the first stage
queued an event for each DataStorage that would write out its contents and then
in the second stage ran these events to completion. This conflicted with how
Firefox's sanitization implementation would make sure that data was cleared at
shutdown, if requested by the user. If the DataStorage objects observed the
first shutdown notification before the sanitization implementation, the existing
data would be queued for writing and any further updates would be prevented.
Thus, when the sanitizer tried to clear data held by DataStorage instances,
nothing would happen and the data would be written to disk.

This patch fixes this issue by implementing DataStorage shutdown in a single
stage that can run after sanitization. Since sDataStorages knows of the
existence of every DataStorage, we can simply iterate through its entries, queue
write events for each one, and then run those events to completion in one pass.

This patch also fixes bug 1528019 (gDataStorageSharedThread could leak if
NS_NewNamedThread failed) because the fix is trivial and not worth handling in
an entirely separate bug.

Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ee7c36ed3ac
write out and shut down DataStorage all at once so that clearing on shutdown works properly r=Ehsan
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Dana, want to request uplift for this to beta? Or do you think it may be too risky?

Flags: needinfo?(dkeeler)
Duplicate of this bug: 1528019

I think this is probably safe enough to uplift. It might be a good idea to await the outcome of bug 1529202, though, since it looks like the test doesn't work when Thunderbird runs it.

Depends on: 1529202
Flags: needinfo?(dkeeler)

OK, thanks, I'll track for now and keep an eye on it.

Comment on attachment 9044351 [details]
bug 1527372 - write out and shut down DataStorage all at once so that clearing on shutdown works properly r?ehsan

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1470918
  • User impact if declined: If a user has Firefox configured to clear site state on shutdown, accumulated HSTS and HPKP data won't be cleared (this includes domain names the user has visited).
  • 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: Bug 1529202
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The fix is fairly localized and is kind of a "oh that makes sense and is probably what we should have done in the first place" fix, but this is a tricky area of code and we've been wrong before. (The other bug to uplift is a test-only change.)
  • String changes made/needed: none
Attachment #9044351 - Flags: approval-mozilla-beta?

When you uplift this to beta, please also uplift bug 1529202 which switches the new test off for Thunderbird. That's "test-only" approval. Thanks in advance.

Comment on attachment 9044351 [details]
bug 1527372 - write out and shut down DataStorage all at once so that clearing on shutdown works properly r?ehsan

Fix for potential privacy issue, may be a bit risky but let's take it for beta 12.
This sounds like something we could verify in nightly, as well.

Attachment #9044351 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [psm-assigned] → [psm-assigned][qa-triaged]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:67.0) Gecko/20100101 Firefox/67.0
Build ID: 20190225215823

Verified as fixed on the latest Nightly build.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:66.0) Gecko/20100101 Firefox/66.0
Build ID: 20190228180200

Verified as fixed on the latest Beta build (66b12).

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [psm-assigned][qa-triaged] → [psm-assigned]
You need to log in before you can comment on or make changes to this bug.