Bug 88760 – [soup] Prevent setting or editing httpOnly cookies from JavaScript

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 146809 [details]
Patch

[Dominik Röttsches (drott)]

*** Bug 87247 has been marked as a duplicate of this bug. ***

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

[Chris Dumez]

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.

[Chris Dumez]

(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.

(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?

[Chris Dumez]

(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.

[Gustavo Noronha (kov)]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Early Warning System Bot]

Comment on attachment 146878 [details]
Patch

Attachment 146878 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12935768

[Gustavo Noronha (kov)]

(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.

[Chris Dumez]

Comment on attachment 146878 [details]
Patch

Clearing flags. Will propose a new patch once libsoup is updated.

[Chris Dumez]

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).

[Mario Sanchez Prada]

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!

[Chris Dumez]

Created attachment 147124 [details]
Patch

Updated patch now that the libsoup patches landed.

[Gustavo Noronha (kov)]

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 =).

[Chris Dumez]

Created attachment 147133 [details]
Patch

Take feedback into consideration. Could someone please cq+ ?

[Gyuyoung Kim]

Comment on attachment 147133 [details]
Patch

Attachment 147133 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12940720

[Chris Dumez]

Comment on attachment 147133 [details]
Patch

The ews efl did not take into consideration the libsoup update. False alarm. Marking as cq? again.

[WebKit Review Bot]

Comment on attachment 147133 [details]
Patch

Clearing flags on attachment: 147133

Committed r120145: <http://trac.webkit.org/changeset/120145>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sergio Villar Senin]

http/tests/misc/redirect-to-about-blank.html has started to fail after committing this.

[Chris Dumez]

(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.

[Wajahat Siddiqui]

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?

[Gustavo Noronha (kov)]

We're now using API from an unreleased version of soup. You need to build libsoup from git or wait for the next release.