Implement and test CanonicalCookie::IsCanonical()

net/cookies/canonical_cookie.cc

Index: net/cookies/canonical_cookie.cc
diff --git a/net/cookies/canonical_cookie.cc b/net/cookies/canonical_cookie.cc
index 6c81a55dc7df1d768618d9d76b338a0b0fd79974..95b1355b659012ee3238afe284a80d04f28666ca 100644
--- a/net/cookies/canonical_cookie.cc
+++ b/net/cookies/canonical_cookie.cc
@@ -50,10 +50,12 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "net/base/url_util.h"
 #include "net/cookies/cookie_util.h"
 #include "net/cookies/parsed_cookie.h"
 #include "url/gurl.h"
 #include "url/url_canon.h"
+#include "url/url_util.h"
 
 using base::Time;
 using base::TimeDelta;
@@ -238,11 +240,13 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::Create(
     return nullptr;
   }
 
-  return base::WrapUnique(new CanonicalCookie(
+  std::unique_ptr<CanonicalCookie> cc(base::WrapUnique(new CanonicalCookie(

[mmenke, 2017/06/09 18:25:49]

base::MakeUnique (pre-existing issue)

[Randy Smith (Not in Mondays), 2017/06/09 19:38:32]

Done.

       parsed_cookie.Name(), parsed_cookie.Value(), cookie_domain, cookie_path,
       creation_time, cookie_expires, creation_time, parsed_cookie.IsSecure(),
       parsed_cookie.IsHttpOnly(), parsed_cookie.SameSite(),
-      parsed_cookie.Priority()));
+      parsed_cookie.Priority())));
+  DCHECK(cc->IsCanonical());

[mmenke, 2017/06/09 18:25:49]

Random comment:  Come to think of it, we do have some fuzzer coverage of this
DCHECK, in the URLRequest fuzzer, though it can't set secure or extension
cookies.

[Randy Smith (Not in Mondays), 2017/06/09 19:38:33]

Acknowledged.

