1527372 - HSTS Not Clearing on Shutdown since FF63

Status: VERIFIED FIXED

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

Description

[Josh]

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.

Comment 1

[Johann Hofmann [:johannh]]

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

Comment 2

[Alice0775 White]

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?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1527372 - write out and shut down DataStorage all at once so that clearing on shutdown works properly r?ehsan

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2ee7c36ed3ac

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 12

[Jorg K (CEST = GMT+2)]

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 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 14

[Cristian Craciun]

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.

Comment 15

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b07b355d59f

Comment 16

[Cristian Craciun]

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