Bug 88760 - [soup] Prevent setting or editing httpOnly cookies from JavaScript
Summary: [soup] Prevent setting or editing httpOnly cookies from JavaScript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
: 87247 (view as bug list)
Depends on:
Blocks: 87208
  Show dependency treegraph
 
Reported: 2012-06-11 00:20 PDT by Chris Dumez
Modified: 2012-06-14 05:56 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.49 KB, patch)
2012-06-11 01:35 PDT, Chris Dumez
gns: review-
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2012-06-11 11:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2012-06-12 11:39 PDT, Chris Dumez
gns: review+
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (9.98 KB, patch)
2012-06-12 12:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-06-11 00:20:54 PDT
Bug 86067 added http/tests/cookies/js-get-and-set-http-only-cookie.html test to make sure httpOnly cookies cannot be set, read or overwritten by JavaScript.
This test is failing for both GTK and EFL ports because the Soup-specific code allows setting/overwriting httpOnly cookies from JavaScript.
Comment 1 Chris Dumez 2012-06-11 01:35:26 PDT
Created attachment 146809 [details]
Patch
Comment 2 Dominik Röttsches (drott) 2012-06-11 02:48:01 PDT
*** Bug 87247 has been marked as a duplicate of this bug. ***
Comment 3 jochen 2012-06-11 05:42:39 PDT
Comment on attachment 146809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());

any reason for this change? It seems unrelated to the CL description
Comment 4 Chris Dumez 2012-06-11 05:46:54 PDT
Comment on attachment 146809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review

>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
>> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());
> 
> any reason for this change? It seems unrelated to the CL description

It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.
Comment 5 Chris Dumez 2012-06-11 05:53:05 PDT
(In reply to comment #4)
> (From update of attachment 146809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review
> 
> >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
> >> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());
> > 
> > any reason for this change? It seems unrelated to the CL description
> 
> It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.

Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally:
1. soup_cookie_parse(); // Useless since we already did it
2. policy check;
3. soup_cookie_jar_add_cookie();

I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1.

Hopefully, this is clearer.
Comment 6 jochen 2012-06-11 05:55:52 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 146809 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review
> > 
> > >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
> > >> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());
> > > 
> > > any reason for this change? It seems unrelated to the CL description
> > 
> > It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.
> 
> Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally:
> 1. soup_cookie_parse(); // Useless since we already did it
> 2. policy check;
> 3. soup_cookie_jar_add_cookie();
> 
> I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1.
> 
> Hopefully, this is clearer.

It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time?
Comment 7 Chris Dumez 2012-06-11 06:08:00 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 146809 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review
> > > 
> > > >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
> > > >> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());
> > > > 
> > > > any reason for this change? It seems unrelated to the CL description
> > > 
> > > It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.
> > 
> > Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally:
> > 1. soup_cookie_parse(); // Useless since we already did it
> > 2. policy check;
> > 3. soup_cookie_jar_add_cookie();
> > 
> > I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1.
> > 
> > Hopefully, this is clearer.
> 
> It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time?

kov/mrobinson: What's your preference? I don't really mind either way, it just seemed unfortunate to parse the Cookie twice.
Comment 8 Gustavo Noronha (kov) 2012-06-11 07:02:54 PDT
Comment on attachment 146809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82
> +    GOwnPtr<SoupCookie> newCookie(soup_cookie_parse(value.utf8().data(), origin.get()));

I think we may have a problem with API mismatch here. setCookies() implies value may contain more than one cookie, but all soup API we're using here only deals with the first cookie if there are many. We probably need a soup_cookie*s*_parse, and a soup_cookie_jar_set_cookie*s*_with_first_party().

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:87
> +    // set from JavaScript (Bug 86067).

I don't see value in mentioning this bug number in the code comments, the ChangeLog is enough.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92
> +    GOwnPtr<GSList> existingCookies(soup_cookie_jar_all_cookies(jar));

I think this is very inefficient, we should not be listing all cookies, how about using soup_cookie_jar_get_cookies(), so you can give it the URI?

>>>>>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
>>>>>> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());
>>>>> 
>>>>> any reason for this change? It seems unrelated to the CL description
>>>> 
>>>> It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.
>>> 
>>> Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally:
>>> 1. soup_cookie_parse(); // Useless since we already did it
>>> 2. policy check;
>>> 3. soup_cookie_jar_add_cookie();
>>> 
>>> I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1.
>>> 
>>> Hopefully, this is clearer.
>> 
>> It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time?
> 
> kov/mrobinson: What's your preference? I don't really mind either way, it just seemed unfortunate to parse the Cookie twice.

