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.

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

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?

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

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)

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

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.

Added bug 364203 for comment 5

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.

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.

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

updated existing patch to new eTLD interface.

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

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.

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.

Bumping up priority- dcamp we gonna get this done?

Comment on attachment 288528 [details] [diff] [review] v1, updated to new interface pretty sure i need sr in dom/ - if not, please correct me ;)

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.

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...

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.

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).

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).

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.

Now that DOM storage

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.

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.

(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.

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.

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

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

Ready to land.

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 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 on attachment 391414 [details] [diff] [review] v2 [Checkin m-c comment 41] http://hg.mozilla.org/mozilla-central/rev/7923f101086a

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

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 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.

Nominating for 1.9.3 blocking.