Bug 85028 – [BlackBerry] Cookies should be checked during parsing to improve performance.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jason Liu]

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.

[Jason Liu]

Created attachment 139330 [details]
Patch

[Jason Liu]

Created attachment 139342 [details]
Patch

[Joe Mason]

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.

[Rob Buis]

Comment on attachment 139342 [details]
Patch

Based on Joe's comments, Jason needs to look into that, so r- for now.

[Jason Liu]

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

[Joe Mason]

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.

[Jason Liu]

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

[Joe Mason]

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.

[Jason Liu]

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

[Jason Liu]

Would you help me review it again?

[Joe Mason]

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

[George Staikos]

Comment on attachment 139342 [details]
Patch

I'm ok with it in principle but haven't reviewed it in detail.

[Jason Liu]

Would you approve this patch? :)
And this is blocking my another cookie patch.

[WebKit Review Bot]

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

[Jason Liu]

Created attachment 142209 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142209 [details]
Patch

Clearing flags on attachment: 142209

Committed r117261: <http://trac.webkit.org/changeset/117261>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.