I prefer reliability and readability in this case. Unless there's a performance issue we need to fix I don't think the loss in readability and the risk of diverging is paid by the gain in not parsing the cookie twice. If you want to make this more efficient I'd suggest proposing new API for soup to set a cookie from an existing SoupCookie object.
Comment 9 Chris Dumez 2012-06-11 07:53:14 PDT
Comment on attachment 146809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review

By the way, the "1 vs several" cookie parsing is a separate issue (which was already there). Should I open another bug for it and use the current bug report only for prevent httpOnly cookie setting from JS?

>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82
>> +    GOwnPtr<SoupCookie> newCookie(soup_cookie_parse(value.utf8().data(), origin.get()));
> 
> I think we may have a problem with API mismatch here. setCookies() implies value may contain more than one cookie, but all soup API we're using here only deals with the first cookie if there are many. We probably need a soup_cookie*s*_parse, and a soup_cookie_jar_set_cookie*s*_with_first_party().

Agreed. Unfortunately, those APIs do not exist in libsoup yet.

>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92
>> +    GOwnPtr<GSList> existingCookies(soup_cookie_jar_all_cookies(jar));
> 
> I think this is very inefficient, we should not be listing all cookies, how about using soup_cookie_jar_get_cookies(), so you can give it the URI?

Well, I considered that first but the issue is that soup_cookie_jar_get_cookies() returns the cookies in a single gchar*. Then, I have currently no way of parsing more than one cookie from a gchar*.

>>>>>>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83
>>>>>>> -    soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data());
>>>>>> 
>>>>>> any reason for this change? It seems unrelated to the CL description
>>>>> 
>>>>> It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.
>>>> 
>>>> Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally:
>>>> 1. soup_cookie_parse(); // Useless since we already did it
>>>> 2. policy check;
>>>> 3. soup_cookie_jar_add_cookie();
>>>> 
>>>> I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1.
>>>> 
>>>> Hopefully, this is clearer.
>>> 
>>> It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time?
>> 
>> kov/mrobinson: What's your preference? I don't really mind either way, it just seemed unfortunate to parse the Cookie twice.
> 
> I prefer reliability and readability in this case. Unless there's a performance issue we need to fix I don't think the loss in readability and the risk of diverging is paid by the gain in not parsing the cookie twice. If you want to make this more efficient I'd suggest proposing new API for soup to set a cookie from an existing SoupCookie object.

Ok, I understand. I'll do that.
Comment 10 Chris Dumez 2012-06-11 10:48:55 PDT
Comment on attachment 146809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review

>>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92
>>> +    GOwnPtr<GSList> existingCookies(soup_cookie_jar_all_cookies(jar));
>> 
>> I think this is very inefficient, we should not be listing all cookies, how about using soup_cookie_jar_get_cookies(), so you can give it the URI?
> 
> Well, I considered that first but the issue is that soup_cookie_jar_get_cookies() returns the cookies in a single gchar*. Then, I have currently no way of parsing more than one cookie from a gchar*.

One other issue with soup_cookie_jar_get_cookies() is that it calls soup_cookie_to_cookie_header() for each cookie internally and not soup_cookie_to_set_cookie_header(). As a consequence, the returned cookies do not have a PATH set (only "NAME=VALUE"). However, I need the PATH (in addition to the NAME) in order to do an "overwrite" check.
Comment 11 Chris Dumez 2012-06-11 11:18:02 PDT
Created attachment 146878 [details]
Patch

Based on the feedback, I'm now proposing this patch. The patch now fixes setCookies() so that it processes all the cookies and not just the first one.
I have clearly documented with FIXME comments the methods that should be removed later and provided by libsoup instead (optimally).

However, I hope we can land this patch like this and improve the code later since this fixes a valid issue and getting a new and improved libsoup release may take time.
I don't mind writing the patches for libsoup though if it helps.
Comment 12 Early Warning System Bot 2012-06-11 11:25:10 PDT
Comment on attachment 146878 [details]
Patch

