916781 - DOM storage instances across managers must not share single eTLD+1 limit check

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

Credits: reported privately by Stefan Scherer via www.janbambas.cz.

There are actually two bugs:
- each localStorage/sessionStorage (+forks) instance must track it's own usage quota (checked that Chrome and IE behave that way), now they all share a global usage limit (filled localStorage currently blocks writes to sessionStorage that is otherwise free)
- and as a result, the consumption is not reverted to 0 when all windows using the particular eTLD+1 scope are closed (there is no hook to decrease usage on sessionStorage drop)

This *is not* a problem in the quota manager Jan Varga is working on (DOM storage is not using it).

Solution is to move the usage counter to respective storage managers.  This way each instance will track it's own limit (localStorage and sessionStorage as well as each fork of sessionStorage).

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- the usage amount collector moved from DB to respective managers
- this way we are in parity with other browsers (checked with IE and CH)
- there is a manual test case (that verifies the problem - will attach), so I don't include an automated test

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: manual test case (by Stefan Scherer)

How to use the test case (snippet from Stefan's email)

----------

Steps:
Open FF 
Open the attached file.

Open a new tab.
Paste in the url to the file. (available sessionStorage is reduced by 1 MB
to ~4 MB)

Open a new tab.
Paste in the url to the file. (available sessionStorage is reduced by 1 MB
to ~3 MB))

Open a new tab.
Paste in the url to the file. (available sessionStorage is reduced by 1 MB
to ~2 MB))

Open a new tab.
Paste in the url to the file. (available sessionStorage is reduced by 1 MB
to ~1 MB))

Open a new tab.
Paste in the url to the file. (available sessionStorage is reduced to
nothing 0 MB))

Navigate to your favorite url on a new tab.
Close all other tabs.

Open a new tab.
Past in the url to the file. (available sessionStorage is still 0 bytes))
Close this tab.
Clear the cache etc. (I cleared everything)
Open a new tab.
Past in the url to the file. (available sessionStorage is still 0 bytes))

The sessionStorage will stay at 0 capacity until the browser is restarted.

----------


I was also testing with middle-clicking the link on the page.  It reproduces the problem same way.

W/o the patch it demonstrates we have one etld+1 limit for all storage instancesm which is wrong.
W/ the patch it demonstrates there is etld+1 limit per-window (i.e. per manager), which is correct.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 805389 [details] [diff] [review]
v1

https://tbpl.mozilla.org/?tree=Try&rev=9fa40e1d3eaf

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Honza Bambas (:mayhemer) from comment #0)
> Credits: reported privately by Stefan Scherer via www.janbambas.cz.
> 
> There are actually two bugs:
> - each localStorage/sessionStorage (+forks) instance must track it's own
> usage quota (checked that Chrome and IE behave that way), now they all share
> a global usage limit (filled localStorage currently blocks writes to
> sessionStorage that is otherwise free)
Why is Gecko's behavior a bug? I thought Chrome and IE are vulnerable to limitless localStorage usage, and we don't want that.

> - and as a result, the consumption is not reverted to 0 when all windows
> using the particular eTLD+1 scope are closed (there is no hook to decrease
> usage on sessionStorage drop)
This is indeed a problem.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] from comment #4)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> > Credits: reported privately by Stefan Scherer via www.janbambas.cz.
> > 
> > There are actually two bugs:
> > - each localStorage/sessionStorage (+forks) instance must track it's own
> > usage quota (checked that Chrome and IE behave that way), now they all share
> > a global usage limit (filled localStorage currently blocks writes to
> > sessionStorage that is otherwise free)
> Why is Gecko's behavior a bug? I thought Chrome and IE are vulnerable to
> limitless localStorage usage, and we don't want that.

This bug is absolutely not about fill disk!  

Let me describe this bug a bit more in detail: what we do wrong is that we coalesce limit for localStorage, sessionStorage and all sessionsStorage clones (a.k.a forks) when user opens a new window using window.open() or middle-clicking a link under single eTLD+1 counter.  All instances of DOM storages under one eTLD+1 scope share only a single limit.

With this patch localStorage will still have one global limit (there is one single instance of a manager for all localStorages in the running process).  So no fill disk attack.

With this patch sessionStorages keep their limit (5MB) only per window (each window keeps its own instance of sessionStorage manager where now the limit is kept).  This is how others behave and we don't w/o this patch.

Does it explain the patch for you?

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 805389 [details] [diff] [review]
v1

Jonas, also see the discussion in the bug.  Comment 5 may explain in details what the problem and the solution are.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ah, that explains what the bug is about.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 805389 [details] [diff] [review]
v1

