367373 - DOM storage quota should exclude offline-app allowed domains

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Daniel Veditz [:dveditz]]

bug 362446 implements a DOM storage quota as recommended by the WHAT-WG spec

    5.4.7.1. Disk space
    User agents should limit the total amount of space allowed for
    a domain based on the domain of the page setting the value.

The mechanism implemented, however, is based on the -host- name which allows a malicious site to abuse the storage space by creating sub-domains. The spec warns against this too:

    User agents should consider additional quota mechanisms (for
    example limiting the amount of space provided to a domain's subdomains
    as a group) so that hostile authors can't run scripts from multiple
    subdomains all adding data to the global storage area in an attempted
    denial-of-service attack.

One such "additional quota mechanism" would be to base the quota on the domain name instead of the host. One way to do this is to use the effective-TLD feature  and use the first non-TLD part of the FQDN.

Comment 1

[Daniel Veditz [:dveditz]]

Wanted on the branch, but depends on the effective-TLD service landing there so not for 1.8.1.2

Comment 2

[Dave Camp (:dcamp)]

Attachment: use the effective tld service to decide the owner domain

Comment 3

[Neil Deakin]

Comment on attachment 251960 [details] [diff] [review]
use the effective tld service to decide the owner domain


>+  PRUint32 tldLength;
>+  rv = tldService->GetEffectiveTLDLength(aDomain, &tldLength);
>+  if (NS_FAILED(rv)) return rv;
>+  
>+  nsACString::const_iterator iter;
>+  aDomain.BeginReading(iter);
>+  const char *start = iter.get();
>+  PRUint32 length = iter.size_forward();
>+  PRInt32 end = length - (tldLength + 1);
>+  for (const char *where = start + end - 1; where >= start; --where) {
>+    if (*where == '.') {
>+      aOwnerDomain = Substring(aDomain, (where + 1) - start, length);
>+      break;
>+    }
>+  }

You create an iterator here but don't seem to use it.

Did you look to see if nsReadableUtils contains a method which can do this search easier?

Comment 4

[Dave Camp (:dcamp)]

Attachment: better string handling

I nabbed that from the tld service;  the stuff in nsReadableUtils is indeed nicer.

Comment 5

[Daniel Veditz [:dveditz]]

nsIEffectiveTLDService doesn't seem to be overly helpful as an interface. I can imagine a lot of the consumers are going to want to know the "effective domain" (e.g. "mozilla.com") as you're trying to figure out here.

If you're going to code that up anyway maybe you can put it into the service instead of locally in dom-storage.

(I can also see a use for utility methods that return the effectiveTLD and effective domains as strings, especially convenient for script use)

Comment 6

[Dão Gottwald [:dao]]

You should take care of bug 364129 as well, I suppose.

Comment 7

[Daniel Veditz [:dveditz]]

asking that code added to fix this bug be moved to a centrally useful place before checkin is probably appropriate, but bug 364129 is really a separate bugfix entirely.

My parenthetical suggestion in comment 5 about extra utility string methods should also go into a separate bug.

Comment 8

[Dave Camp (:dcamp)]

Added bug 364203 for comment 5

Comment 9

[Dave Camp (:dcamp)]

Comment on attachment 251977 [details] [diff] [review]
better string handling

Patch should use the methods proposed in bug 367446.  Also, until the data file is added (bug 342314), adding this will cause all .co.uk sites to share the same quota.

Comment 10

[Dave Camp (:dcamp)]

err, bug 367446 was the bug for comment 5.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #7)
> but bug 364129 is really a separate bugfix entirely.

Sure, another fix, but still somehow related. Just like with .co.uk, you don't want IP addresses ending with the same number to share the same quota.

Comment 12

[Daniel Veditz [:dveditz]]

