New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 730633 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Storage for extensions cookies is weird, non-performant, and possibly has a security problem

Project Member Reported by rdsmith@chromium.org, Jun 7 2017

Issue description

As a result of some (mostly) wild goose chases debugging for http://codereview.chromium.org/2898953008 I've realized that cookies created with non-canonical schemes (anything other than file:, filesystem:, http:, https:, mailto:, ftp:, ws:, wss:, http-so:,https-so:; see DoCanonicalize in url/url_util.cc) from URLs without an explicit "domain" field won't have a Domain() field (everything after the // is put in the path), and as a result will all be stored using the same, null, key in the cookie monster.  

This has performance implications for storage and lookup of chrome-extensions: cookies (though I don't think that's particularly important), and *might* have security implications.  I did a nihinihi manual test by inspecting the backing pages of a couple of extensions I have running (no cookies set in document.cookie), and set cookies in one and did not see it in others.  So the obvious pathway for leakage between extensions doesn't seem to work; presumably there's some path based filter on it.  But I don't have the depth of understanding of the "extensions cookies" path to be sure that there isn't a problem here.  

I'm filing this bug to track digging down to the floor on this one, but will not be pursuing it with high priority.  If someone wants to take it from me, feel free.  I'll remove the security permission when I've either decided there's no chance of leakage of cookie values between extensions or gotten a security review :-}.



 
Even if there is no way to access the cookies between extensions, that would mean a single per-host cookie limit is applied to cookies from all extensions, which seems rather problematic.
Cc: reillyg@chromium.org nhar...@chromium.org pwnall@chromium.org
Owner: ----
Status: Available (was: Assigned)
Removing myself as owner, adding other possibly interested cookie parties.

Comment 3 by mmenke@chromium.org, Apr 18 2018

Status: Fixed (was: Available)
Project Member

Comment 4 by sheriffbot@chromium.org, Apr 19 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
mmenke@, can you add an explanation of why this is now considered fixed?

Comment 6 by mmenke@chromium.org, Apr 19 2018

Labels: Network-Triaged
Status: Available (was: Fixed)
We don't allow cookies to be set with any non-canonical scheme, other than the extensions scheme, but that's just for extensions that call document.cookie, and uses its own cookie store.  Hrm....I may have misunderstood the description.
Status: Fixed (was: Available)
First of all, the original report is mistaken about the key used in the database:
$ sqlite3 ~/.config/chromium/Default/Extension\ Cookies -cmd "SELECT host_key FROM cookies;"
aknmeieabphheepdkibhjbpoagohaido

Now the key used by SQLitePersistentCookieStore and CookieMonster was inconsistent, with one being an empty string, indeed, but that got fixed in https://chromium-review.googlesource.com/c/chromium/src/+/940141, standardizing on GetKey which handles extension stuff distinctly:
https://cs.chromium.org/chromium/src/net/cookies/cookie_monster_unittest.cc?rcl=d223f8bf342614d9831f5c8534ddfa438d373dcb&l=2002
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 27

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment