Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: bypass DOM storage quota #15596

Merged
merged 4 commits into from Nov 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion patches/common/chromium/.patches
Expand Up @@ -11,7 +11,7 @@ browser_plugin_wheel.patch
can_create_window.patch
compositor_delegate.patch
disable_hidden.patch
dom_storage_map.patch
dom_storage_limits.patch
frame_host_manager.patch
net_url_request_job.patch
no_stack_dumping.patch
Expand Down
98 changes: 98 additions & 0 deletions patches/common/chromium/dom_storage_limits.patch
@@ -0,0 +1,98 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jacob Quant <jacobq@gmail.com>
Date: Tue, 6 Nov 2018 15:26:00 -0600
Subject: dom_storage_limits.patch

This patch circumvents the restriction on DOM storage objects,
namely `localStorage` and `sessionStorage`, which chromium otherwise
limits to approximately 10MiB.

That restriction originates from a recommendation
[in the Web Storage API specification](https://html.spec.whatwg.org/multipage/webstorage.html#disk-space-2)
that is motivated by the concern that hostile code could abuse this
feature to exhaust available storage capacity.
However, in the case of Electron, where the application developers
have control over all of the code being executed,
this safety precaution becomes a hindrance that does not add much value.
For example, if a malicious developer wanted to consume disk space
on a victim's machine they could do so via Node's native file system API.

By disabling this restriction or increasing the quota,
electron application developers can use `localStorage`
as their application's "back end", without being having
to limit the amount of data stored to 10MiB.

There may still be some benefit to keeping this restriction for applications that load remote content.
Although all remote data should be from a trusted source and transferred using
a secure channel, it is nevertheless advisable to include additional layers of protection
to mitigate risks associated with potential compromise of those other technologies.
With that in mind, an acceptable alternative to disabling the limit at compile-time
(as this patch currently does) would be to instead allow it to be disabled at run-time
for a given `BrowserWindow` via a `webPreferences` option,
similar to [`nodeIntegration`](https://electronjs.org/docs/tutorial/security#2-disable-nodejs-integration-for-remote-content).

diff --git a/content/common/dom_storage/dom_storage_map.cc b/content/common/dom_storage/dom_storage_map.cc
index fd088fb170be..b90b6cf9132d 100644
--- a/content/common/dom_storage/dom_storage_map.cc
+++ b/content/common/dom_storage/dom_storage_map.cc
@@ -185,10 +185,12 @@ bool DOMStorageMap::SetItemInternal(MapType* map_type,
size_t new_item_size = size_in_storage(key, value);
size_t new_storage_used = storage_used_ - old_item_size + new_item_size;

+#if 0
// Only check quota if the size is increasing, this allows
// shrinking changes to pre-existing files that are over budget.
if (new_item_size > old_item_size && new_storage_used > quota_)
return false;
+#endif

(*map_type)[key] = value;
ResetKeyIterator();
diff --git a/content/common/dom_storage/dom_storage_types.h b/content/common/dom_storage/dom_storage_types.h
index e87afe5b8ee0..61c9a0dfff60 100644
--- a/content/common/dom_storage/dom_storage_types.h
+++ b/content/common/dom_storage/dom_storage_types.h
@@ -21,6 +21,7 @@ typedef std::map<base::string16, base::NullableString16> DOMStorageValuesMap;

// The quota for each storage area.
// This value is enforced in renderer processes and the browser process.
+// However, Electron's dom_storage_limits.patch removes the code that checks this limit.
const size_t kPerStorageAreaQuota = 10 * 1024 * 1024;

// In the browser process we allow some overage to
diff --git a/content/renderer/dom_storage/dom_storage_cached_area.cc b/content/renderer/dom_storage/dom_storage_cached_area.cc
index 402c27727ff1..f5908a1c55f9 100644
--- a/content/renderer/dom_storage/dom_storage_cached_area.cc
+++ b/content/renderer/dom_storage/dom_storage_cached_area.cc
@@ -53,11 +53,13 @@ bool DOMStorageCachedArea::SetItem(int connection_id,
const base::string16& key,
const base::string16& value,
const GURL& page_url) {
+#if 0
// A quick check to reject obviously overbudget items to avoid
// the priming the cache.
if ((key.length() + value.length()) * sizeof(base::char16) >
kPerStorageAreaQuota)
return false;
+#endif

PrimeIfNeeded(connection_id);
base::NullableString16 old_value;
diff --git a/content/renderer/dom_storage/local_storage_cached_area.cc b/content/renderer/dom_storage/local_storage_cached_area.cc
index ffa21c9200d0..0edbd6152292 100644
--- a/content/renderer/dom_storage/local_storage_cached_area.cc
+++ b/content/renderer/dom_storage/local_storage_cached_area.cc
@@ -141,11 +141,13 @@ bool LocalStorageCachedArea::SetItem(const base::string16& key,
const base::string16& value,
const GURL& page_url,
const std::string& storage_area_id) {
+#if 0
// A quick check to reject obviously overbudget items to avoid priming the
// cache.
if ((key.length() + value.length()) * sizeof(base::char16) >
kPerStorageAreaQuota)
return false;
+#endif

EnsureLoaded();
bool result = false;
23 changes: 0 additions & 23 deletions patches/common/chromium/dom_storage_map.patch

This file was deleted.

14 changes: 14 additions & 0 deletions spec/chromium-spec.js
Expand Up @@ -937,6 +937,20 @@ describe('chromium feature', () => {
})

describe('storage', () => {
describe('DOM storage quota override', () => {
['localStorage', 'sessionStorage'].forEach((storageName) => {
alexeykuzmin marked this conversation as resolved.
Show resolved Hide resolved
it(`allows saving at least 50MiB in ${storageName}`, () => {
const storage = window[storageName]
const testKeyName = '_electronDOMStorageQuotaOverrideTest'
// 25 * 2^20 UTF-16 characters will require 50MiB
const arraySize = 25 * Math.pow(2, 20)
storage[testKeyName] = new Array(arraySize).fill('X').join('')
expect(storage[testKeyName]).to.have.lengthOf(arraySize)
jacobq marked this conversation as resolved.
Show resolved Hide resolved
delete storage[testKeyName]
})
})
})

it('requesting persitent quota works', (done) => {
navigator.webkitPersistentStorage.requestQuota(1024 * 1024, (grantedBytes) => {
assert.strictEqual(grantedBytes, 1048576)
Expand Down