good point :-(
added bug 364129 to the dependency list

Comment 13

[[e-mail]]

Attachment: v1, updated to new interface

updated existing patch to new eTLD interface.

Comment 14

[Anne (:annevk)]

FYI: The specification changed. This is now a same-origin feature.

Comment 15

[Daniel Veditz [:dveditz]]

This should block FF3 unless we decide we have time to obsolete the old syntax in favor of the new. Then we get to have fun deciding what to do with any migrated storage data or whether to try to support pages using the old syntax or not.

Comment 16

[[e-mail]]

we probably want this even if this is same-origin; this is really serving as an "additional quota mechanism" per comment 0, and will make it hard for sites to abuse storage.

Comment 17

[Mike Schroepfer]

Bumping up priority- dcamp we gonna get this done?

Comment 18

[[e-mail]]

Comment on attachment 288528 [details] [diff] [review]
v1, updated to new interface

pretty sure i need sr in dom/ - if not, please correct me ;)

Comment 19

[Dave Camp (:dcamp)]

Hrm, actually this will break the offline app preferences patch.

To purge an offline app's data, we need to be able to identify the hostname it came from.  With this patch we won't be able to tell which data came from mail.mozilla.com and which came from www.mozilla.com.

Any ideas?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Can we still store the full domain and either store the effective TLD in addition or get the list of all entries that have the given effective TLD or something?

Comment 21

[Dave Camp (:dcamp)]

It wouldn't be too hard to store the full domain in addition to the effective tld.

And if we implement the recent changes to the globalStorage spec (bug 407839), we'll need to do that anyway.  So that's probably the best plan.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 288528 [details] [diff] [review]
v1, updated to new interface

We still want this patch, right? If not, let me know and I won't spend time sr'ing...

Comment 23

[Dave Camp (:dcamp)]

I don't think we want that patch as it stands right now.  It would break some offline-app stuff, and I think 407839 will prevent abuse.

Comment 24

[Dave Camp (:dcamp)]

So if/when 407839 lands, we can make a few changes to make this work correctly with offline apps:

a) the offline app management stuff should start working with Storages rather than Owners (clearing mail.google.com can clear the mail.google.com storage, instead of the mail.google.com owner).  This would need some changes to nsDOMStorageManager::ClearOfflineApps(), nsDOMStorageManager::GetUsage(), and nsDOMStorageManager::Observe()

