880507 - Granting a temporary cert exception grants a permanent exception even after a phone restart or shut down and power up

Status: RESOLVED DUPLICATE of bug 660749

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Might be a regression from bug 855543.

STR

1. Install Hosted App Test Case 1 from http://mozqa.com/webapi-permissions-tests/
2. Launch app
3. Select Expired Cert Site
4. Select Visit Site (grants a temporary exception)
5. Restart the phone
6. Launch app
7. Select Expired Cert Site

Expected

A cert error page should be present.

Actual

The user gets access to the site immediately.

Additional Notes

This is different bug 858730 in that when that bug was filed, the issue would fix itself on phone restart or power off & start up. This bug persists even on phone restart or power off/on.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Regression + security concerns, blocking.

Comment 2

[Alex Keybl [:akeybl]]

Doug - can you help with an assignee here?

Comment 3

[Doug Turner (:dougt)]

sid manages the security engineering team.

Comment 4

[Carmen Jimenez Cabezas]

I'm taking this one

Comment 5

[Jason Smith [:jsmith]]

Sounds good. I'll pull needinfo on Sid since we have someone looking into this.

Comment 6

[Carmen Jimenez Cabezas]

After losing some time tracking the life of the bool that marks if an exception is temporary or permanent (and not understanding anything since it should work according to the code), I thought of doing a simple test. And, effectively, when you select 'Visit site' (that is, when you grant a temporary exception) the certificate exception isn't stored permanently.

What is happening, instead, is that the test seems to fail because the visited page is being cached, and the cache survives a phone reboot. You can check this easily just disabling the phone data connection before executing step 6 of the original STR.

If you want to check that, actually, the exception hasn't been granted permanently, at step 5 of the STR you should erase the cache instead of (or besides) rebooting the phone.

