DOM storage quota should exclude offline-app allowed domains

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: dveditz, Assigned: mayhemer)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +
blocking1.9.1 -
wanted1.9.1 +
blocking1.9 -
wanted1.9.0.x -

Firefox Tracking Flags

(blocking2.0 alpha1+)

Details

(Whiteboard: [sg:dos] wontfix for 1.9 and older, URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Wanted on the branch, but depends on the effective-TLD service landing there so not for 1.8.1.2
Depends on: 331510, 342314
Flags: wanted1.8.1.x+

Comment 2

12 years ago
Created attachment 251960 [details] [diff] [review]
use the effective tld service to decide the owner domain
Attachment #251960 - Flags: review?(enndeakin)

Updated

12 years ago
Attachment #251960 - Attachment is patch: true
Attachment #251960 - Attachment mime type: application/octet-stream → text/plain

Comment 3

12 years ago
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

12 years ago
Created attachment 251977 [details] [diff] [review]
better string handling

I nabbed that from the tld service;  the stuff in nsReadableUtils is indeed nicer.
Attachment #251960 - Attachment is obsolete: true
Attachment #251977 - Flags: review?(enndeakin)
Attachment #251960 - Flags: review?(enndeakin)
(Reporter)

Comment 5

12 years ago
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)
Assignee: general → dcamp
You should take care of bug 364129 as well, I suppose.
(Reporter)

Comment 7

12 years ago
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 9

12 years ago
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.
Attachment #251977 - Flags: review?(enndeakin)
err, bug 367446 was the bug for comment 5.
(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.
(Reporter)

Comment 12

12 years ago
good point :-(
added bug 364129 to the dependency list
Depends on: 364129
Depends on: 386155
Depends on: 386154

Comment 13

11 years ago
Created attachment 288528 [details] [diff] [review]
v1, updated to new interface

updated existing patch to new eTLD interface.
Attachment #251977 - Attachment is obsolete: true
Attachment #288528 - Flags: review?

Updated

11 years ago
Attachment #288528 - Flags: review? → review?(dcamp)

Comment 14

11 years ago
FYI: The specification changed. This is now a same-origin feature.
(Reporter)

Comment 15

11 years ago
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.
Flags: blocking1.9?

Comment 16

11 years ago
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.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4

Comment 17

11 years ago
Bumping up priority- dcamp we gonna get this done?
Priority: P4 → P3

Updated

11 years ago
Attachment #288528 - Flags: review?(dcamp) → review+

Comment 18

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

pretty sure i need sr in dom/ - if not, please correct me ;)
Attachment #288528 - Flags: superreview?(jst)
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?
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?
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.
Attachment #288528 - Flags: superreview?(jst)

Updated

11 years ago
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-
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...
Attachment #288528 - Flags: superreview?(jst)
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.

Updated

11 years ago
Attachment #288528 - Flags: review+ → review-

Updated

11 years ago
Attachment #288528 - Flags: superreview?(jst)
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).
(Reporter)

Comment 25

11 years ago
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?
(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.

(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.
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).
Flags: blocking1.9- → blocking1.9?
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.
Flags: blocking1.9? → blocking1.9-
(Reporter)

Comment 30

10 years ago
Now that DOM storage
Flags: wanted1.9.1?
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Whiteboard: [sg:dos]
(Reporter)

Comment 31

10 years ago
oops, ignore comment 30. Nominating for future releases because this is still a potential DoS.
Not a blocker, but if a low-risk patch shows up we'll consider it.
Flags: wanted1.9.2+
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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.
Assignee: dcamp → honzab.moz
Flags: blocking1.9.2? → blocking1.9.2-
QA Contact: ian → general
(Assignee)

Comment 34

10 years ago
(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.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Depends on: 399526
(Assignee)

Comment 35

10 years ago
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.
(Assignee)

Comment 36

10 years ago
Created attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

This is implementation conforming comment 35 including tests for all the new stuff.
Attachment #288528 - Attachment is obsolete: true
Attachment #391414 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Summary: DOM storage quota should use effectiveTLD → DOM storage quota should exclude offline-app allowed domains
Attachment #391414 - Flags: review?(jst) → review+
Comment on attachment 391414 [details] [diff] [review]
v2 [Checkin m-c comment 41]

r=jst!
(Assignee)

Comment 38

9 years ago
Created attachment 406238 [details] [diff] [review]
v2 (merged)

Ready to land.
Attachment #391414 - Attachment is obsolete: true
Attachment #406238 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #391414 - Attachment is obsolete: false
(Assignee)

Updated

9 years ago
Attachment #406238 - Attachment is obsolete: true
(Assignee)

Comment 39

9 years ago
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?
(Assignee)

Updated

9 years ago
Attachment #391414 - Flags: superreview?(mrbkap)
(Assignee)

Comment 40

9 years ago
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.
(Reporter)

Updated

9 years ago
Flags: wanted1.9.0.x+ → wanted1.9.0.x-
Whiteboard: [sg:dos] → [sg:dos] wontfix for 1.9 and older
Attachment #391414 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 41

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

http://hg.mozilla.org/mozilla-central/rev/7923f101086a
Attachment #391414 - Attachment description: v2 → v2 [Checkin m-c comment 41]
Attachment #391414 - Flags: approval1.9.2?
Attachment #391414 - Flags: approval1.9.1.5?
(Assignee)

Comment 42

9 years ago
Removing 1.8.1 wanted flag, see https://bugzilla.mozilla.org/show_bug.cgi?id=399526#c6.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: wanted1.8.1.x+
Resolution: --- → FIXED
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.
Attachment #391414 - Flags: approval1.9.1.6? → approval1.9.1.6-
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.
Attachment #391414 - Flags: approval1.9.2?
(Assignee)

Comment 45

9 years ago
Nominating for 1.9.3 blocking.
blocking2.0: --- → ?
blocking2.0: ? → alpha1
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.