Closed
Bug 367373
Opened 18 years ago
Closed 15 years ago
DOM storage quota should exclude offline-app allowed domains
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | alpha1+ |
People
(Reporter: dveditz, Assigned: mayhemer)
References
()
Details
(Whiteboard: [sg:dos] wontfix for 1.9 and older)
Attachments
(1 file, 4 obsolete files)
64.78 KB,
patch
|
jst
:
review+
jst
:
superreview+
samuel.sidler+old
:
approval1.9.1.6-
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
Wanted on the branch, but depends on the effective-TLD service landing there so not for 1.8.1.2
Comment 2•18 years ago
|
||
Attachment #251960 -
Flags: review?(enndeakin)
Updated•18 years ago
|
Attachment #251960 -
Attachment is patch: true
Attachment #251960 -
Attachment mime type: application/octet-stream → text/plain
Comment 3•18 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•18 years ago
|
||
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•18 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
Comment 6•18 years ago
|
||
You should take care of bug 364129 as well, I suppose.
Reporter | ||
Comment 7•18 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 8•18 years ago
|
||
Added bug 364203 for comment 5
Comment 9•18 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)
Comment 10•18 years ago
|
||
err, bug 367446 was the bug for comment 5.
Comment 11•18 years ago
|
||
(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•18 years ago
|
||
good point :-(
added bug 364129 to the dependency list
Depends on: 364129
Comment 13•17 years ago
|
||
updated existing patch to new eTLD interface.
Attachment #251977 -
Attachment is obsolete: true
Attachment #288528 -
Flags: review?
Updated•17 years ago
|
Attachment #288528 -
Flags: review? → review?(dcamp)
Comment 14•17 years ago
|
||
FYI: The specification changed. This is now a same-origin feature.
Reporter | ||
Comment 15•17 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•17 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
Updated•17 years ago
|
Attachment #288528 -
Flags: review?(dcamp) → review+
Comment 18•17 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)
Comment 19•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #288528 -
Flags: superreview?(jst)
Updated•17 years ago
|
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-
Comment 22•17 years ago
|
||
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)
Comment 23•17 years ago
|
||
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•17 years ago
|
Attachment #288528 -
Flags: review+ → review-
Updated•17 years ago
|
Attachment #288528 -
Flags: superreview?(jst)
Comment 24•17 years ago
|
||
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•17 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?
Comment 26•17 years ago
|
||
(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•17 years ago
|
||
(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•17 years ago
|
||
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•16 years ago
|
||
Now that DOM storage
Flags: wanted1.9.1?
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Whiteboard: [sg:dos]
Reporter | ||
Comment 31•16 years ago
|
||
oops, ignore comment 30. Nominating for future releases because this is still a potential DoS.
Comment 32•16 years ago
|
||
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-
Comment 33•15 years ago
|
||
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-
Updated•15 years ago
|
QA Contact: ian → general
Assignee | ||
Comment 34•15 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 | ||
Comment 35•15 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•15 years ago
|
||
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•15 years ago
|
Summary: DOM storage quota should use effectiveTLD → DOM storage quota should exclude offline-app allowed domains
Updated•15 years ago
|
Attachment #391414 -
Flags: review?(jst) → review+
Comment 37•15 years ago
|
||
Assignee | ||
Comment 38•15 years ago
|
||
Ready to land.
Attachment #391414 -
Attachment is obsolete: true
Attachment #406238 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Attachment #391414 -
Attachment is obsolete: false
Assignee | ||
Updated•15 years ago
|
Attachment #406238 -
Attachment is obsolete: true
Assignee | ||
Comment 39•15 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•15 years ago
|
Attachment #391414 -
Flags: superreview?(mrbkap)
Assignee | ||
Comment 40•15 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•15 years ago
|
Flags: wanted1.9.0.x+ → wanted1.9.0.x-
Whiteboard: [sg:dos] → [sg:dos] wontfix for 1.9 and older
Updated•15 years ago
|
Attachment #391414 -
Flags: superreview?(mrbkap) → superreview+
Assignee | ||
Comment 41•15 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•15 years ago
|
||
Removing 1.8.1 wanted flag, see https://bugzilla.mozilla.org/show_bug.cgi?id=399526#c6.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: wanted1.8.1.x+
Resolution: --- → FIXED
Comment 43•15 years ago
|
||
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 44•15 years ago
|
||
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?
Updated•15 years ago
|
blocking2.0: ? → alpha1
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•