732166 - escape cookies in sessionstore

Status: RESOLVED WORKSFORME

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

name, value, path all need to be escaped

Comment 1

[Josh Aas]

Marking sg:moderate based on a similar rating in bug 730531, Paul said to assign to him.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

(pretty sure it was possible to use cookie.path as an attack vector, though it may require server side setting). This at least tests encoding

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 617981 [details] [diff] [review]
Patch v0.1

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

per talk IRL, r=me with a simple api regression test added.

Comment 4

[:Gijs (he/him)]

Is this bug still current? It looks like this never landed and was never marked fixed, and session store has changed a lot...

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Nobody is working on Session Restore atm. Well, maybe a bit mconley for e10s-specific stuff :/

I double-checked, we still don't escape cookies. The patch is dead, but it should be simple to port SessionCookies.jsm to adopt these conventions – the main difficulty is to make sure we don't break existing sessionstore.js.

Comment 6

[:Gijs (he/him)]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #5)
> Nobody is working on Session Restore atm. Well, maybe a bit mconley for
> e10s-specific stuff :/
> 
> I double-checked, we still don't escape cookies. The patch is dead, but it
> should be simple to port SessionCookies.jsm to adopt these conventions – the
> main difficulty is to make sure we don't break existing sessionstore.js.

Pulling in mconley.

Out of curiosity, why do we need to do this (escape these values)? We serialize to JSON, which should escape anything that wants escaping, right?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #6)
> Out of curiosity, why do we need to do this (escape these values)? We
> serialize to JSON, which should escape anything that wants escaping, right?

Would love to know too, this doesn't seem to make a lot of sense thus far... Unfortunately non of the bugs have tests and don't name any specific examples.

Comment 8

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Out of curiosity, why do we need to do this (escape these values)? We
> > serialize to JSON, which should escape anything that wants escaping, right?
> 
> Would love to know too, this doesn't seem to make a lot of sense thus far...
> Unfortunately non of the bugs have tests and don't name any specific
> examples.

I just realized all the detail here is in the blocking bug this hangs off of. Both this and its 3 other deps are about escaping data in the sessionstore json file in order to make it harder to use it for driveby XSS uploads (of the kind of "stick HTML file on disk, put HTML in title that ends up in sessionstore, load sessionstore as HTML in an iframe, now the HTML you put in the <title> gets executed and you can use it to upload stuff in the profile folder (where we keep sessionstore.js)"). The initial PoC used <title>, but you could do similar things with cookies and various other tidbits because they're all stored as-is in sessionstore.js

As noted on bug 729093, if we just gzipped all the sessionstore files we could avoid this problem, too.

Looking at that bug, and considering how old it is, I don't know what the priority of this bug and its friends should be. The migration of data, as noted in comment #5, is the annoying thing here...

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Is this attack even still possible? How do I load up my session store JSON file and render it like HTML? Do I have to spoof the mimetype or something?

Comment 10

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> Is this attack even still possible? How do I load up my session store JSON
> file and render it like HTML? Do I have to spoof the mimetype or something?

<object type="text/html" data="file:///some/thing"></object>

does that. See the testcase in bug 729093.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I unfortunately have no access to that bug, so I'm missing some context. :/

Comment 12

[David Teller [:Yoric] - still alive but not very active]

How would encoding change anything? As soon as the page can somehow access the content of a local file, it can decode it, or am I missing some cross-platform authorization security thingy thingy?

Comment 13

[:Gijs (he/him)]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #12)
> How would encoding change anything? As soon as the page can somehow access
> the content of a local file, it can decode it, or am I missing some
> cross-platform authorization security thingy thingy?

Being able to load a page (in an iframe, other window, whatever) != being able to read what is on the page

The problem, then, is that you can inject things into sessionstore.js (because it contains things from the current pages you have open) that get reflected exactly. Think XSS. So you can store a valid script value inside the title of a page, and that then gets executed. Similar things are likely possible with cookies etc.

So once you can load sessionstore.js as HTML, you win, because the thing you stuck in there earlier now gets run, and that means you can execute code running in that folder, which means you can access other things in that folder, and potentially upload them places.

file:/// pages in other directories cannot generally read sessionstore.js because of the security checks we do. But they can still open it in an iframe or whatever, if they know the full path, and that produces this issue.

That said... the impact of the bug was clearly reduced by the move of sessionstore.js to a separate folder, because it is now no longer possible to access files in the parent directory from there. Of course, sessionstore.js still has plenty of privacy-sensitive things that we should be protecting...

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> I unfortunately have no access to that bug, so I'm missing some context. :/

I'll CC all of you.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from
> comment #12)
> > How would encoding change anything? As soon as the page can somehow access
> > the content of a local file, it can decode it, or am I missing some
> > cross-platform authorization security thingy thingy?
> 
> Being able to load a page (in an iframe, other window, whatever) != being
> able to read what is on the page

But I still don't get the difference between an encoded and a decoded version of sessionstore.js

Comment 15

[:Gijs (he/him)]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from
> > comment #12)
> > > How would encoding change anything? As soon as the page can somehow access
> > > the content of a local file, it can decode it, or am I missing some
> > > cross-platform authorization security thingy thingy?
> > 
> > Being able to load a page (in an iframe, other window, whatever) != being
> > able to read what is on the page
> 
> But I still don't get the difference between an encoded and a decoded
> version of sessionstore.js

If sessionstore.js is not encoded, and I open a page that has a cookie with a path that looks like this:

<script>alert('lol')</script>

and that gets saved literally to sessionstore.js, and I now load sessionstore.js as an HTML page, I will get an alert with that text.

If we encoded cookies (and page titles, and everything else we store), loading sessionstore.js as HTML would not be able to do that.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Gotcha, thanks.

Comment 17

[Mike de Boer [:mikedeboer]]

We are now compressing all the sessionstore files on disk... comment 8 seems to suggest that this would have fixed the issue here, am I right?

Comment 18

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> We are now compressing all the sessionstore files on disk... comment 8 seems
> to suggest that this would have fixed the issue here, am I right?

I think it would, yeah. Can you mark this as a dep of whatever bug actually added the compression, and mark this WFM + remove the security group? (I can do the latter afterwards if you don't have permissions to do so.)

From a quick look I suspect we can close bug 729093 in the same way.

Comment 19

[Mike de Boer [:mikedeboer]]

I can't remove the sec group, unfortunately, so can you do that for me?

Comment 20

[Mike de Boer [:mikedeboer]]

Oh and I can't access bug 729093.