+  return cc;
 }
 
 bool CanonicalCookie::IsEquivalentForSecureCookieMatching(
@@ -399,6 +403,32 @@ bool CanonicalCookie::FullCompare(const CanonicalCookie& other) const {
   return Priority() < other.Priority();
 }
 
+bool CanonicalCookie::IsCanonical() const {
+  if (ParsedCookie::ParseTokenString(name_) != name_ ||
+      ParsedCookie::ParseValueString(value_) != value_ ||
+      ParsedCookie::ParseValueString(domain_) != domain_ ||
+      ParsedCookie::ParseValueString(path_) != path_) {

[mmenke, 2017/06/09 18:25:49]

I don't think this is enough.

In particular, disallow CRs/LFs and embedded NULLs, which all seems like they
could be rather problematic.  What happens if someone tries to set such cookies
via document.cookie?

[mmenke, 2017/06/09 19:00:47]

Also, may be worth testing if there are any paths where we let web platform
consumers set cookies with semi-colons or commas (or quotes?) in cookie name or
value strings.  

[Randy Smith (Not in Mondays), 2017/06/09 19:38:32]

See comments elsewhere about currently web platform consumers (Set-Cookie:
header and document.cookie) using SetCookieWithOptions() and hence going through
parsing.  But I'm not sure what you mean by "testing"?  How would one test that?

[Randy Smith (Not in Mondays), 2017/06/09 19:38:32]

Sorry, could you say a bit more?  I may be confused, but it looks to me as if
ParseTokenString(x) will indeed return something that != x if x contains CRs,
LFs, or nulls.   I've added a couple of tests in cc_unittest.cc to confirm this,
and cookies constructed with those characters in the name aren't canonical.

[mmenke, 2017/06/09 19:53:59]

If they go through SetCookieWithOptions, that's enough for me.  So yea, I think
we should ban ";", at least, in name and value (Not sure about commas or quotes
- depends on whether they're allowed in set-cookie lines).

One would test it by trying to set a cookie with the values, to see if it's
possible (Reading specs wouldn't be enough, given our....flexible relationship
to the spec).

[mmenke, 2017/06/09 19:53:59]

Oops, I missed the FindFirstTerminator call, and was digging into ParseToken
instead.

+    return false;
+  }
+
+  url::CanonHostInfo ignored;
+  std::string canonical_domain(CanonicalizeHost(domain_, &ignored));
+  // TODO(rdsmith): This specifically allows for empty domains.  This is
+  // arguable from the spec (the domain attribute may not be empty, but
+  // no specification is made as to the value of the cookie domain in
+  // the absence of a domain attribute), but are needed for chrome

[mmenke, 2017/06/09 18:25:49]

are -> is.  Also, should Chrome be capitalized, or be chrome/ instead?  Doesn't
really matter.

[Randy Smith (Not in Mondays), 2017/06/09 19:38:33]

Done.

+  // extension cookies.  See http://crbug.com/730633 for more information.
+  if (!url::HostIsIPAddress(domain_) && !domain_.empty() &&

[mmenke, 2017/06/09 18:25:49]

instead of url::HostIsIPAddress, can't we just grab this from
CanonHostInfo::IsIPAddress(), to save some work?

[Randy Smith (Not in Mondays), 2017/06/09 19:38:33]

Done.

+      canonical_domain != domain_) {

[mmenke, 2017/06/09 18:25:49]

If the URL is an IP address, shouldn't canonical_domain == domain_?

My only concern here is domains that can be expressed in multiple ways.  i.e.
[::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's).  Just
because domain_ is an IP address doesn't mean it can necessarily be part of a
canonical cookie.

[Randy Smith (Not in Mondays), 2017/06/09 19:38:32]

Is the issue what "canonical" means, i.e. that you could have two equivalent
hosts that are IP addresses and represented differently, and hence they wouldn't
look like the same cookie?  If so, yes, that's an issue, but it's an issue that
exists in the current code, and I'm not particularly inclined to shave the yak
unless you think it's specifically important?

[mmenke, 2017/06/09 19:53:59]

I'm not sure if canonicalizing domain names canonicalizes IPs or not.  This is
entirely dependent on what CanonicalizeHost does.  Just remove the IP address
check, and I think we're fine, regardless?

Hrm...It looks like CanonicalCookie deliberately skips canonicalization when the
domain is an IP address, sorry, I missed that, so this looks right.

My concern is that CanonicalCookie::Create may have a different notion of what's
canonical than this function.  Ideally, this would only say true iff such a
cookie could be created with CanonicalCookie::Create(), in my opinion.  Do you
have a different notion of what this should do?

[mmenke, 2017/06/09 20:05:04]

So...this is really weird:  CanonicalCookie::Create quite deliberately does not
canonicalize hosts that are IP address, so it would allow cookies to be set on a
domain of "[0::1]", but GURL *does* canonicalize domains.  So normally, it's
impossible to set a cookie on the domain "[0::1]", because it will be normalized
to "[::1]" by GURL.  However, this code will let cookies be set on the domain.

[Randy Smith (Not in Mondays), 2017/06/11 15:35:24]

That's an excellent clarifying question.  I think that the set of canonical
cookies as defined by IsCanonical() should be the set createable by
CanonicalCookie::Create closed over what happens when cookies get put into and
removed from the cookie store.  I'll take a stab at defining that more
precisely:
* Everything CC::Create can produce except
* Creation times may be arbitrary or null (that's looking forward; null means
"figure it out for me when entering it into the store")
* Access and modification times must be null or after creation times unless
creation time is null, in which case they must be null.

I'll take a stab at bringing IsCanonical() in line with the above (and dealing
with any test failures :-}), but I'd value your reaction to the definition above
in the meantime.

[Randy Smith (Not in Mondays), 2017/06/11 15:35:24]

Yeah, that fits with my understanding, and it sounds wrong to me, but I'm not
inclined to change it; it's a pre-existing to this CL wart.  Let me know if that
bothers you.

[mmenke, 2017/06/12 15:27:00]

I think that's reasonable, and should be documented.  Note that this would
include canonicalizing IP address host names, since CanonicalCookie::Create
takes a GURL, rather than a string hostname.

[mmenke, 2017/06/12 15:27:00]

I agree this CL shouldn't try to change behavior, or work around existing warts,
however, only allowing cookies to be set on normalized IP addresses *does* seem
to be existing behavior, just because CanonicalCookie::Create takes a GURL,
rather than a hostname.

[Randy Smith (Not in Mondays), 2017/06/13 15:06:02]

Ignore the above about modification times; they don't exist.  I am not planning
to put any constraints on expiry times; they can exist even if the creation time
is null (through use of expires rather than max age).
Just a quick note: As best I can tell, CanonicalCookie::Create() relies on
creation_time being non-null (see the call to CanonExpiration()) but does not
assert or document that it so relies.  I don't plan on trying to fix
CanonicalCookie::Create() in this CL, but I do plan to allow canonical cookies
to have times as noted above despite the slightly strange behavior of
CC::Create().  I don't like the inconsistency, but neither do I want to get
caught in too large a yak shave, and I think null creation times for CCs are a
fine thing, and could be useful--specifically, it allows only a single CC copy
on setting a CC on the cookie store while making sure that creation times are
unique within a cookie store.

[Randy Smith (Not in Mondays), 2017/06/13 20:37:35]

Huh.  Good point.  I think the only code modification required for this is to
remove the check above for HostIsIPAddress(); no matter what the domain must
equal the canonicalized domain (even if it's empty, since canonicalizing an
empty domain just returns an empty domain).  I'll put tests in for both these
cases. 

+    return false;
+  }
+
+  if (path_.empty() || path_[0] != '/')

[mmenke, 2017/06/09 18:25:49]

Hrm...I guess we do nothing about invalid characters anywhere in the cookie
code, unless I'm missing something?  So we depend on the site to do any
normalization when they specify a path (i.e., escape rules, like " " -> "%20)

[Randy Smith (Not in Mondays), 2017/06/09 19:38:32]

That sounds right to me, though if there's anything this exercise has taught me
it's that I'm not very well grounded in all the nuances of the cookie code.  
There's some protection on the path through which web sites set cookies (because
it uses SetCookieWithOptions(), which means the cookie code is parsing the
cookie line, and that parsing presumably doesn't something about invalid
characters).  And direct renderer setting of cookies uses the same interface (as
makes sense for setting through document.cookies).  

Which means that we're going to need to add that validation before we switch the
renderer pathway over to Mojo, since currently the parsing's done in the browser
and that'll move it to the renderer.  I'd rather do that in a separate CL (so
this is pretty purely a refactor of what we currently have), but I'll take a
note that it'll need to be done.

+    return false;
+
+  return true;

[mmenke, 2017/06/09 18:25:49]

What if there's no creation time, or the creation time is far in the future? 
The main concern is that may much with GC, or that a compromised renderer could
create a forever cookie, that will never expire, due to always passing the
30-day checks.

Of course, just dropping them could cause problems if the clock changes, or
clocks are off.  Are there cases where consumers really want to set a cookie
with a weird creation time?  If not, can just have the API to set a
CanonicalCookie overwrite the time itself.  I'm less concerned about funky
expiration times, since that's what GC is for, and that's based mostly on
creation time.

We'll presumably want to overwrite last_access_data.  The only other fields that
look to be of potential concern when set by a compromised renderer are domain
and secure, but those can already be set be such a renderer, so that's nothing
new.

[Randy Smith (Not in Mondays), 2017/06/09 19:38:32]

As noted above, I'd like to put in the Mojo-required renderer checks in a
separate CL that's more closely associated with the shift to mojo, and leave
this as being just a refactor of/based on the current code.  Is that reasonable,
or do you think that this CL is incomplete without those checks?
Some of the issues you're raising are handled in
https://codereview.chromium.org/2882063002; specifically, in that CL a null
creation time is taken as "set the creation time when you push the cookie into
the monster".  (And CC already has a method for overriding the last access
time.)

Historically, renderers (and the net) haven't had a way of setting creation
time, so my inclination is to have a specific check on reception of renderer
messages for a null creation time (in a later CL).  Let me know if you don't
think that addresses the set of issues you're concerned about.

ETA: Thinking about this from a slightly more general perspective, I don't think
that variations in creation time or last access time will affect whether a
cookie is *canonical*; it's that we want to have different privileges able to
set those values or not.  Which may mean that we'll want a null last access time
to have a specific ("Figure it out for me") meaning, and then assert that the
renderer must always provide a null last access time.

From that perspective, the validation around illegal characters that we talk
about above *should* be part of the definition of a canonical cookie, and if you
want I'm willing to expand this CL to take care of that if you'd like, though I
still think it's reasonable to have it in a different CL when the privilege
issue comes up (mostly because CookieMonster::SetCookieWithDetails() does
nothing in this space, and as I mention elsewhere, that's my initial model for
IsCanonical() and SetCanonicalCookie()).  

[mmenke, 2017/06/12 15:27:00]

I think that's fine.  Part of the trouble I'm having with this CL is the code
isn't even used, so I have to speculate on how it will be used (Or punt that
part of the review until it's actually used, which means I'll be reviewing a lot
of code that the CL doesn't actually modify, which seems kinda funky).
I think this is reasonable.  We should be very clear in function docs just what
this does and does not check, though.
I'd prefer to do it in this CL, mostly because I think it's easier to make sure
we have good testing of this method on a CL that just adds this method, rather
than in a CL that does other stuff, and it also highlights any issue with the
checks earlier, so we can fix them now, if need be, rather than having to deal
with that in a CL where changes here aren't the focus.

[mmenke, 2017/06/12 15:28:48]

Another option would be to land this largely as-is, and then fix it up in
another CL that does nothing else, which I'm also fine with.

[Randy Smith (Not in Mondays), 2017/06/13 15:06:02]

I see that as part of the basic philosophical tension between doing a lot of
small CLs vs. one big CL.  I'm happy to go with your preference, but I don't see
a way to provide cake for both having and eating :-} :-J.

I'll note that *some* of how this CL will be used (not the renderer checks,
which are down the line) will be occurring in
https://codereview.chromium.org/2882063002, and I'd actually be happy to duck
back and forth between the two CLs based on your prompting if you'd like to
review them in parallel.  Let me know if you want to do that, and I'll bring
them both to "reviewable level" before I next ping you on either.  

But for the renderer checks, yeah, I think you'll need to use your imagination
or review this code again when we get to those CLs (though I don't intend to put
any of the renderer specific checks in the main cookie code).  I'd suggest
thinking of this CL as if its consumers are all privileged, and we'll deal with
the unprivileged consumers later, and separately.
Yeah, I had basically come to this same conclusion, free-running on defining
IsCanonical() based on CC::Create().  Accepted.

[Randy Smith (Not in Mondays), 2017/06/13 15:06:02]

Not really any point; let's keep the CL conceptually coherent.  I've done enough
salami slicing, at least IMO.

+}
+
 // static
 CanonicalCookie::CookiePrefix CanonicalCookie::GetCookiePrefix(
     const std::string& name) {