Attachment 146878 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12935768
Comment 13 Gustavo Noronha (kov) 2012-06-11 11:39:46 PDT
(In reply to comment #9)
> By the way, the "1 vs several" cookie parsing is a separate issue (which was already there). Should I open another bug for it and use the current bug report only for prevent httpOnly cookie setting from JS?

Yes, please, I just thought I'd mention it so we can all be aware of the issue.

> However, I hope we can land this patch like this and improve the code later since this fixes a valid issue and getting a new and improved libsoup release may take time.
> I don't mind writing the patches for libsoup though if it helps.

Like I said on IRC, I don't think there's a particular rush: we don't have any deadlines pressing us, and we can use our jhbuild infrastructure to bump the libsoup version as soon as new APIs are pushed. That said, if you want to go ahead I think your first patch is better than creating all those helper functions, modulo copying the policy check logic instead of calling soup's API we discussed.
Comment 14 Chris Dumez 2012-06-11 11:45:57 PDT
Comment on attachment 146878 [details]
Patch

Clearing flags. Will propose a new patch once libsoup is updated.
Comment 15 Chris Dumez 2012-06-12 01:32:57 PDT
Proposed the following libsoup patches to add the few API functions we need:
https://bugzilla.gnome.org/show_bug.cgi?id=677922
https://bugzilla.gnome.org/show_bug.cgi?id=677923

I don't really think we need a new soup_cookies_parse() function in libsoup since we can split on newlines and call soup_cookie_parse() for each cookie.
From libsoup point of view, such function makes little sense IMHO since it is not a header format (No "Set-Cookie:" or "Cookie:" at the beginning of the lines).
Comment 16 Mario Sanchez Prada 2012-06-12 03:14:12 PDT
Skipping http/tests/cookies/js-get-and-set-http-only-cookie.html in the GTK port:
http://trac.webkit.org/changeset/120052

Please unskip when fixed. Thanks!
Comment 17 Chris Dumez 2012-06-12 11:39:14 PDT
Created attachment 147124 [details]
Patch

Updated patch now that the libsoup patches landed.
Comment 18 Gustavo Noronha (kov) 2012-06-12 11:55:20 PDT
Comment on attachment 147124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147124&action=review

Since I have a small suggestion I'm not setting cq+, thanks, great work!

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:75
> +static inline SoupCookie* findCookieInList(const GSList* cookies, const gchar* name, const gchar* path)

I think you could make this helper contain the full logic, so instead of finding the cookie in the list and then verifying it's http-only at the caller you can instead have httpOnlyCookiExists(...).

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:113
> +        // Make sure we do not overwrite httpOnly cookies from JavaScript.
> +        SoupCookie* existingCookie = findCookieInList(existingCookies, soup_cookie_get_name(cookie.get()), soup_cookie_get_path(cookie.get()));
> +        if (existingCookie && soup_cookie_get_http_only(existingCookie))
> +            continue;

Would make this more readable =).
Comment 19 Chris Dumez 2012-06-12 12:13:32 PDT
Created attachment 147133 [details]
Patch

Take feedback into consideration. Could someone please cq+ ?
Comment 20 Gyuyoung Kim 2012-06-12 12:38:19 PDT
Comment on attachment 147133 [details]
Patch

Attachment 147133 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12940720
Comment 21 Chris Dumez 2012-06-12 12:41:08 PDT
Comment on attachment 147133 [details]
Patch

The ews efl did not take into consideration the libsoup update. False alarm. Marking as cq? again.
Comment 22 WebKit Review Bot 2012-06-12 17:33:21 PDT
Comment on attachment 147133 [details]
Patch

Clearing flags on attachment: 147133

Committed r120145: <http://trac.webkit.org/changeset/120145>
Comment 23 WebKit Review Bot 2012-06-12 17:33:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Sergio Villar Senin 2012-06-13 02:17:32 PDT
http/tests/misc/redirect-to-about-blank.html has started to fail after committing this.
Comment 25 Chris Dumez 2012-06-13 02:54:20 PDT
(In reply to comment #24)
> http/tests/misc/redirect-to-about-blank.html has started to fail after committing this.

Yes, I skipped the test for both GTK and EFL and I opened Bug 88961 to track the issue. This is caused by a regression in the new version of libsoup. It is actually unrelated to this fix but a side effect of the libsoup version bump in jhbuild.
Comment 26 Wajahat Siddiqui 2012-06-14 05:46:47 PDT
I am using a non jhbuild on ubuntu 12.04 to build webkit-gtk with options to autogen --enable-webkit2 && --with-gtk=3.0 and I get the following build error on svn rev# 120311

../Source/WebCore/platform/network/soup/CookieJarSoup.cpp: In function ‘void WebCore::setCookies(WebCore::Document*, const WebCore::KURL&, const WTF::String&)’:
../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:99:86: error: ‘soup_cookie_jar_get_cookie_list’ was not declared in this scope
../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:117:92: error: ‘soup_cookie_jar_add_cookie_with_first_party’ was not declared in this scope

I tried installing latest libsoup-2.39.2 but of no use, I could not find these APIs defined soup_cookie_jar_get_cookie_list & soup_cookie_jar_add_cookie_with_first_party in libsoup-2.39.2

Is something getting missed here?
Comment 27 Gustavo Noronha (kov) 2012-06-14 05:56:52 PDT
We're now using API from an unreleased version of soup. You need to build libsoup from git or wait for the next release.