b) We'd need to fix up the quota wrt offline apps.  GetQuota() would need to be passed the complete requesting domain, not just the etld+1.  And ideally when writing to a storage with a domain that has the offline-app permission, you'd measure the usage of the Storage instead of the Owner (because the owner will be the etld+1, so mail.google.com and reader.google.com would share a quota, which isn't really what you want).

Comment 25

[Daniel Veditz [:dveditz]]

I'd like to believe that restricting globalStorage to same-origin would obviate the need for this bug, but since the point of the quota is to prevent "abuse" we have to think abusively. A misbehaved site that wants to exceed its quota can create a bunch of virtual subdomains and then share the information back to the host site through either setting document.domain or (new for FF3) using postMessage.

I suppose postMessage would let them escape the quota in any case by creating new unrelated base domains -- cheap enough if you really want to. But not so cheap that some joker will post a PoC on full-disclosure that fills your disk more than some small number of quota-multiples.

I think limiting the abuse is better so I still support this change even with bug 407839 landing.  We'll have to make an exception for offline-apps so that the Owner matches the Storage, but use the eTLD+1 for the Owner in all other cases. And then if some app was removed from the offline approval list we'd have to switch the owner on its global storage back to eTLD+1. Would that work?

Comment 26

[Dave Camp (:dcamp)]

(In reply to comment #25)
> I think limiting the abuse is better so I still support this change even with
> bug 407839 landing.  We'll have to make an exception for offline-apps so that
> the Owner matches the Storage, but use the eTLD+1 for the Owner in all other
> cases. And then if some app was removed from the offline approval list we'd
> have to switch the owner on its global storage back to eTLD+1. Would that work?

I think that we can always store the etld+1 in the owner field, and the complete domain in the storage-name field, and just change which one we count depending on the offline-app permission.

Comment 27

[Jeff Walden [:Waldo]]

(In reply to comment #25)
> I suppose postMessage would let them escape the quota in any case by creating
> new unrelated base domains -- cheap enough if you really want to.

This was already possible anyway, albeit with a fair amount of pain and coordination and potential for raciness, using any one of the existing methods for communicating across origins by polling on changes to the hash portion of a window.

Comment 28

[Dave Camp (:dcamp)]

Based on comment #25, we should probably be blocking on this one.

It should be relatively easy to fix for b5.  It will change how the data is stored on disk a bit (we'll be tagging data with the etld+1 in addition to the complete domain).  Basically, any data we store before this patch won't be accounted for in the quota after this patch (unless it happened to come from an etld+1 domain in the first place).

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not a blocker. The only thing you can do is work around the storage limit, which, while bad, won't really cause that much harm to the user.

Comment 30

[Daniel Veditz [:dveditz]]

Now that DOM storage

Comment 31

[Daniel Veditz [:dveditz]]

oops, ignore comment 30. Nominating for future releases because this is still a potential DoS.

Comment 32

[Johnny Stenback (:jst)]

Not a blocker, but if a low-risk patch shows up we'll consider it.

Comment 33

[Johnny Stenback (:jst)]

Honza, is this something you could look into? If not, please let me know. Also, should we block 1.9.2 on this? I'll say no for now, but please renominate if this is serious enough to block on.

Comment 34

[Honza Bambas (:mayhemer)]

(In reply to comment #33)
> Honza, is this something you could look into? If not, please let me know. 

I can work on this. As I remember I should have some ideas how to fix this.

> Also,
> should we block 1.9.2 on this? I'll say no for now, but please renominate if
> this is serious enough to block on.

For now, I don't think it should block, at least the a1 release. As web-storage w3c spec says, browser just _should_ do something about this.

Comment 35

[Honza Bambas (:mayhemer)]

I find out that we've been already using eTLD+1 for DOM storage quota checks. But the way we do it is not complete. Imagine the following scenario:

offlineapp.example.com is storing offline data, user has set offline application privilege to it and it now can store it's own 50MB of data. So can do any subdomains to this one like sub.offlineapp.example.com etc (after bug 399526 is fixed).

Then, www.example.com wants to use localStorage to save some user preferences on the main web page. It has to fit to limit of 5MB for the whole example.com (eTLD+1) domain. If offlineapp.example.com has already stored some (more then 5MB of data) www.example.com would fail to use the localStorage with the code we have now, because we sum all data stored under example.com at the moment. However from the logical point it should be able to store the web site data.

What we need is to exclude the offline application data from the usage under example.com domain. That's what I'm currently working on.

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v2 [Checkin m-c comment 41]

This is implementation conforming comment 35 including tests for all the new stuff.

Comment 37

[Johnny Stenback (:jst)]

Comment on attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

r=jst!

Comment 38

[Honza Bambas (:mayhemer)]

Attachment: v2 (merged)

Ready to land.

Comment 39

[Honza Bambas (:mayhemer)]

The v2 is correct (no need for merge). I realized that this bug is dependent on bug 399526.

jst, could you please review also that bug?

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

Blake, could you please take a look or suggest someone else to do this? We should get this on m-c ASAP, this bug grows old.

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

http://hg.mozilla.org/mozilla-central/rev/7923f101086a

Comment 42

[Honza Bambas (:mayhemer)]

Removing 1.8.1 wanted flag, see https://bugzilla.mozilla.org/show_bug.cgi?id=399526#c6.

Comment 43

[Samuel Sidler (old account; do not CC)]

Comment on attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

We won't take this on 1.9.1. We're getting close to releasing 1.9.2 and are hoping to eventually auto-update users from 1.9.1 so you should get approval for that release.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

Comment 45

[Honza Bambas (:mayhemer)]

Nominating for 1.9.3 blocking.