># HG changeset patch
># Parent dc909122bcf5465392a73e4440bea8a4780818e9
>
>diff --git a/dom/src/storage/DOMStorageCache.cpp b/dom/src/storage/DOMStorageCache.cpp
>--- a/dom/src/storage/DOMStorageCache.cpp
>+++ b/dom/src/storage/DOMStorageCache.cpp
>@@ -83,18 +83,18 @@ DOMStorageCache::DOMStorageCache(const n
> , mSessionOnlyDataSetActive(false)
> , mPreloadTelemetryRecorded(false)
> {
>   MOZ_COUNT_CTOR(DOMStorageCache);
> }
> 
> DOMStorageCache::~DOMStorageCache()
> {
>-  if (mManager) {
>-    mManager->DropCache(this);
>+  if (mPersistingManager) {
>+    mPersistingManager->DropCache(this);
>   }
> 
>   MOZ_COUNT_DTOR(DOMStorageCache);
> }
> 
> NS_IMETHODIMP_(void)
> DOMStorageCache::Release(void)
> {
>@@ -129,16 +129,17 @@ DOMStorageCache::Init(DOMStorageManager*
> 
>   mManager = aManager;
>   mInitialized = true;
>   mPrincipal = aPrincipal;
>   mPersistent = aPersistent;
>   mQuotaScope = aQuotaScope.IsEmpty() ? mScope : aQuotaScope;
> 
>   if (mPersistent) {
>+    mPersistingManager = aManager;
>     Preload();
>   }
> }
> 
> inline bool
> DOMStorageCache::Persist(const DOMStorage* aStorage) const
> {
>   return mPersistent &&
>@@ -190,34 +191,31 @@ DOMStorageCache::ProcessUsageDelta(const
> {
>   return ProcessUsageDelta(GetDataSetIndex(aStorage), aDelta);
> }
> 
> bool
> DOMStorageCache::ProcessUsageDelta(uint32_t aGetDataSetIndex, const int64_t aDelta)
> {
>   // Check if we are in a low disk space situation
>-  if (aDelta > 0 && mManager && mManager->IsLowDiskSpace()) {
>+  if (aDelta > 0 && mPersistingManager && mPersistingManager->IsLowDiskSpace()) {
>     return false;
>   }
> 
>   // Check limit per this origin
>   Data& data = mData[aGetDataSetIndex];
>   uint64_t newOriginUsage = data.mOriginQuotaUsage + aDelta;
>   if (aDelta > 0 && newOriginUsage > DOMStorageManager::GetQuota()) {
>     return false;
>   }
> 
>   // Now check eTLD+1 limit
>-  GetDatabase();
>-  if (sDatabase) {
>-    DOMStorageUsage* usage = sDatabase->GetScopeUsage(mQuotaScope);
>-    if (!usage->CheckAndSetETLD1UsageDelta(aGetDataSetIndex, aDelta)) {
>-      return false;
>-    }
>+  DOMStorageUsage* usage = mManager->GetScopeUsage(mQuotaScope);
>+  if (!usage->CheckAndSetETLD1UsageDelta(aGetDataSetIndex, aDelta)) {
>+    return false;
>   }
> 
>   // Update size in our data set
>   data.mOriginQuotaUsage = newOriginUsage;
>   return true;
> }
> 
> void
>@@ -229,17 +227,17 @@ DOMStorageCache::Preload()
> 
>   if (!StartDatabase()) {
>     mLoaded = true;
>     mLoadResult = NS_ERROR_FAILURE;
>     return;
>   }
> 
>   sDatabase->AsyncPreload(this);
>-  sDatabase->GetScopeUsage(mQuotaScope);
>+  mManager->GetScopeUsage(mQuotaScope);
> }
> 
> namespace { // anon
> 
> // This class is passed to timer as a tick observer.  It refers the cache
> // and keeps it alive for a time.
> class DOMStorageCacheHolder : public nsITimerCallback
> {
>@@ -264,17 +262,17 @@ NS_IMPL_ISUPPORTS1(DOMStorageCacheHolder
> 
> } // anon
> 
> void
> DOMStorageCache::KeepAlive()
> {
>   // Missing reference back to the manager means the cache is not responsible
>   // for its lifetime.  Used for keeping sessionStorage live forever.
>-  if (!mManager) {
>+  if (!mPersistingManager) {
>     return;
>   }
> 
>   if (!NS_IsMainThread()) {
>     // Timer and the holder must be initialized on the main thread.
>     nsRefPtr<nsRunnableMethod<DOMStorageCache> > event =
>       NS_NewRunnableMethod(this, &DOMStorageCache::KeepAlive);
> 
>diff --git a/dom/src/storage/DOMStorageCache.h b/dom/src/storage/DOMStorageCache.h
>--- a/dom/src/storage/DOMStorageCache.h
>+++ b/dom/src/storage/DOMStorageCache.h
>@@ -159,21 +159,26 @@ private:
>   bool Persist(const DOMStorage* aStorage) const;
> 
>   // Changes the quota usage on the given data set if it fits the quota.
>   // If not, then false is returned and no change to the set must be done.
>   bool ProcessUsageDelta(uint32_t aGetDataSetIndex, const int64_t aDelta);
>   bool ProcessUsageDelta(const DOMStorage* aStorage, const int64_t aDelta);
> 
> private:
>+  // Weak reference to our manager, always non-null. It is ensured this pointer
>+  // is always valid and cannot underlive the cache.
What guarantees that?


>+  DOMStorageManager* mManager;

>+
>   // When a cache is reponsible for its life time (in case of localStorage data
>   // cache) we need to refer our manager since removal of the cache from the hash
>   // table is handled in the destructor by call to the manager.
>   // Cache could potentially overlive the manager, hence the hard ref.
>-  nsRefPtr<DOMStorageManager> mManager;
>+  // Note: keeps the same value as mManager.
>+  nsRefPtr<DOMStorageManager> mPersistingManager;
So this is odd. Based on the comment we have two member variables pointing to the same
object.
Why exactly do we need both member variables?
(and I'm very worried about raw pointer, which isn't explicitly set to null.
 Raw pointers have caused us tons of security critical bugs.)


r- because of use of raw pointer without explicitly setting it to null.
If possible, just use nsRefPtr and possibly DOMStorageManager itself should know whether it is
persistent.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2

I rather don't touch the manager <-> cache referencing.  There is a better way to get the usage object in the cache.

- DOMStorageUsage is now referencible
- it is obtained from the manager in DOMStorageCache::Init() and hard-referenced by DOMStorageCache
- DOMStorageCache::mManager left unchanged and is still held only when it is DOMLocalStorageManager (see [1]/Overview/DOMStorageCache why)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/DOM_Storage_implementation_notes?redirectlocale=en-US&redirectslug=DOM%2FStorage%2FImplementation

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 809142 [details] [diff] [review]
v2

Still asking for feedback from Jonas, just to make sure he agrees.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 809142 [details] [diff] [review]
v2

># HG changeset patch
># Parent d50f590056fdca521a53af8562e3aee754b6c863
>
>diff --git a/dom/src/storage/DOMStorageCache.cpp b/dom/src/storage/DOMStorageCache.cpp
>--- a/dom/src/storage/DOMStorageCache.cpp
>+++ b/dom/src/storage/DOMStorageCache.cpp
>@@ -70,18 +70,17 @@ NS_IMETHODIMP_(void) DOMStorageCacheBrid
>     /* NS_ASSERT_OWNINGTHREAD(_class); */
>     delete (this);
>   }
> }
> 
> // DOMStorageCache
> 
> DOMStorageCache::DOMStorageCache(const nsACString* aScope)
>-: mManager(nullptr)
>-, mScope(*aScope)
>+: mScope(*aScope)
> , mMonitor("DOMStorageCache")
> , mLoaded(false)
> , mLoadResult(NS_OK)
> , mInitialized(false)
> , mSessionOnlyDataSetActive(false)
> , mPreloadTelemetryRecorded(false)
Hmm, not about this bug, but perhaps you could fix it here anyway. Please initialize all the bool member variables
in the ctor. At least mPersistent is missing.

> class DOMStorageUsageBridge
> {
> public:
>+  NS_IMETHOD_(nsrefcnt) AddRef(void);
>+  NS_IMETHOD_(nsrefcnt) Release(void);
>+
>   virtual ~DOMStorageUsageBridge() {}
> 
>   virtual const nsCString& Scope() = 0;
>   virtual void LoadUsage(const int64_t aUsage) = 0;
>+
>+protected:
>+  ThreadSafeAutoRefCnt mRefCnt;
>+  NS_DECL_OWNINGTHREAD

Please use the normal macros to implement refcounting.
NS_INLINE_DECL_THREADSAFE_REFCOUNTING should work.

Comment 12

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

Comment on attachment 809142 [details] [diff] [review]
v2

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

So both before and after this patch we keep a per-eTLD+1-limit of 5MB for localStorage.

But this patch changes sessionStorage such that it keeps a 5MB per-window limit? However this data is generally not written to disk (other than as part of session-restore?) so there is no fill-disk attack here.

And after this patch localStorage and sessionStorage never interfere with each other's quota (unclear from comments if that was the case before as well?)

If so, that sounds awesome!

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #12)
> Comment on attachment 809142 [details] [diff] [review]
> v2
> 
> Review of attachment 809142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks.

> So both before and after this patch we keep a per-eTLD+1-limit of 5MB for
> localStorage.

Yes, we do.

> 
> But this patch changes sessionStorage such that it keeps a 5MB per-window
> limit? 

Yes.

> However this data is generally not written to disk (other than as
> part of session-restore?) 

Exactly

> so there is no fill-disk attack here.

Absolutely :)

> 
> And after this patch localStorage and sessionStorage never interfere with
> each other's quota (unclear from comments if that was the case before as
> well?)

Yes!

> 
> If so, that sounds awesome!

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2.1 [as landed]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e5fe06a43c
https://hg.mozilla.org/integration/mozilla-inbound/rev/073deedc525d

(forgot to refresh...)

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d6e5fe06a43c
https://hg.mozilla.org/mozilla-central/rev/073deedc525d