Bug 85028 - [BlackBerry] Cookies should be checked during parsing to improve performance.
Summary: [BlackBerry] Cookies should be checked during parsing to improve performance.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-26 20:24 PDT by Jason Liu
Modified: 2012-05-16 04:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.53 KB, patch)
2012-04-27 20:39 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2012-04-27 23:39 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (8.46 KB, patch)
2012-05-16 02:57 PDT, Jason Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Liu 2012-04-26 20:24:28 PDT
If a cookie coming in has a domain with domain="opera.com, the domain of the
cookie will be saved in memory (and database) as '"opera.com'. The cookie will
never be used because '"' is never used in a request domain.

The expected behavior is to throw out the cookie immediately.
Comment 1 Jason Liu 2012-04-27 20:39:14 PDT
Created attachment 139330 [details]
Patch
Comment 2 Jason Liu 2012-04-27 23:39:40 PDT
Created attachment 139342 [details]
Patch
Comment 3 Joe Mason 2012-04-30 07:52:15 PDT
The approach looks good.

size_t dotPosition = parsedValue.find(".", 1);

That looks wrong, though. The original code skipped this check if the domain did not start with a dot.  So originally we had (where x is any character that isn't a dot):

.xxxxx - accepted
.xx.xx - accepted
xxxxxx - REJECTED
xxx.xx - accepted

This is the same as searching for the dot from position 0, not 1.

Also we have lost this check:

162  // ignore domain security if protocol doesn't have domains
163  if (shouldIgnoreDomain(cookie->protocol()))
164      return false;
165 
166  // Reject Cookie if domain is empty
167  if (!cookie->domain().length())
168      return true; 

We should check this in the parser, after the switch statement when both protocol and domain have been read.
Comment 4 Rob Buis 2012-05-04 12:14:17 PDT
Comment on attachment 139342 [details]
Patch

Based on Joe's comments, Jason needs to look into that, so r- for now.
Comment 5 Jason Liu 2012-05-06 18:52:40 PDT
(In reply to comment #3)
> The approach looks good.
> 
> size_t dotPosition = parsedValue.find(".", 1);
> 
> That looks wrong, though. The original code skipped this check if the domain did not start with a dot.  So originally we had (where x is any character that isn't a dot):
> 
> .xxxxx - accepted
> .xx.xx - accepted
> xxxxxx - REJECTED
> xxx.xx - accepted
> 
> This is the same as searching for the dot from position 0, not 1.

We check if the domain contains an embedded dot. 
And originally we had:
 .xxxxx - rejected   not accepted
 .xx.xx - accepted
 xxxxxx - REJECTED
 xxx.xx - accepted
So this is the same as searching for the dot from 1, not 0.

> Also we have lost this check:
> 
> 162  // ignore domain security if protocol doesn't have domains
> 163  if (shouldIgnoreDomain(cookie->protocol()))
> 164      return false;
> 165
If the protocol doesn't have domains, the code path won't contain domain parsing. So drop this "if" check in parsing.
 
> 166  // Reject Cookie if domain is empty
> 167  if (!cookie->domain().length())
> 168      return true; 
> 

size_t dotPosition = realDomain.find(".", 1); This is including length check.
Comment 6 Joe Mason 2012-05-07 08:16:31 PDT
In the original code:

if (cookie->domain()[0] == '.') {
    // Check if the domain contains an embedded dot.
    size_t dotPosition = cookie->domain().find(".", 1);
    if (dotPosition == notFound || dotPosition == cookie->domain().length()) {
        LOG_ERROR("Cookie %s is rejected because its domain does not contain an embedded dot.\n", cookie->toString().utf8().data());
        return true;
    }
}

// The request host should domain match the Domain attribute.
// Domain string starts with a dot, so a.b.com should domain match .a.b.com.
// add a "." at beginning of host name, because it can handle many cases such as
// a.b.com matches b.com, a.b.com matches .B.com and a.b.com matches .A.b.Com
// and so on.
String hostDomainName = url.host();
hostDomainName = hostDomainName.startsWith(".") ? hostDomainName : "." + hostDomainName;
if (!hostDomainName.endsWith(cookie->domain(), false)) {
    LOG_ERROR("Cookie %s is rejected because its domain does not domain match the URL %s\n", cookie->toString().utf8().data(), url.string().utf8().data());
    return true;
}

// We should check for an embedded dot in the portion of string in the host not in the domain
// but to match firefox behaviour we do not.

return false; 


So for .xxxxx, cookie->domain()[0] is '.' and it skips the first "if" statement entirely.  Then the second "if" statement looks only at url.host() so the dot position in the domain doesn't matter.  Then it returns false, meaning "do not reject this cookie".

Am I reading that wrong?


The new code does:

// Check if the domain contains an embedded dot.
size_t dotPosition = parsedValue.find(".", 1);
if (dotPosition == notFound || dotPosition == parsedValue.length())
    LOG_AND_DELETE("Invalid cookie %s (domain): it does not contain an embedded dot", cookie.ascii().data()); 

So for ".xxxxx", dotPosition is notFound since it's starting at position 1 and not position 0.  Which means it marks the cookie invalid and deletes it.


I'm assuming the old code is correct.  If not, we should file a different PR to fix the issue since this is a performance patch and should not cause any behaviour differences.
Comment 7 Jason Liu 2012-05-08 02:27:14 PDT
(In reply to comment #6)
> In the original code:
> 
> if (cookie->domain()[0] == '.') {
>     // Check if the domain contains an embedded dot.
>     size_t dotPosition = cookie->domain().find(".", 1);
>     if (dotPosition == notFound || dotPosition == cookie->domain().length()) {
>         LOG_ERROR("Cookie %s is rejected because its domain does not contain an embedded dot.\n", cookie->toString().utf8().data());
>         return true;
>     }
> }
> 
> // The request host should domain match the Domain attribute.
> // Domain string starts with a dot, so a.b.com should domain match .a.b.com.
> // add a "." at beginning of host name, because it can handle many cases such as
> // a.b.com matches b.com, a.b.com matches .B.com and a.b.com matches .A.b.Com
> // and so on.
> String hostDomainName = url.host();
> hostDomainName = hostDomainName.startsWith(".") ? hostDomainName : "." + hostDomainName;
> if (!hostDomainName.endsWith(cookie->domain(), false)) {
>     LOG_ERROR("Cookie %s is rejected because its domain does not domain match the URL %s\n", cookie->toString().utf8().data(), url.string().utf8().data());
>     return true;
> }
> 
> // We should check for an embedded dot in the portion of string in the host not in the domain
> // but to match firefox behaviour we do not.
> 
> return false; 
> 
> 
> So for .xxxxx, cookie->domain()[0] is '.' and it skips the first "if" statement entirely.  Then the second "if" statement looks only at url.host() so the dot position in the domain doesn't matter.  Then it returns false, meaning "do not reject this cookie".
> 
> Am I reading that wrong?
> 
In the original code:
if (cookie->domain()[0] == '.') {

For .xxxxx, it won't skip this if statement.
it will go into this if branch and rejected by the following codes.

// Check if the domain contains an embedded dot.
size_t dotPosition = cookie->domain().find(".", 1);
if (dotPosition == notFound || dotPosition == cookie->domain().length())  {
   LOG_ERROR("Cookie %s is rejected because its domain does not contain an embedded dot.\n", cookie->toString().utf8().data());
   return true;
}

So there is no change in behaviour.
Comment 8 Joe Mason 2012-05-08 07:01:12 PDT
Wow.  How did I read that so wrong?

Ok, so the new code has the opposite problem.  With the old code:

.xxxxx - REJECTED
.xx.xx - accepted
xxxxxx - accepted
xxx.xx - accepted

With the new code:

.xxxxx - REJECTED
.xx.xx - accepted
xxxxxx - REJECTED
xxx.xx - accepted

The old code only did the check when the domain started with a dot, but the new code always does it.
Comment 9 Jason Liu 2012-05-08 18:58:36 PDT
(In reply to comment #8)
> Wow.  How did I read that so wrong?
> 
> Ok, so the new code has the opposite problem.  With the old code:
> 
> .xxxxx - REJECTED
> .xx.xx - accepted
> xxxxxx - accepted
> xxx.xx - accepted
> 
> With the new code:
> 
> .xxxxx - REJECTED
> .xx.xx - accepted
> xxxxxx - REJECTED
> xxx.xx - accepted
> 
> The old code only did the check when the domain started with a dot, but the new code always does it.

(In reply to comment #8)
> Wow.  How did I read that so wrong?
> 
> Ok, so the new code has the opposite problem.  With the old code:
> 
> .xxxxx - REJECTED
> .xx.xx - accepted
> xxxxxx - accepted
> xxx.xx - accepted
> 
> With the new code:
> 
> .xxxxx - REJECTED
> .xx.xx - accepted
> xxxxxx - REJECTED
> xxx.xx - accepted
> 
> The old code only did the check when the domain started with a dot, but the new code always does it.

CookieParser::parseOneCookie
{
...
case 'D':
case 'd' : {
...
// If the domain does not start with a dot, add one for security checks, 	
// For example: ab.c.com dose not domain match b.c.com;
String realDomain = parsedValue[0] == '.' ? parsedValue : "." + parsedValue;
...
}

These codes will add '.' for xxxx.
So xxxx becomes .xxxx before checking in the old code. And then rejected.

The new code will check xxxx directly and drop it.

So there is no change in behaviour for xxxx.
Comment 10 Jason Liu 2012-05-11 00:26:13 PDT
Would you help me review it again?
Comment 11 Joe Mason 2012-05-11 09:17:03 PDT
(In reply to comment #10)
> Would you help me review it again?

In case anyone was waiting for to weigh in, Jason's convinced me.  The actual reviewer should double-check, though.
Comment 12 George Staikos 2012-05-12 11:14:45 PDT
Comment on attachment 139342 [details]
Patch

I'm ok with it in principle but haven't reviewed it in detail.
Comment 13 Jason Liu 2012-05-15 03:16:17 PDT
Would you approve this patch? :)
And this is blocking my another cookie patch.
Comment 14 WebKit Review Bot 2012-05-15 23:37:18 PDT
Comment on attachment 139342 [details]
Patch

Rejecting attachment 139342 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ED at 138.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/blackberry/CookieManager.cpp.rej
patching file Source/WebCore/platform/blackberry/CookieManager.h
patching file Source/WebCore/platform/blackberry/CookieParser.cpp
Hunk #1 succeeded at 230 (offset 5 lines).
Hunk #2 succeeded at 249 (offset 5 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'George Sta..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12709325
Comment 15 Jason Liu 2012-05-16 02:57:41 PDT
Created attachment 142209 [details]
Patch
Comment 16 WebKit Review Bot 2012-05-16 04:42:29 PDT
Comment on attachment 142209 [details]
Patch

Clearing flags on attachment: 142209

Committed r117261: <http://trac.webkit.org/changeset/117261>
Comment 17 WebKit Review Bot 2012-05-16 04:42:35 PDT
All reviewed patches have been landed.  Closing bug.