The way I did this (and if there's a better way, this is a good moment to tell me) was:

adb shell
> stop b2g
> rm /data/b2g/mozilla/??*.default/startupCache/*
> start b2g

Therefore, this is not a security failure, actually (since the exception isn't being stored), but a case of... overzealous caching.

What I really don't know is if this is actually a bug or not. The way it's working may be because the logic to make hosted apps work in offline mode means caching everything that can be cached, or it may be that the visited pages (your hosted app and example sites) headers are actually returning a very long life time for the cache. In other words, I don't know if there's a failure at all or not.

A possible solution, in any case, would be to disable caching only for visited sites with temporary certificate exemptions, but I don't really know if it's worth it.

Also, I don't know if this is really a blocker anymore, since it doesn't actually involve any risk for the end users.

Comment 7

[Jason Smith [:jsmith]]

I don't think we should caching by default if it's a hosted app - we should only be caching web content that has explicitly been allowed by appcache. The caching issue sounds like incorrect behavior to me, but someone on networking might know more. I'm not sure who to ask - Andrew do you know?

Comment 8

[Andrew Overholt [:overholt]]

Honza, can you clarify the cache/appcache/cert behaviour we expect here?  Thanks.

Comment 9

[Honza Bambas (:mayhemer)]

I think this is a dupe of bug 660749.  bsmith?

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Carmen Jimenez Cabezas from comment #6)
> Therefore, this is not a security failure, actually (since the exception
> isn't being stored), but a case of... overzealous caching.
>
> What I really don't know is if this is actually a bug or not. The way it's
> working may be because the logic to make hosted apps work in offline mode
> means caching everything that can be cached, or it may be that the visited
> pages (your hosted app and example sites) headers are actually returning a
> very long life time for the cache. In other words, I don't know if there's a
> failure at all or not. 

See bug 660749. We don't re-validate certificates for items from the cache. Generally, there is agreement that we should be re-validating certificates when we read items from the cache. But, that work hasn't been prioritized yet.

> A possible solution, in any case, would be to disable caching only for
> visited sites with temporary certificate exemptions, but I don't really know
> if it's worth it.

I agree that it seems like a good idea to avoid writing items to the persistent cache for sites with cert error overrides. Sites with cert error overrides are not something to optimize for, and in fact there is a very high probability that we would cache the wrong thing anyway (e.g. a redirect to a captive portal login page). But, would we put them in the memory cache? Or, just not cache any of their responses? It seems bad to bloat the memory cache. But, not caching the resources at all would result in highly suboptimal behavior for any site where the user actually needs to use a cert error override.

> Also, I don't know if this is really a blocker anymore, since it doesn't
> actually involve any risk for the end users.

Besides all of the above: On B2G, our cert error override mechanism is global and highly persistent. I do not think that a user who uses the cert error override mechanism in the browser is intending for that to affect the operation of any app. And, also, I would suspect that users that choose "temporary" don't intend to allow the override for several days or weeks or months that they have their phone powered on for. So, perhaps we should just change our concept of "temporary" to be time-based instead of "until Gecko is restarted." I think that we did something similar for click-to-play plugins.

Comment 11

[Jason Smith [:jsmith]]

Turns out this isn't a regression then from bug 855543 then.

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

I don't think this is really a blocker... I'm not even sure this is a security concern at all, since the moment the network is accessed a warning is given again. Restricting the exception duration to a given amount of time (or to an application's lifetime) is something that we should probably do, but I don't think that's critical for the 1.1 timeframe (amongst other things, I'm not even sure what the best thing would be... restrict it to the child process that required the exception? set a given time limit?).

In any case, on it's current form this seems a complete dupe of bug 660749. We can open a new bug to restrict the scope of 'temporary' on FFOS, but I don't think anything is actually needed on this concrete bug (other than closing it if you agree as a dupe of 660749).

Comment 13

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #12)
> I don't think this is really a blocker... I'm not even sure this is a
> security concern at all, since the moment the network is accessed a warning
> is given again. Restricting the exception duration to a given amount of time
> (or to an application's lifetime) is something that we should probably do,
> but I don't think that's critical for the 1.1 timeframe (amongst other
> things, I'm not even sure what the best thing would be... restrict it to the
> child process that required the exception? set a given time limit?).

Note this analysis isn't correct. Accessing the network again does *not* produce the warning again, which is the whole point of this bug. The fact is that we're caching the temporary certificates so long that granting a temporary certificate to a user feels the same as granting a permanent exception given the length of certificate granting, so we're in violation of definition of what temporary granting means.

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

Accessing the network again after a shutdown (which is what this bug was about ;) ) does indeed produce the warning again. It does not produce the warning when the network isn't accessed (and the resource is returned from the cache) which is what bug 660749 is about.

What I would propose to do here is to close this as a dupe of 660749 (which is what it is from the description and the analysis) and then if you wish open another bug to restrict the meaning of temporary in B2G. The new bug can be done for 1.2, I don't think we have time to do that for 1.1 (not if temporary means restricting it to the app lifetime which isn't something that exists at all on the current code AFAIk).

Comment 15

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #14)
> Accessing the network again after a shutdown (which is what this bug was
> about ;) ) does indeed produce the warning again. It does not produce the
> warning when the network isn't accessed (and the resource is returned from
> the cache) which is what bug 660749 is about.

I think I understand the point of confusion here. Upon a restart of the phone, the network you originally connected to is the same, so you are still granted access via a cert override mechanism. When that network connection is disconnected and re-established, then the cert override error appears. The bug that I'm talking about here *is* still reproducing - if I restart my phone, I still have the same network, so the temporary cert override still applies. I just confirmed this on today's build.

Note that the discussion on caching is irrelevant to this bug. This bug is not a dupe of the bug you mention - this is talking about the case of when you remain on the same network after granting a temporary cert override. Which apparently, survives a restart.

Comment 16

[Jason Smith [:jsmith]]

Actually disregard the irrelavancy of the caching comment. We're caching upon remaining on the same network. I'll close this as a dupe then.