1757138 - (CVE-2022-29910) HSTS policy "forgotten" on Firefox for Android (HTTP request in next browsing session does not redirect to HTTPS)

Status: RESOLVED FIXED

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

Description

[mark burdett]

Steps to reproduce:

1. Navigate to http://radio.indymedia.org in Android Firefox 97.2.0 (Build #2015863827)
2. Navigate to https://radio.indymedia.org which has HSTS header (Strict-Transport-Security: max-age=63072000; includeSubDomains; preload)
3. At this point, HSTS appears to work (HTTP request directs to HTTPS)
4. Use other apps for a while or reboot phone
5. Return to Firefox and visit http://radio.indymedia.org and it loads; however the browser should have redirected me to https://radio.indymedia.org

Actual results:

HTTP website loaded again despite HSTS policy

Expected results:

Automatic redirect to HTTPS

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Sounds like a platform problem. Sending to GV to evaluate if this is something they should handle or networking.

Comment 2

[Andrew McCreight [:mccr8]]

Dana, any guesses about what part of our Android browser might be responsible? Thanks.

Comment 3

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

I would check if the SiteSecurityServiceState.txt file in the profile directory has an entry corresponding to that site. It only gets written out every 5 minutes and at shutdown, but if that doesn't happen, the data won't be saved.

Comment 4

[mark burdett]

Is there a procedure to view profile files such as SiteSecurityServiceState.txt on a non-rooted device? If so I could look at this file where I saw the issue.

Meanwhile I did some playing around in a rooted Android emulator device running Fennec F-Droid 97.1.1, and yes there are situations where the SiteSecurityServiceState.txt is not saved, e.g. when rebooting device or force quitting the app. Would it be possible to save the SiteSecurityServiceState.txt as soon as HSTS is encountered? Admittedly, this might only be an issue in edge cases like rebooting (unless there is some scenario like the file isn't saved due to power-saving preventing background tasks from running).

I also found another fairly minor edge case that if the site is not present in SiteSecurityServiceState.txt, then loading the HTTPS page from cache (the page had both HSTS header and Cache-Control header allowing caching) will not apply the HSTS policy; only when the HTTPS page is reloaded from the site will the HSTS policy will be applied. This leads to the unexpected situation where the page with HSTS policy is cached but the HSTS policy itself was lost.

Comment 5

[Emily Toop (:fluffyemily)]

Agi, is there work that needs to be done in GV for this?

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #3)

I would check if the SiteSecurityServiceState.txt file in the profile directory has an entry corresponding to that site. It only gets written out every 5 minutes and at shutdown, but if that doesn't happen, the data won't be saved.

This... doesn't sound ideal on mobile. Our sessions can easily be shorter than 5 minutes and we don't have a shutdown hook. Dana is there any chance we could lower this timeout or hook it to an event that fires on mobile? application-background perhaps? https://searchfox.org/mozilla-central/search?q=application-background&path=

Comment 7

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

Hooking it to an event would be difficult. We can shorten the timeout. What would you like it to be?

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #7)

Hooking it to an event would be difficult. We can shorten the timeout. What would you like it to be?

I would say 30s should be sufficient. I'm not sure if there's any specific trade-off.

Comment 9

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

The trade-off is how often the platform writes out a 50-100K file. If it's okay to do that every 30s, then that's what we can do.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #9)

The trade-off is how often the platform writes out a 50-100K file. If it's okay to do that every 30s, then that's what we can do.

I think that would be better than not writing it, but could you explain to me why it would be hard to handle application-background here: https://searchfox.org/mozilla-central/source/security/manager/ssl/DataStorage.cpp#763-775? it seems like it would be very similar to the existing block (minus the shutdown stuff).

Comment 11

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

My initial thought was that we didn't have a straightforward way to ensure that the background task that writes the file runs before responding to the notification finishes, but since there's TaskQueue::AwaitIdle(), it seems like that should work (as long as we're okay with blocking the main thread? (and if we're not, then there's definitely no way to do this)).

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

My initial thought was that we didn't have a straightforward way to ensure that the background task that writes the file runs before responding to the notification finishes

I don't think we need that guarantee, a best effort to start writing to disk when we get the notification is enough. The last app to be active is always highly prioritized on Android, so the chance that we get killed immediately after receiving application-background is very low.

Comment 13

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

Can the app still run when it's in the background? We don't want a half-written security state file - that would be worse than not adding new entries to it (we could lose existing entries).

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #13)

Can the app still run when it's in the background? We don't want a half-written security state file - that would be worse than not adding new entries to it (we could lose existing entries).

It can yes, but by being in the background the likelihood of being killed is higher than when in the foreground. Blocking application-background won't change that, by the time we get the notification we're already in the background (there is no way for an app to block being put in the background).

This is also a problem for saving the file on a timer, as I understand we're not really checking whether we're in foreground or not when the timer fires, so there is the possibility of being killed while writing the file.

The only way to avoid being killed (up to a reasonable point) is to display a blocking notification, which elevates the app to foreground priority, but this seems overkill for this case.

Could we make writing the file atomic? E.g. by creating a temp file and then renaming it for the real one when finished writing it?

Comment 15

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

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #14)

by the time we get the notification we're already in the background

Oh, I see. Well, I guess it doesn't matter, then.

Could we make writing the file atomic? E.g. by creating a temp file and then renaming it for the real one when finished writing it?

Yeah that's probably a good idea anyway.

Comment 16

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

Attachment: Bug 1757138 - atomically write DataStorage data and kick off a write when backgrounded r?agi!,jschanck!

DataStorage writes should be atomic to avoid losing data if writing is
interrupted. Additionally, on mobile, if the app is backgrounded, it is more
likely to be killed, so an asynchronous write should be kicked off to hopefully
avoid losing data.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

atomically write DataStorage data and kick off a write when backgrounded r=agi,jschanck
https://hg.mozilla.org/integration/autoland/rev/adf6bf264f34df3973456103341763ae5bd9c600
https://hg.mozilla.org/mozilla-central/rev/adf6bf264f34

Comment 18

[Frederik Braun [:freddy]]

Attachment: advisory.txt