Dawid Czagan Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.2; WOW64; Trident/6.0; .NET4.0E; .NET4.0C; .NET CLR 3.5.30729; .NET CLR 2.0.50727; .NET CLR 3.0.30729; McAfee; MAARJS)

Steps to reproduce:

Cookie with secure flag can be overwritten by HTTP response. This way an insecure HTTP response can have an impact on secure HTTPS connection. Exemplary consequences:
1) switching a user to the attacker's account (by setting the session ID of the attacker). The user thinks that he is using his own account, but enters some sensitive information to the attacker's account as a result of this attack.
2) Session fixation via secure cookie (by setting the session ID of the attacker's choice). It will work when there is no session regeneration in the application after successful log in. Finally the attacker can impersonate the user, because the attacker knows the user's session ID.





Expected results:

Conclusion: There is no reason to allow overwriting of cookies with secure flag by HTTP response. However, this overwriting is possible in Firefox (tested with Firefox 27.0.1) and the exemplary aforementioned attacks are possible. Fix proposal - check if HTTP response tries to overwrite the cookie with secure flag (when this is a case, then the attempt is rejected).

Regards,
Dawid Czagan

	Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]
	Comment 1 • 10 years ago

So sites are allowed to modify their cookies over whatever type of connection and not letting sites remove the secure flag does break things. We tried being strict and it didn't work out. And you can't "remove the flag".... you can create a new cookie without the flag that replaces the old cookie. So I'm a bit confused on the actual attack here, any host can set "domain" cookies for sibling or parent hosts potentially. It's unclear from this report exactly what is happening to change the cookie setting. Is it the same domain or a different domain, as those 2 scenarios have vastly different implications.

	Dawid Czagan Reporter
	Comment 2 • 10 years ago

Let's assume that the site is working over HTTPS and the authentication cookie is PHPSESSID with secure flag set. Moreover let's assume, that there is also HTTP version of the site. The attacker makes a victim to send a request to HTTP version of the site. If the attacker controls the communication channel, then he can inject a header Set-Cookie: in HTTP response. It turns out, that the attacker can place a new value of PHPSESSID in this HTTP response and it will overwrite the value of PHPSESSID cookie with secure flag. This way HTTP response can overwrite cookies with secure flag (HTTP traffic has an impact on HTTPS traffic), which can lead to the exemplary aforementioned attacks.

	Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]
	Comment 3 • 10 years ago

Upon doing some more research this issue was originally brought up by Chris Evans in 2008 and called Cookie Forcing http://scarybeastsecurity.blogspot.com/2008/11/cookie-forcing.html.
As well this appears to be a well known issue with the current spec https://code.google.com/p/browsersec/wiki/Part2#Same-origin_policy_for_cookies (see the table, line Can HTTP pages clobber secure cookies?*). This is all good reasons why HSTS should be used by sites. And until cookies are replaced this is going to be an issue.

Since this is well known I am removing the security flag.

	Daniel Stenberg [:bagder]
	Comment 4 • 9 years ago

Related discussion on the chrome side: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/IU5t6eLuS2Y/Uq-7Kat9BwAJ

	Mike West
	Comment 5 • 9 years ago

There's also https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone, which is more directly related to this topic.

	Mike West
	Comment 6 • 8 years ago

The HTTP WG has picked up the draft I mentioned above as https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone/, and it's on track for an update to the cookie RFC. Chrome is in the process of shipping that behavior. It would be lovely to see Mozilla ship an implementation as well. :)

	Daniel Veditz [:dveditz]
	Comment 7 • 8 years ago

We're interested in trying this (for the reasons scarybeast pointed out long ago) but concerned about breakage. We'll have to measure the heck out of it.

Currently on Firefox 47 release (where telemetry is opt-in) we're seeing 0.04% of cookie setting being set insecurely with the "secure" flag. On Beta 48 where telemetry is opt-out we're seeing 0.13%. That sounds small but it's 100-250 million cookies (over a month).

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-07-21&keys=__none__!__none__!__none__&max_channel_version=beta%252F48&measure=COOKIE_SCHEME_SECURITY&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-06-21&table=0&trim=1&use_submission_date=0

Hard to know what kind of breakage this will cause. Will probably break specific sites (making a large number of users unhappy) but not affect most users. If we can identify those sites we might be able to (eventually) talk them into changing their code.

	Amy Chung [:Amy] Assignee
	Comment 8 • 8 years ago

Hi Josh,
Please help me to review my patch.
My modifications as below:
1. Added three security conditions according to the spec https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone/.
   i.   spec 3.1: ignore http host to set secure flag.
   ii.  spec 3.2: if user wants to modify the cookie that already existed and be set security flag, the new cookie can't set by didn't set "secure-only-flag"
and the "scheme" component of the "request-uri" does not denote a "secure" protocol.
   iii. spec 3.3: Modified the algorithm for removing cookie when cookie set more than 150 on same host name.
2. Added telemetry on 3.1 & 3.2.
3. Added preference id.
4. Added test cases:
   i. for 3.1 and 3.2.
+------------+-------------------+----------------------------+
|            | Set security flag |   Modify the cookie value  |
+            +                   +----------------------------+
|            |                   | same path | different path |
+------------+-------------------+-----------+----------------+
|  http host |         F         |     F     |        F       |
+------------+-------------------+-----------+----------------+
| https host |         T         |     T     |        T       |
+------------+-------------------+-----------+----------------+ 
      ii. for 3.3: created 60 cookies. first, create 49 security cookies, then
          create 11 non-security cookies.
          In TestCookie.cpp, set CookiesMaxPerHost to 50, and only the latest
          non-security cookie will be saved the other non-security cookies would
          be to remove.
 

Thanks!

	Daniel Veditz [:dveditz]
	Comment 10 • 8 years ago

Comment on attachment 8779299 [details] [diff] [review]
Added three security conditions according to the spec.

Review of attachment 8779299 [details] [diff] [review]:
-----------------------------------------------------------------

Good start but needs some tweaks.

::: browser/app/profile/firefox.js
@@ +1083,5 @@
>  pref("services.sync.prefs.sync.network.cookie.cookieBehavior", true);
>  pref("services.sync.prefs.sync.network.cookie.lifetimePolicy", true);
>  pref("services.sync.prefs.sync.network.cookie.lifetime.days", true);
>  pref("services.sync.prefs.sync.network.cookie.thirdparty.sessionOnly", true);
> +pref("services.sync.prefs.sync.network.cookie.security.enabled", true);

I don't think we need to sync this one. We're using it as a way to roll out the feature or revert it if we encounter unexpected problems after release. We don't expect even advanced users to be fiddling with it.

::: modules/libpref/init/all.js
@@ +1903,5 @@
>  #ifdef ANDROID
>  pref("network.cookie.cookieBehavior",       0); // Keep the old default of accepting all cookies
>  #endif
>  pref("network.cookie.thirdparty.sessionOnly", false);
> +pref("network.cookie.security.enabled", false);

A longer pref name would be more explanatory, for example network.cookie.leave-secure-alone.enabled (from the original spec title). Otherwise it invites misunderstanding "Of course I want cookie security!" or worse "Why is Firefox shipping with security disabled?!"

We should default to true, and not land until it's good enough for Nightly audiences. The false setting is so we can turn it off if worst comes to worst, or if we want to hold the feature to Dev Edition or Beta (in which case we need #ifdefs).

We want to eventually get rid of this pref, maybe a release or two after we ship and are satisfied it's not breaking too much.

::: netwerk/cookie/nsCookieService.cpp
@@ +702,5 @@
>  nsCookieService::nsCookieService()
>   : mDBState(nullptr)
>   , mCookieBehavior(nsICookieService::BEHAVIOR_ACCEPT)
>   , mThirdPartySession(false)
> + , mSecurityEnabled(false)

Go ahead and default to true. I have a similar name nit here, something like mLeaveSecureAlone is better (something close to the pref name). No need to put "enabled" in the name, just pick a name that makes it clear what "true" means.

@@ +3337,5 @@
>      return;
>    }
>  
> +  bool isHTTPS = false;
> +  if ( aHostURI != nullptr) {

You could simplify this code a bit by checking "insecure site can't set 'secure' cookie" first and separately.

// a cookie with the secure-only-flag must be set by a secure site
if (aCookie->IsSecure() && mLeaveSecureAlone) {
  bool isHTTPS = false;
  if (aHostURI == nullptr
      || NS_FAILED(aHostURI->SchemeIs("https",&isHTTPS))
      || !isHTTPS) {
     COOKIE_LOGFAILURE ...
     Telemetry ...
     return;
  }
}
(obviously formatting is screwy here). The point is to get the 3.1 checks out of the way first rather than have to check it on each side of the "if(foundCookie)/else" structure.

@@ +3346,3 @@
>    nsListIter matchIter;
>    bool foundCookie = FindCookie(aKey, aCookie->Host(),
>      aCookie->Name(), aCookie->Path(), matchIter);

This is going to be a problem -- 3.2 of the spec explicitly ignores path, and FindCookie() only matches exact paths. I think we'll need a new Find function, or at least a secure-cookie-exists function because you don't need to return the cookie.

e.g. if (mLeaveSecureAlone && !aCookie->isSecure()
         && FindSecure(aKey, host, name)) {
        // log, telemetry, return
      }

But that scans twice, so maybe you want to add another out param to FindCookie() that can return a true/false name/domain match even if FindCookie() itself didn't find an exact match.

@@ +3432,5 @@
>      }
>  
>    } else {
> +    // If the new cookie is non-https and wants to set sercure flag,
> +    // browser have to ignore this new cookie.

You don't need this if you move the 3.1 checks before the FindCookie() bit

@@ +3450,5 @@
>      // check if we have to delete an old cookie.
>      nsCookieEntry *entry = mDBState->hostTable.GetEntry(aKey);
>      if (entry && entry->GetCookies().Length() >= mMaxCookiesPerHost) {
>        nsListIter iter;
> +      FindStaleCookie(entry, currentTime, iter, mSecurityEnabled);

i wonder why FindStaleCookie() is static and FindCookie() isn't? If we remove static you wouldn't have to pass in mSecurityEnabled (or mLeaveSecureAlone as I prefer). Ultimately if the spec is adopted we will take away the pref so it'd be nice to be able to simplify.

@@ +4295,5 @@
>        return;
>      }
> + 
> +    // if we found an non security cookie and preference id set enable
> +    if (!cookie->IsSecure() && CookieSecurityEnabled) {

This will evict the first insecure cookie, not necessarily the oldest. We can't change the algorithm that much. (also couple of typos in the comment)

@@ +4304,5 @@
>  
>      // Check if we've found the oldest cookie so far.
>      if (!aIter.entry || oldestTime > cookie->LastAccessed()) {
>        oldestTime = cookie->LastAccessed();
>        aIter.entry = aEntry;

You're going to need to keep track of the oldest insecure and the oldest secure. One way would be a second nsListIter to track the secure one, and then when the loop is done if aIter.entry is still nullptr then you copy the secure tmp into aIter.

Or you could use just aIter and keep a boolean whether an insecure one has been found. If an insecure one is found then skip any secure ones, but still store an older insecure one. If an insecure one isn't found then keep storing the oldest secure one.

@@ +4497,5 @@
>          aName.Equals(cookie->Name())) {
>        aIter = nsListIter(entry, i);
> +      if (cookie->IsSecure()) {
> +          return true;
> +      } else if( aPath.Equals(cookie->Path())) {

What part of the spec is this code for? The second bit will always be true (the outer if statement ensures that) and in any case you return true for both options.

::: netwerk/cookie/nsCookieService.h
@@ +306,5 @@
>      bool                          RequireThirdPartyCheck();
>      CookieStatus                  CheckPrefs(nsIURI *aHostURI, bool aIsForeign, bool aRequireHostMatch, const char *aCookieHeader);
>      bool                          CheckDomain(nsCookieAttributes &aCookie, nsIURI *aHostURI, const nsCString &aBaseDomain, bool aRequireHostMatch);
>      static bool                   CheckPath(nsCookieAttributes &aCookie, nsIURI *aHostURI);
>      static bool                   GetExpiry(nsCookieAttributes &aCookie, int64_t aServerTime, int64_t aCurrentTime);

What version of the source are you patching? There should be a CheckPrefixes() method just after CheckPath.

::: netwerk/test/TestCookie.cpp
@@ +39,5 @@
>  
> +bool
> +ConfirmCookieValue( const char* cookie, const char* value)
> +{
> +   if (PL_strcmp(cookie,value) > 0) {

that's not how strcmp() works. Why do you need this function? it looks like you're trying to reimplement CheckResult( , MUST_EQUAL, ).

@@ +658,5 @@
>  
>        allTestsPassed = PrintResult(rv, 9) && allTestsPassed;
>  
> +      // *** security tests
> +      sBuffer = PR_sprintf_append(sBuffer, "*** Beginning security tests...\n");

I'd say "Beginning leave-secure-alone tests" or similar referencing the spec. "security" sounds much broader than these tests actually are.

@@ +662,5 @@
> +      sBuffer = PR_sprintf_append(sBuffer, "*** Beginning security tests...\n");
> +
> +      SetACookie(cookieService, "http://security.test/", nullptr, "test=non-security; secure", nullptr);
> +      GetACookieNoHttp(cookieService, "http://security.test", getter_Copies(cookie));
> +      rv[0] = CheckResult(cookie.get(), MUST_BE_NULL);

We'd expect this result even without this patch--the cookie would get set with the flag and then not returned to an insecure host, only secure ones. I don't think we have tests for that so it's fine to leave, but add a second test trying to retrieve the cookie from httpS://security.test and make sure that is also MUST_BE_NULL.

@@ +668,5 @@
> +      GetACookieNoHttp(cookieService, "https://security.test", getter_Copies(cookie));
> +      rv[1] = CheckResult(cookie.get(), MUST_EQUAL, "test=security");
> +      SetACookie(cookieService, "http://security.test/", nullptr, "test=non-security", nullptr);
> +      GetACookieNoHttp(cookieService, "http://security.test", getter_Copies(cookie));
> +      rv[2] = !ConfirmCookieValue(cookie.get(), "non-secuity");

As per the above comment, I'd expect this to be null. Better to test from https://security.test and make sure the cookie hadn't been overwritten. Don't see why CheckResult() doesn't work here and in the following

These would be clearer if you commented about what you were testing, 3.1 (setting a secure cookie from an insecure host) or 3.2 (overwriting an existing cookie with one set insecurely). Obviously wordy to comment on every one, but you could do them in chunks.

@@ +677,5 @@
> +      GetACookieNoHttp(cookieService, "http://security.test", getter_Copies(cookie));
> +      rv[4] = !ConfirmCookieValue(cookie.get(), "non-secuity");
> +      SetACookie(cookieService, "https://security.test/", nullptr, "test=security3; path=/path", nullptr);
> +      GetACookieNoHttp(cookieService, "https://security.test", getter_Copies(cookie));
> +      rv[5] = ConfirmCookieValue(cookie.get(), "secuity3");

None of these tests use domain or path attributes. We need to make sure insecure cookies can be set if the domains differ, and that paths differing don't make a difference.

::: toolkit/components/telemetry/Histograms.json
@@ +8069,5 @@
>      "n_values": 10,
>      "releaseChannelCollection": "opt-out",
>      "description": "How often are secure cookies set from non-secure origins, and vice-versa? 0=nonsecure/http, 1=nonsecure/https, 2=secure/http, 3=secure/https"
>    },
> +  "IGNORE_NON_SECURITY_COOKIE": {

To help people find these on the telemetry site it would be best to start with COOKIE_, like COOKIE_INSECURE_SECURE or COOKIE_LEAVE_SECURE_ALONE or something.

@@ +8071,5 @@
>      "description": "How often are secure cookies set from non-secure origins, and vice-versa? 0=nonsecure/http, 1=nonsecure/https, 2=secure/http, 3=secure/https"
>    },
> +  "IGNORE_NON_SECURITY_COOKIE": {
> +    "alert_emails": ["seceng@mozilla.org"],
> +    "expires_in_version": "50",

Nightly is already 51, definitely don't want 50 here! While you're here we probably want to bump the one above (COOKIE_SCHEME_SECURITY) to the same value. Probably want at least 55, if not 59 (the next ESR) or 60. I think the last big "expire" point was 40, so probably people are using 60 by default (but you could look around that file).

	Josh Matthews [:jdm]
	Comment 11 • 8 years ago

Comment on attachment 8779299 [details] [diff] [review]
Added three security conditions according to the spec.

Review of attachment 8779299 [details] [diff] [review]:
-----------------------------------------------------------------

Dan's done a great job of reviewing the changes so far; I'm in agreement with all of his comments and have little to add of my own. I'm going to be away next week, so please ask Ehsan for feedback on any subsequent changes.

::: netwerk/cookie/nsCookieService.cpp
@@ +4304,5 @@
>  
>      // Check if we've found the oldest cookie so far.
>      if (!aIter.entry || oldestTime > cookie->LastAccessed()) {
>        oldestTime = cookie->LastAccessed();
>        aIter.entry = aEntry;

The eviction strategy is getting much more complicated in bug 1264192. Adding extra state tracking the oldest insecure cookie in each case is not going to be pleasant :( It's not clear to me that prioritizing evicting insecure cookies is worthwhile.

	Amy Chung [:Amy] Assignee
	Comment 12 • 8 years ago

Hi Daniel,
I found cookie can't get when secure flag enable and from non-secuire host with out my modification.
https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp?q=%2Bfunction%3A%22nsCookieService%3A%3AGetCookieStringInternal%28nsIURI+%2A%2C+bool%2C+bool%2C+const+mozilla%3A%3ANeckoOriginAttributes%2C+bool%2C+nsCString+%26%29%22&redirect_type=single#3124
For this reason, I create a testing item for spec 3.1 and modified the SetCookieStringCommon() that when setting cookie failure then return NS_ERROR_FAILURE.
But this modification lets many unit tests and mochitests fail.
For example:
===Unit Test===
1. In test_bug526789.js line 128, cs.setCookieString(emptyuri, null, "foo4=bar; domain=.", null);
   the domain name error, so SetCookieStringCommon() return NS_ERROR_FAILURE.
2. In test_cookies_profile_close.js line 44, Services.cookies.setCookieString(uri, null, "oh2=hai", null);
   shows "No DBState! Profile already closed?" return NS_ERROR_FAILURE.
3. In extensions/cookie/test/unit/test_cookies_thirdparty.js line 116, return NS_ERROR_FAILURE.
   Set secure flag in non-secure host.
4. In netwerk/cookie/test/unit/test_bug1155169.js line 28, return NS_ERROR_FAILURE.
   Set secure flag in non-secure host.
5. In netwerk/test/unit/test_bug667818.js, return NS_ERROR_FAILURE.
   Used invalid cookie name.

===mochitest===
1. browser_storage_basic.js, can't set cookie c3 and uc1, because the cookies set secure flag from non-secure host.
2. other mochitests about delete cookie, the fail reason is c3 and uc1 didn't set successfully.
3. browser_storage_sidebar.js, the testing iterm "running {"location":"uc1","sidebarHidden":false}" fail, because uc1 can't be set.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca884fea09cd&selectedJob=25692976

Do I have to crate a bug for these testing errors? 
And Do I need to modify these testing errors or delete the testing items for letting all tests pass?

	Amy Chung [:Amy] Assignee
	Comment 14 • 8 years ago

Hi Daniel,
In this bug, I would like to crate two patch:
1. For implementing spec:
I have modified the test cases 0 and 1 for testing sepc 3.1 on TestCookie.cpp without the modification that modified SetCookieStringCommon() for returning NS_FAIL when cookie can't set.

2. For fixing errors of try server
I got the testing result of try server, and the uint test pass after reverting the modification of SetCookieStringCommon().
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2589766062

But I have to modify the testing cases(mochitest and W3C Web Platform Tests) that set secure flag on non-security domain.
For this reason, I will create a other path for modifying testing cases.

	Amy Chung [:Amy] Assignee
	Comment 15 • 8 years ago

(In reply to Josh Matthews [:jdm] (away until 8/22) from comment #11)
> Comment on attachment 8779299 [details] [diff] [review]
> Added three security conditions according to the spec.
> 
> Review of attachment 8779299 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Dan's done a great job of reviewing the changes so far; I'm in agreement
> with all of his comments and have little to add of my own. I'm going to be
> away next week, so please ask Ehsan for feedback on any subsequent changes.
> 
> ::: netwerk/cookie/nsCookieService.cpp
> @@ +4304,5 @@
> >  
> >      // Check if we've found the oldest cookie so far.
> >      if (!aIter.entry || oldestTime > cookie->LastAccessed()) {
> >        oldestTime = cookie->LastAccessed();
> >        aIter.entry = aEntry;
> 
> The eviction strategy is getting much more complicated in bug 1264192.
> Adding extra state tracking the oldest insecure cookie in each case is not
> going to be pleasant :( It's not clear to me that prioritizing evicting
> insecure cookies is worthwhile.

Josh, thanks for your suggestions!

	Daniel Veditz [:dveditz]
	Comment 17 • 8 years ago

(In reply to Josh Matthews [:jdm] (away until 8/22) from comment #11)
> It's not clear to me that prioritizing evicting insecure cookies is worthwhile.

If we don't then preventing an insecure cookie from overwriting an existing secure cookie is useless in preventing attacks: the attacker simply blows out the cookie store to evict the old secure one, then replaces the formerly-secure cookie with the new value.

We can argue the point that the spec should be changed in the httpbis mailing list/IETF group (Mozilla has several members and it's not a standard yet), but if we're going to implement the spec we should implement the spec.

	Amy Chung [:Amy] Assignee
	Comment 18 • 8 years ago

Hi Daniel,
I modified nsCookieService::FindStaleCookie() on if statement, please give me some suggestions on my path.

Thanks!

	Patrick McManus [:mcmanus]
	Comment 19 • 8 years ago

(In reply to Daniel Veditz [:dveditz] from comment #17)
> (In reply to Josh Matthews [:jdm] (away until 8/22) from comment #11)
> > It's not clear to me that prioritizing evicting insecure cookies is worthwhile.
> 
> If we don't then preventing an insecure cookie from overwriting an existing
> secure cookie is useless in preventing attacks: the attacker simply blows
> out the cookie store to evict the old secure one, then replaces the
> formerly-secure cookie with the new value.
> 
> We can argue the point that the spec should be changed in the httpbis
> mailing list/IETF group (Mozilla has several members and it's not a standard
> yet), but if we're going to implement the spec we should implement the spec.


I also bet Mike West would talk about it right here..

	Mike West
	Comment 20 • 8 years ago

Hi!

Dan's explanation makes sense to me, and reflects why we wrote the document the way we did. If you don't change the eviction logic in some way similar to what the spec suggests, then the restrictions it places upon `secure` cookies are fairly trivially bypassable.

I agree that it's a pain in the butt to implement (and Chrome's implementation is even more complicated, since we also support https://tools.ietf.org/html/draft-west-cookie-priority-00). I hope y'all agree that it's important nonetheless.

	Daniel Veditz [:dveditz]
	Comment 21 • 8 years ago

Comment on attachment 8781578 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8781578 [details] [diff] [review]:
-----------------------------------------------------------------

Going the right direction but needs some work. I didn't look at the tests this time. Because the code is doing the wrong thing the tests might be testing the wrong thing.

::: netwerk/cookie/nsCookieService.cpp
@@ +1988,5 @@
>    nsDependentCString serverTime(aServerTime ? aServerTime : "");
>    SetCookieStringInternal(aHostURI, isForeign, cookieString,
>                            serverTime, aFromHttp, attrs,
>                            isPrivate, aChannel);
> +

You may have added some unwanted whitespace here. Not sure why this was flagged as a changed line.

@@ +2074,5 @@
>      // document.cookie can only set one cookie at a time
>      if (!aFromHttp)
>        break;
>    }
> +

Same here. Might want to adjust your editor settings to make these stand out.

@@ +3407,5 @@
>        "cookie is httponly; coming from script");
>      return;
>    }
>  
> +  bool isHTTPS = false;

My previous comment wasn't against setting this value outside the "if found" block -- in fact it's much better to do so only once!

One important thing to watch out for: it is possible for aHostURI to be null legitimately! Unlike SetCookieInternal() where it null is an error, this AddInternal() is called from AddNative() and ImportCookie(), both of which explicitly pass nullptr. We do not want to block secure cookies from either caller! Wonder if it's worth counting how many secure cookies are set that way?

Maybe change the name of this to "isSecure":

  bool isSecure == true;
  if ( aHostURI && NS_FAILED( aHostURI->SchemeIs("https", &isSecure) ) {
    isSecure == false;
  }

Once you know this get the 3.1 checks out of the way right here -- before you try looking up the cookie. You don't delay it to the else{} clause.

  // reject cookies with the "secure" flag if set from an insecure host
  // (draft-ietf-httpbis-cookie-alone section 3.1)
  if ( mLeaveSecureAlone && aCookie->IsSecure() && !isSecure ) {
    COOKIE_LOGFAILURE
    Telemetry::Accumulate( ...., SECURE_SET_FROM_HTTP );
    return;
  }

Oh yeah, we shouldn't use magic numbers in the Accumulate() -- to easy to get it wrong or make mistakes in future edits. Set up some #defines

#define SECURE_SET_FROM_HTTP         0
#define DOWNGRADE_SECURE_EXACT       1
#define DOWNGRADE_SECURE_INEXACT     2
#define DOWNGRADE_SECURE_FROM_SECURE 3
#define EVICTED_NEWER_INSECURE       4

Those states are my suggestions. The spec only has 3 reasons for evicting a cookie so we could coalesce. Will talk about why I'm interested in separating out 1,2, & 3 later.

@@ +3413,2 @@
>    nsListIter matchIter;
>    bool foundCookie = FindCookie(aKey, aCookie->Host(),

FindCookie() only finds exact matches, the spec wants us to look for inexact (ignoring path) matches if there's an attempt to set an insecure cookie. I'm slightly dubious whether that's a good idea which is why I'd like telemetry to distinguish ones where we block inexact vs exact.

It's tempting to call FindCookie() and then only check for inexact matches if we don't find one -- but that has a "bug" if the exact match happens to be an insecure cookie, while one or more secure cookies (with different paths) would otherwise block setting the cookie. How would that case come about if we're blocking them? Ignoring grandfathered cookies, the current spec is perfectly fine with a _secure_ site downgrading a cookie to remove the secure flag. I question that part of the spec, too, (seems asymmetric) which is why I added a telemetry value for that.

I think you're going to have to create a parallel FindSecureCookie()--or call the current one with a flag or maybe a nullptr path to indicate it--and look for cookies twice. Sadly inefficient -- I hope this isn't a hot path. Probably not given network latencies in the first place.

Given how messy this gets I tend to lean toward creating a separate function even if it's mostly duplicative. Otherwise future maintenance will get messy.

  // insecure cookie can't overwrite one with a "secure" flag
  bool exact;
  if (mLeaveSecureAlone && !aCookie->IsSecure() &&
      FindSecureCookie(, &exact)) {
    if ( !isSecure ) {
      // we have to block this one
      COOKIE_LOGFAILURE
      if ( exact )
        Accumulate(..., DOWNGRADE_SECURE_EXACT);
      else
        Accumulate(..., DOWNGRADE_SECURE_INEXACT);
      return;
    }
    else {
      // per proposed spec we don't block this, but let's track
      Accumulate(..., DOWNGRADE_SECURE_FROM_INSECURE);
    }
  }

... then on with the current FindCookie() code, which now doesn't need to change because we've done all the 3.1 and 3.2 checks before we got there.

@@ +3559,5 @@
>      NotifyChanged(purgedList, u"batch-deleted");
>    }
>  
>    NotifyChanged(aCookie, foundCookie ? u"changed" : u"added");
> +

Whitespace.

@@ +4645,5 @@
>        aIter = nsListIter(entry, i);
> +      if ( mLeaveSecureAlone) {
> +          return true;
> +      } else if( aPath.Equals(cookie->Path())) {
> +        return true;

We can't change FindCookie() like this. When we want to replace a cookie it needs to be an exact match. And even when we're looking for an inexact match (to see if we want to block it) the mLeaveSecureAlone rules only apply if the setting host is insecure and the cookie is secure. This will break lots of stuff.

::: toolkit/components/telemetry/Histograms.json
@@ +7334,5 @@
>    },
> +  "COOKIE_LEAVE_SECURE_ALONE": {
> +    "alert_emails": ["seceng@mozilla.org"],
> +    "expires_in_version": "55",
> +    "kind": "count",

I'm sorry I missed this last time, but you do not want a "count" probe here; please use "enumerated". That means you'll have to add a "n_values" property as well.

New probes require a separate "Data Collection Review" as described here:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

@@ +7336,5 @@
> +    "alert_emails": ["seceng@mozilla.org"],
> +    "expires_in_version": "55",
> +    "kind": "count",
> +    "releaseChannelCollection": "opt-out",
> +    "description": "How often are secure-only-flag cookies set from non-secure origins, and newly created cookie’s secure-only-flag is not set when the scheme component of the request-uri does not denote a secure protocol. 0=non-secure origins set secure-only-flag, 1=newly created cookie set secure-only-flag"

We're not measuring how often they're set (that's partially covered by the COOKIE_SCHEME_SECURITY probe), we're measuring how often the new feature blocks them. If we can let's tighten up (shorten) the description. I've also added a couple of states to capture other aspects of the spec or changes we might propose.

description: "Measuring the effects of draft-ietf-httpbis-cookie-alone blocking. 0=http setting secure cookie; 1=http downgrading secure cookie (exact); 2=http downgrading secure cookie (inexact); 3=https downgrading secure cookie; 4=evicting newer insecure cookie"

	Daniel Veditz [:dveditz]
	Comment 22 • 8 years ago

Comment on attachment 8781578 [details] [diff] [review]
part1 For implementing spec.

My immediately preceding comments were on this version.

	Daniel Veditz [:dveditz]
	Comment 23 • 8 years ago

Comment on attachment 8782405 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8782405 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like my previous comments apply to this version.

::: devtools/client/storage/test/browser_storage_basic.js
@@ +91,5 @@
> +       var result = doc.querySelector(".table-widget-cell[data-id='" + id + "']");
> +       ok((result==null), "Table item " + id + " shouldn't be present");
> +     } else {
> +       ok(doc.querySelector(".table-widget-cell[data-id='" + id + "']"),
> +          "Table item " + id + " should be present");

Why are browser_storage_basic.js test changes part of this patch? is this from a different bug?

If this is a test broken by the current patch it might be better to update the part of the test that tries to use cookies insecurely rather than compensate for broken results (which might invalidate whatever this is really trying to test)

::: netwerk/cookie/nsCookieService.cpp
@@ +4432,5 @@
> +      // Check if we've found the oldest cookie so far.
> +      if (!oldestIter.entry || oldestTime > lastAccessed) {
> +        oldestIter.entry = aEntry;
> +        oldestIter.index = i;
> +        oldestTime = lastAccessed;

If mLeaveSecureAlone is true and there are only secure cookies, you won't identify one to delete. oldestIter can't ever be set. We need to always keep track of the oldest cookie (secure or not), and then in parallel keep track of the oldest insecure cookie if mLeaveSecureAlone.

	Amy Chung [:Amy] Assignee
	Comment 24 • 8 years ago

Hi Daniel,
I have modified the implement of sepc as below:
1. isHttp=> isSecure
2. if ( aHostURI && NS_FAILED( aHostURI->SchemeIs("https", &isSecure) ) {
    isSecure == false;
   }
3. Added sept section on comment
4. Added telemetry define
5. Added FindSecureCookie and exact flag
6. Separated findSecureCookie() from findCookie(), and use findSecureCookie first.
7. Modified the telemetry setting.

And I modified browser_storage_basic.js for fixing try server errors that occurs by this patch.
I will fix all try server errors(mochitest fails) and create other patch.

Thanks for your help!

	Daniel Veditz [:dveditz]
	Comment 25 • 8 years ago

Comment on attachment 8782992 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8782992 [details] [diff] [review]:
-----------------------------------------------------------------

needs another pass (and again I didn't check the tests)

::: netwerk/cookie/nsCookieService.cpp
@@ +3421,5 @@
>    nsListIter matchIter;
> +  bool exact;
> +  bool foundCookie = FindSecureCookie(aKey, aCookie->Host(),aCookie->Name(),
> +                                      aCookie->Path(), matchIter, exact);
> +  if (mLeaveSecureAlone && !aCookie->IsSecure() && foundCookie) {

Being a paranoid person fearing future code updates I'd prefer initializing exact here as well as inside FindSecureCookie().

The result of FindSecureCookie() is useless later if it's not an exact match, and it doesn't find _anything_ that's not a secure cookie. It's better to initialize foundCookie to false and then only call FindSecureCookie() if ( mLeaveSecureAlone && !aCookie->IsSecure() ) to avoid unnecessary scans through the cookie list. That is, something like

  bool foundCookie = false;
  if ( mLeaveSecureAlone && !aCookie->IsSecure() ) {
    // need to make sure we're not overwriting a secure cookie
    foundCookie = FindSecureCookie(...);
    if ( foundCookie ) {

       ... the rest of what you have ...
    }
  }

It's another nesting level, but avoids searching through cookies twice sometimes when not necessary.

The other way is to stuff all this into the original FindCookie() and call it only once, but always iterate all the way through unless there's both an exact match and a secure semi-match found. My gut feel at the moment is that iterating twice isn't so expensive but I have no idea how many cookies people have on average (we don't seem to have telemetry on it).

@@ +3437,5 @@
> +         Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,DOWNGRADE_SECURE_INEXACT);
> +       }
> +       return;
> +     } else {
> +       Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,DOWNGRADE_SECURE_FROM_SECURE);

I'd add a comment above this like
  // a secure site is allowed to downgrade a secure cookie but we want to measure anyway

Mozilla reviewers will want a space after the comma (here and elsewhere).

@@ +3442,5 @@
> +     }
> +  }
> +
> +  if (!foundCookie) {
> +    foundCookie = FindCookie(aKey, aCookie->Host(),

There's one case where this goes wrong: if a secure site downgrades a secure cookie then foundCookie will be true but matchIter may be pointing at a cookie with the wrong path! In this case we won't know if an exact match exists or not because FindSecureCookie() stops after the first partial match.

The easiest fix is to change this to "if ( !foundCookie || !exact ) {", forcing a re-search in that case.

The other way would be to change the code as discussed above, making FindCookie() do everything in one pass. Is this extra work worth it? It would eliminate the need to iterate through the cookie list twice a lot of the time, but it would also mean we'd have to iterate all the way through more of the time. It would make our telemetry about downgrades more accurate since in theory the current patch will understate the number of exact matches on sites that have multiple cookies with the same name. In practice how many sites do that? I would have thought it was pretty rare, but twitter.com has a whole bunch of cookies named "suggestion" that differ only by path! (on the other hand these are not "secure" cookies so they won't impact anything here.)

If you go the more complicated route then it might be worth splitting DOWNGRADE_SECURE_FROM_SECURE into exact and inexact variants since we'll have trustworthy numbers. I think we're OK going the easy route, though.

Also worth noting that if people have so many cookies that iterating through the list twice has an impact then it's probably impacting people already without this change and the right solution would be to change the data structure we're using for cookies.

@@ +3522,5 @@
> +    // If the new cookie is non-https and wants to set sercure flag,
> +    // browser have to ignore this new cookie.
> +    // (draft-ietf-httpbis-cookie-alone section 3.1)
> +    if (mLeaveSecureAlone && aCookie->IsSecure() && !isSecure) {
> +      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,

Is there something I've missed that is leading you to insist on doing this check way down here rather than first as I've suggested (and as suggested by the ordering in the spec)? If you do it first you can skip calling FindSecureCookie()/FindCookie() entirely in this case because we can bail early.

If you leave this check until here we'll end up allowing an insecure site to UPGRADE an insecure cookie to a secure cookie. The the insecure one will be found so it skips this check altogether, but the found one was not secure so it's not caught in the "downgrade" checks.

@@ +4434,5 @@
>      }
>  
> +    // Ensure that a non-secure site can never cause a
> +    // "secure" cookie to be evisted.
> +    // (draft-ietf-httpbis-cookie-alone section 3.3)

"evisted" must be a typo for "evicted" in the spec -- I can't find that word in English dictionaries nor in any other cookie spec. Only 1900 results on Google, and a bunch of those were for some guy Eric Visted. Go ahead and use "evicted" in the comment.

@@ +4445,5 @@
> +          nonSecurityIter.index = i;
> +          oldestNonSecurityTime = lastAccessed;
> +      }
> +    } else {
> +      // Check if we've found the oldest cookie so far.

By making this an "else" we essentially never track the oldest cookie (unless the pref is disabled). But we NEED that info! If there are no expired cookies and all the cookies are secure cookies then oldestIter is never set and then we will return aIter.entry == nullptr (original initialized value). This probably crashes RemoveCookieFromList() dereferencing null at aIter.Cookie()->IsSession().

@@ +4654,5 @@
> +  nsCookieEntry *entry = mDBState->hostTable.GetEntry(aKey);
> +  if (!entry)
> +    return false;
> +
> +  exact = false;

You should move this initialization up top, before the chance of an early return.

	Daniel Veditz [:dveditz]
	Comment 26 • 8 years ago

> then oldestIter is never set and then we will return aIter.entry == nullptr

I'm not convinced you need oldestIter: you can leave that part of the code unchanged from the original (using aIter) and then at the end overwrite with the aIter = nonSecurityIter; bit in the right conditions.

	Daniel Veditz [:dveditz]
	Comment 27 • 8 years ago

Comment on attachment 8782992 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8782992 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/TestCookie.cpp
@@ +639,5 @@
>  
> +      // *** leave-secure-alone tests
> +      sBuffer = PR_sprintf_append(sBuffer, "*** Beginning leave-secure-alone tests...\n");
> +
> +      // testing items 1 & 2 for 3.1 of sepc Deprecate modification of ’secure’

typo: "sepc" should be "spec" (here and in the next section)

Do "items 1 & 2" refer to the tests below? They don't seem to relate directly to the spec sub-sections 1 & 2 (that is, 3.1.1 and 3.1.2). Less confusing to use "0 & 1" to match the rv[] indexes.

@@ +651,5 @@
> +      // testing items 3 & 4 & 5 for 3.2 of sepc Deprecate modification of ’secure’
> +      // cookies from non-secure origins
> +      SetACookie(cookieService, "http://www.security.test/path/", nullptr, "test=non-security; path=/path/", nullptr);
> +      GetACookieNoHttp(cookieService, "http://www.security.test/path/", getter_Copies(cookie));
> +      rv[2] = !CheckResult(cookie.get(), MUST_EQUAL, "test=non-secuity");

A blank line above the comment would improve readability

It would be better to CheckResult(... MUST_BE_NULL) here than "!CheckResult()". If you need to, and used different cookie names (not just different values) then you could check using CheckResult(... MUST_NOT_CONTAIN) when you expect potentially multiple values. For example you could add a check for getting a cookie from secure https: here (on the path) to make sure it can still see test=security (path="/") but not test=non-security.

@@ +654,5 @@
> +      GetACookieNoHttp(cookieService, "http://www.security.test/path/", getter_Copies(cookie));
> +      rv[2] = !CheckResult(cookie.get(), MUST_EQUAL, "test=non-secuity");
> +      SetACookie(cookieService, "https://www.security.test/path/", nullptr, "test=security2; path=/path/; secure", nullptr);
> +      GetACookieNoHttp(cookieService, "https://www.security.test/path/", getter_Copies(cookie));
> +      rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=security2");

won't this fail? There should be two cookies, test=security (path=/) from before and test=security2 (path=/path) from here. The longer path ought to come first ("test=security2; test=security"). Also not sure what you're intending to test here (comments before each would help!).

What we need to test, and isn't tested here, is that a _secure_ site can set an _insecure_ cookie, both a fuzzy match (different path) for a secure cookie as well as downgrading an exact match.

@@ +657,5 @@
> +      GetACookieNoHttp(cookieService, "https://www.security.test/path/", getter_Copies(cookie));
> +      rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=security2");
> +      SetACookie(cookieService, "http://www.security.test/", nullptr, "test=non-security; domain=security.test", nullptr);
> +      GetACookieNoHttp(cookieService, "http://www.security.test", getter_Copies(cookie));
> +      rv[4] = CheckResult(cookie.get(), MUST_EQUAL, "test=non-security");

This works for now, but if you add a test to make sure a secure site can downgrade a secure cookie then you might have multiple cookies returned here. Be careful about results and ordering.

	Amy Chung [:Amy] Assignee
	Comment 28 • 8 years ago

(In reply to Daniel Veditz [:dveditz] from comment #25)
> Comment on attachment 8782992 [details] [diff] [review]
> part1 For implementing spec.
> 
> Review of attachment 8782992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> needs another pass (and again I didn't check the tests)
> 
> ::: netwerk/cookie/nsCookieService.cpp
> @@ +3421,5 @@
> >    nsListIter matchIter;
> > +  bool exact;
> > +  bool foundCookie = FindSecureCookie(aKey, aCookie->Host(),aCookie->Name(),
> > +                                      aCookie->Path(), matchIter, exact);
> > +  if (mLeaveSecureAlone && !aCookie->IsSecure() && foundCookie) {
> 
> Being a paranoid person fearing future code updates I'd prefer initializing
> exact here as well as inside FindSecureCookie().
> 
> The result of FindSecureCookie() is useless later if it's not an exact
> match, and it doesn't find _anything_ that's not a secure cookie. It's
> better to initialize foundCookie to false and then only call
> FindSecureCookie() if ( mLeaveSecureAlone && !aCookie->IsSecure() ) to avoid
> unnecessary scans through the cookie list. That is, something like
> 
>   bool foundCookie = false;
>   if ( mLeaveSecureAlone && !aCookie->IsSecure() ) {
>     // need to make sure we're not overwriting a secure cookie
>     foundCookie = FindSecureCookie(...);
>     if ( foundCookie ) {
> 
>        ... the rest of what you have ...
>     }
>   }
> 
> It's another nesting level, but avoids searching through cookies twice
> sometimes when not necessary.
> 
> The other way is to stuff all this into the original FindCookie() and call
> it only once, but always iterate all the way through unless there's both an
> exact match and a secure semi-match found. My gut feel at the moment is that
> iterating twice isn't so expensive but I have no idea how many cookies
> people have on average (we don't seem to have telemetry on it).
> 
> @@ +3437,5 @@
> > +         Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,DOWNGRADE_SECURE_INEXACT);
> > +       }
> > +       return;
> > +     } else {
> > +       Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,DOWNGRADE_SECURE_FROM_SECURE);
> 
> I'd add a comment above this like
>   // a secure site is allowed to downgrade a secure cookie but we want to
> measure anyway
> 
> Mozilla reviewers will want a space after the comma (here and elsewhere).
> 
> @@ +3442,5 @@
> > +     }
> > +  }
> > +
> > +  if (!foundCookie) {
> > +    foundCookie = FindCookie(aKey, aCookie->Host(),
> 
> There's one case where this goes wrong: if a secure site downgrades a secure
> cookie then foundCookie will be true but matchIter may be pointing at a
> cookie with the wrong path! In this case we won't know if an exact match
> exists or not because FindSecureCookie() stops after the first partial match.
> 
> The easiest fix is to change this to "if ( !foundCookie || !exact ) {",
> forcing a re-search in that case.
> 
> The other way would be to change the code as discussed above, making
> FindCookie() do everything in one pass. Is this extra work worth it? It
> would eliminate the need to iterate through the cookie list twice a lot of
> the time, but it would also mean we'd have to iterate all the way through
> more of the time. It would make our telemetry about downgrades more accurate
> since in theory the current patch will understate the number of exact
> matches on sites that have multiple cookies with the same name. In practice
> how many sites do that? I would have thought it was pretty rare, but
> twitter.com has a whole bunch of cookies named "suggestion" that differ only
> by path! (on the other hand these are not "secure" cookies so they won't
> impact anything here.)
> 
> If you go the more complicated route then it might be worth splitting
> DOWNGRADE_SECURE_FROM_SECURE into exact and inexact variants since we'll
> have trustworthy numbers. I think we're OK going the easy route, though.
> 
> Also worth noting that if people have so many cookies that iterating through
> the list twice has an impact then it's probably impacting people already
> without this change and the right solution would be to change the data
> structure we're using for cookies.
> 
> @@ +3522,5 @@
> > +    // If the new cookie is non-https and wants to set sercure flag,
> > +    // browser have to ignore this new cookie.
> > +    // (draft-ietf-httpbis-cookie-alone section 3.1)
> > +    if (mLeaveSecureAlone && aCookie->IsSecure() && !isSecure) {
> > +      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
> 
> Is there something I've missed that is leading you to insist on doing this
> check way down here rather than first as I've suggested (and as suggested by
> the ordering in the spec)? If you do it first you can skip calling
> FindSecureCookie()/FindCookie() entirely in this case because we can bail
> early.
> 
> If you leave this check until here we'll end up allowing an insecure site to
> UPGRADE an insecure cookie to a secure cookie. The the insecure one will be
> found so it skips this check altogether, but the found one was not secure so
> it's not caught in the "downgrade" checks.
> 
> @@ +4434,5 @@
> >      }
> >  
> > +    // Ensure that a non-secure site can never cause a
> > +    // "secure" cookie to be evisted.
> > +    // (draft-ietf-httpbis-cookie-alone section 3.3)
> 
> "evisted" must be a typo for "evicted" in the spec -- I can't find that word
> in English dictionaries nor in any other cookie spec. Only 1900 results on
> Google, and a bunch of those were for some guy Eric Visted. Go ahead and use
> "evicted" in the comment.
> 
> @@ +4445,5 @@
> > +          nonSecurityIter.index = i;
> > +          oldestNonSecurityTime = lastAccessed;
> > +      }
> > +    } else {
> > +      // Check if we've found the oldest cookie so far.
> 
> By making this an "else" we essentially never track the oldest cookie
> (unless the pref is disabled). But we NEED that info! If there are no
> expired cookies and all the cookies are secure cookies then oldestIter is
> never set and then we will return aIter.entry == nullptr (original
> initialized value). This probably crashes RemoveCookieFromList()
> dereferencing null at aIter.Cookie()->IsSession().
> 
> @@ +4654,5 @@
> > +  nsCookieEntry *entry = mDBState->hostTable.GetEntry(aKey);
> > +  if (!entry)
> > +    return false;
> > +
> > +  exact = false;
> 
> You should move this initialization up top, before the chance of an early
> return.
Hi Daniel,
Thanks for your help, and I have a question on spec 3.2. 
In spec 3.2, cookie can modify security flag to false on security site, 
but why can't cookie modify value and keep security flag to true on security site?

	Daniel Veditz [:dveditz]
	Comment 29 • 8 years ago

(In reply to Amy Chung [:Amy] from comment #28)
> Thanks for your help, and I have a question on spec 3.2. 
> In spec 3.2, cookie can modify security flag to false on security site, but
> why can't cookie modify value and keep security flag to true on security site?

Please trim the quoted part down to just the relevant bits: I'm not sure what I said that prompted this question. The proposed spec puts no restrictions on what a secure site can do to any cookie.

(I did request telemetry on when a secure site downgrades a secure cookie. I want to see if it's worth proposing adding that to the spec but currently there's no restriction; it's measurement only.)

	Amy Chung [:Amy] Assignee
	Comment 30 • 8 years ago

Hi Daniel,
I have modified from your suggestions as below:
[nsCookieService.cpp]
1. init foundCookie to false
2. Moved foundCooke = FindSecureCookie() into if (mLeaveSecureAlone && !aCookie->IsSecure()) statement.  
3. Added comment on downgrade secure flag from https host.
4. Added "!exact" condition when call findCookie().
5. Moved implement spec 3.1 before to implement spec 3.2.
6. Always found the oldest cookie for avoiding the DB only stores security cookies.
7. Added a "aIter.entry != nullprt"condition before remove cookie.
8. Moved "exact = false;" before return on FindSecureCookie()

[TestCookie.cpp]
1. ”sepc" should be "spec".
2. Modified test 1&2=> test 0&1 on comment.
3. Modified rv[2] to let security host can modify value and keep secure flag =true.
4. Modified rv[3] to get cookie for "test=sercurity2".
5. Added a test for downgrading secure.

And I have another question about spec 3.2: 
If secure host set security flag to true then the conditions of confirm the cookie already store in database need to match Cookie's name, Cookie's host and Cookie's path.
Please confirm whether my understanding is correct, thanks!

	Amy Chung [:Amy] Assignee
	Comment 31 • 8 years ago

Updated patch file.

Hi Daniel,
I have modified from your suggestions as below:
[nsCookieService.cpp]
1. init foundCookie to false
2. Moved foundCooke = FindSecureCookie() into if (mLeaveSecureAlone && !aCookie->IsSecure()) statement.  
3. Added comment on downgrade secure flag from https host.
4. Added "!exact" condition when call findCookie().
5. Moved implement spec 3.1 before to implement spec 3.2.
6. Always found the oldest cookie for avoiding the DB only stores security cookies.
7. Added a "aIter.entry != nullprt"condition before remove cookie.
8. Moved "exact = false;" before return on FindSecureCookie()

[TestCookie.cpp]
1. ”sepc" should be "spec".
2. Modified test 1&2=> test 0&1 on comment.
3. Modified rv[2] to let security host can modify value and keep secure flag =true.
4. Modified rv[3] to get cookie for "test=sercurity2".
5. Added a test for downgrading secure.

And I have another question about spec 3.2: 
If secure host set security flag to true then the conditions of confirm the cookie already store in database need to match Cookie's name, Cookie's host and Cookie's path.
Please confirm whether my understanding is correct, thanks!

	Daniel Veditz [:dveditz]
	Comment 32 • 8 years ago

(In reply to Amy Chung [:Amy] from comment #31)
> And I have another question about spec 3.2: 
> If secure host set security flag to true then the conditions of confirm the
> cookie already store in database need to match Cookie's name, Cookie's host
> and Cookie's path.
> Please confirm whether my understanding is correct, thanks!

To replace a cookie in the database the name, host, and path must all match. Otherwise you're creating a new cookie.

	Daniel Veditz [:dveditz]
	Comment 33 • 8 years ago

Comment on attachment 8784346 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8784346 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple more things.

::: netwerk/cookie/nsCookieService.cpp
@@ +3430,5 @@
>    nsListIter matchIter;
> +  bool exact;
> +  bool foundCookie = false;
> +  if (mLeaveSecureAlone && !aCookie->IsSecure()) {
> +    foundCookie = FindSecureCookie(aKey, aCookie->Host(),aCookie->Name(),

Remember what I said about being paranoid and initializing 'exact' just in case? Now if we're setting a secure cookie or someone turns off the feature 'exact' will be used uninitialized on line 3458. Please initialize to false.

@@ +3437,5 @@
> +       if (!isSecure) {
> +         // If the newly created cookie’s "secure-only-flag" is not set,
> +         // and the "scheme" component of the "request-uri" does not
> +         // denote a "secure" protocol, then abort these steps and ignore
> +         // the newly created cookie.

The comment seems incomplete: "ignore the new cookie if there is already a secure one by that name and host"

@@ +3447,5 @@
> +         } else {
> +           Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,DOWNGRADE_SECURE_INEXACT);
> +         }
> +         return;
> +       } else if (isSecure && !aCookie->IsSecure()){

Just "else" was fine here: we already know both those conditions are true.

@@ +3545,5 @@
>        nsListIter iter;
>        FindStaleCookie(entry, currentTime, iter);
> +
> +      if (iter.entry) {
> +        oldCookie = iter.Cookie();

Why this change? We're over the limit and we need to delete a cookie, and if we're over the limit then there is a cookie to be found. FindStaleCookie() can't be changed to return a null cookie.

In comment 25 I didn't mean change the code so that condition wouldn't cause a crash, I was pointing to the potential crash as one reason fix the way you picked the stale cookie. But the main reason to fix it is because FindStaleCookie() must return a cookie.

@@ +4444,5 @@
> +    if (mLeaveSecureAlone) {
> +      // confirm secure flag and last access time.
> +      if ((!nonSecurityIter.entry
> +           || oldestNonSecurityTime > lastAccessed)
> +           && !cookie->IsSecure()) {

It would be more readable to move the !cookie->IsSecure() check to the outer if(), while logically the same.

if (mLeaveSecureAlone && !cookie->IsSecure()) {
  // keep track of oldest non-secure cookie
  if (!nonSecurityIter.entry || oldestNonSecurityTime > lastAccessed) {

@@ +4463,5 @@
> +  if (nonSecurityIter.entry && mLeaveSecureAlone) {
> +    aIter = nonSecurityIter;
> +    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,EVICTED_NEWER_INSECURE);
> +  } else {
> +    // If mLeaveSeucreAlone set false or all cookies set secure in DB

I prefer checking mLeaveSecureAlone first for consistency, and makes it clearer that this condition is dependent on that feature.

Add a comment "// return the oldest non-secure cookie" above aIter = nonSecurityIter;

typo in the comment you do have, should be "Secure". Instead of referencing the variable name (which is pretty easy to see a couple lines above) reference the feature:

  // return true oldest if "leave secure alone" disabled, or all cookies are secure

Just realized this telemetry data by itself won't tell us much if we don't have anything to compare it to. Also, if all the cookies are non-secure then we'll evict nonSecurityIter and count it, but it's really the same as oldestIter.

#define EVICTED_OLDEST_COOKIE 5  // move with others, obviously
#define EVICTED_OLDEST_SECURE_COOKIE 6

if (mLeaveSecureAlone && nonSecurityIter.entry) {
  // return the oldest non-secure cookie
  aIter = nonSecurityIter;

  if (oldestNonSecurityTime > oldestTime) {
    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
                          EVICTED_NEWER_SECURE);
  } else {
    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
                          EVICTED_OLDEST_COOKIE);
  }
} else {
  // return true oldest if all cookies are secure
  // or if "leave secure alone" is disabled
  aIter = oldestIter;
  Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
                        EVICTED_OLDEST_SECURE_COOKIE);
}
 

I was going to lump two cases together under EVICTED_OLDEST_COOKIE. I wouldn't expect too many hits on the second one, but one pathological case of this spec would be that old secure cookies could accumulate and "get stuck", preventing the _insecure_ cookies actually used by the site. That is, this is a potential attack on a site. I think it would be worth measuring.

::: netwerk/test/TestCookie.cpp
@@ +656,5 @@
> +      rv[2] = CheckResult(cookie.get(), MUST_EQUAL, "test=security2");
> +      // Non-secure site can't modify secure cookie
> +      SetACookie(cookieService, "http://www.security.test/test/", nullptr, "test=non-security; path=/test/", nullptr);
> +      GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
> +      rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=security2");

The GetACookie...() isn't on the path that was set, so it's not a valid test that the non-security cookie was blocked. Change it to "https://www.security.test/test/" to make the test valid.

@@ +657,5 @@
> +      // Non-secure site can't modify secure cookie
> +      SetACookie(cookieService, "http://www.security.test/test/", nullptr, "test=non-security; path=/test/", nullptr);
> +      GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
> +      rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=security2");
> +      // Secure site can downgrod secure level

"downgrade"

	Amy Chung [:Amy] Assignee
	Comment 34 • 8 years ago

Hi Daniel,
I have modified from your suggestions as below:
[nsCookieSerivce.cpp]
1. Set exact init value to false before call FindSecureCookie()
2. Modified comment 3.2
3. Removed "else if (isSecure && !aCookie->IsSecure())"
4. Removed "if (aIter.entry)"
5. Modify condition of search oldest non-secure cookie in FindStaleCookie()
6. Added comment above  aIter = nonSecurityIter;
7. Modified comment above after = oldestSecurityIter;
8. Added telemetry for confirming the cookie that removing is the oldest.

[TestCookie.cpp]
1. modified uri of GetACookieNoHttp on rv[3] 
2. modified typo

Thanks for your reviewing.

	Amy Chung [:Amy] Assignee
	Comment 35 • 8 years ago

Removing some extra blanks.

Hi Daniel,
I have modified from your suggestions as below:
[nsCookieSerivce.cpp]
1. Set exact init value to false before call FindSecureCookie()
2. Modified comment 3.2
3. Removed "else if (isSecure && !aCookie->IsSecure())"
4. Removed "if (aIter.entry)"
5. Modify condition of search oldest non-secure cookie in FindStaleCookie()
6. Added comment above  aIter = nonSecurityIter;
7. Modified comment above after = oldestSecurityIter;
8. Added telemetry for confirming the cookie that removing is the oldest.

[TestCookie.cpp]
1. modified uri of GetACookieNoHttp on rv[3] 
2. modified typo

Thanks for your reviewing.

	Amy Chung [:Amy] Assignee
	Comment 36 • 8 years ago

Hi Daniel,
I have modified error of mochitest on try server.
1. devtools/client/storage/test/storage-listings.html
   Set secure flag to false on c3 cookie.
2. devtools/client/storage/test/storage-unsecured-iframe.html
   Set secure flag to false on uc1 cookie.
3. devtools/server/tests/browser/browser_storage_listings.js
   i.  Modified secure flag of c3 cookie to false on searching task, because it already set a c3 cookie with non-secure flag in storage-dynamic-windows.html.
       If the searching task set secure flag to false, and the secure flag doesn't match to the c3 cookie that exists on data base.
   ii. If secure flag of cookie set true, it doesn't store in database.
       For this reason, this test case needs to deduct above situation for calculating cookie lengths.
4. testing/web-platform/meta/cookies/secure/set-from-dom.sub.html.ini
   Item 'expected' needs to modify to PASS after implementing " draft-west-leave-secure-cookies-alone-05". 
5. testing/web-platform/meta/cookies/secure/set-from-http.sub.html.ini
   Item 'expected' needs to modify to PASS after implementing " draft-west-leave-secure-cookies-alone-05". 

Please help me to review my path, thanks!

	Daniel Veditz [:dveditz]
	Comment 37 • 8 years ago

Comment on attachment 8784702 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8784702 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just some style and comment suggestions but this is ready to get a review from an actual Cookie "Peer" (like Josh).

::: netwerk/cookie/nsCookieService.cpp
@@ +3425,5 @@
> +  // (draft-ietf-httpbis-cookie-alone section 3.1)
> +  if (mLeaveSecureAlone && aCookie->IsSecure() && !isSecure) {
> +    COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
> +      "non-https cookie can't set secure flag");
> +    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,SECURE_SET_FROM_HTTP);

Please leave a space after the comma. It's also a long line so you can consider instead doing a line-break after the comma and lining "SECURE_SET..." under the first argument. Either way, some whitespace between params. This goes for all the other calls to Accumulate, too.

@@ +3441,5 @@
> +        // and the "scheme" component of the "request-uri" does not
> +        // denote a "secure" protocol, then abort these steps and ignore
> +        // the newly cookie if there is already a secure one by that
> +        // name and host.
> +        // (draft-ietf-httpbis-cookie-alone section 3.2)

Might make sense to move this comment block _above_ FindSecureCookie(), to explain what you're doing before you've done it instead of after.

Typo: "newly cookie" should be "new cookie" now that you reworded this sentence.

@@ +3458,5 @@
> +    }
> +  }
> +
> +  if (!foundCookie || !exact) {
> +    foundCookie = FindCookie(aKey, aCookie->Host(),

This might be a little mysterious to a future maintainer without a comment. Something like "If we found a secure cookie we can only use it if it's an exact match", or "If we didn't find an exactly matching secure cookie earlier we must look again".

@@ +4437,5 @@
>        aIter.index = i;
>        return;
>      }
>  
> +    // Ensure that a non-secure site can never cause a

non-secure "cookie" can't evict a secure cookie, we're not checking the site at this point (and according to the spec that's correct). In other words "site" -> "cookie".

@@ +4461,5 @@
> +  if (nonSecurityIter.entry && mLeaveSecureAlone) {
> +    // confirm the removing cookie is the oldest cookie.
> +    if (oldestNonSecurityTime > oldestTime) {
> +      Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
> +                                          EVICTED_NEWER_INSECURE);

Our usual style is to line the "EVICTED..." under the "T" in "Telemetry::COOKIE_LEAVE_SECURE_ALONE" (here and next 2 spots). Align on the left, not the right.

@@ +4665,5 @@
> +                            const nsAFlatCString &aPath,
> +                            nsListIter           &aIter,
> +                            bool                 &exact)
> +{
> +  EnsureReadDomain(aKey);

You took out the initialization of exact here. "Better safe than sorry" suggests initializing it here as well, in case someone in the future (mis-)uses this function elsewhere and doesn't initialize it properly. Your code isn't wrong (because exact is already initialized by the current one-and-only caller), just a suggestion. Unless it's on the hot path for performance paranoia pays off in the future.

You probably should expand the comment above the function to note

// We set "exact" to true if the paths happen to match, but false just 
// means the returned cookie isn't a match not that no match exists.
// An opportunistic way to avoid calling FindCookie later if we don't have to.

	Daniel Veditz [:dveditz]
	Comment 38 • 8 years ago

Comment on attachment 8785931 [details] [diff] [review]
part 2 for modifying errors from tyr server.

Review of attachment 8785931 [details] [diff] [review]:
-----------------------------------------------------------------

The web platform test changes are great, but I can't opine on the devtools test changes. The changes look like they'd "work", but I don't know what exactly they were wanting to test. The "secure" attribute wasn't in there by accident: it's possible they would prefer changing the tests to use a secure server instead.

	Amy Chung [:Amy] Assignee
	Comment 39 • 8 years ago

Hi Ryan,
I've implemented the spec https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone/.
This update ignores the non-secure host to set secure cookie.
Since in mochitest, the cookie "c3" and cookie "uc1" would be set secure flag from non-secure host, 
it seems like the modification of mochitest is needed for passing all tests from try server.
Please check the attachment for detail "part 2 for modifying errors from tyr server". 

I would like to know the reason that why mochitest had set secure flag from non-secure host, just in case.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 40 • 8 years ago

(In reply to Amy Chung [:Amy] from comment #39)
> Hi Ryan,
> I've implemented the spec
> https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone/.
> This update ignores the non-secure host to set secure cookie.
> Since in mochitest, the cookie "c3" and cookie "uc1" would be set secure
> flag from non-secure host, 
> it seems like the modification of mochitest is needed for passing all tests
> from try server.
> Please check the attachment for detail "part 2 for modifying errors from tyr
> server". 
> 
> I would like to know the reason that why mochitest had set secure flag from
> non-secure host, just in case.

I am not sure about the purpose of the secure flag in this test either...  Let's ask :mratcliffe who was involved with the Storage panel early on.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 41 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40)
> I am not sure about the purpose of the secure flag in this test either... 
> Let's ask :mratcliffe who was involved with the Storage panel early on.

The secure flag on these cookies was purely to ensure that most cookie configurations were covered.
Setting isSecure to false on these cookies is fine now that the cookies are no longer editable.

	Amy Chung [:Amy] Assignee
	Comment 42 • 8 years ago

Hi Daniel,
I had update the code, based on your suggestions. 
Please help me review my patch.
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 43 • 8 years ago

Comment on attachment 8787278 [details] [diff] [review]
part1 For implementing spec.

Hi Josh,
Would you help me to review my patch?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 44 • 8 years ago

Comment on attachment 8785931 [details] [diff] [review]
part 2 for modifying errors from tyr server.

Hi Josh,
I have modified test cases on Mochitest for passing try server,
please help me to review my patch.

Thanks!

	Daniel Veditz [:dveditz]
	Comment 45 • 8 years ago

There is an updated version of the spec available: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01

According to Mike West (in mail to the httpbis group)

  The diff between this and the previous draft are fairly minimal. In short, we've added a
  path-matching check to the storage model modifications suggested in
  https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01#section-3 in order to deal
  with real-world compatibility issues raised in https://github.com/httpwg/http-extensions/issues/223
  (and https://bugs.chromium.org/p/chromium/issues/detail?id=580770).

The behavior is not "exact match for path" like FindCookie so we still need FindSecureCookie, but the algorithm will have to be different.

           4.  The "path" of the newly created cookie path-matches the
               "path" of the existing cookie.

           Note: The "path" comparison is not symmetric, ensuring only
           that a newly-created non-secure cookie does not overlay an
           existing secure cookie, providing some mitigation against
           cookie fixing attacks.  That is, given an existing secure
           cookie named "a" with a "path" of "/login", a non-secure
           cookie named "a" could be set for a "path" of "/" or "/foo",
           but not for a "path" of "/login" or "/login/en".

	Daniel Veditz [:dveditz]
	Comment 46 • 8 years ago

Comment on attachment 8787278 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8787278 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good (apart from some alignment and a typo "FinSecureCookie") for the -00 version of the spec we started implementing. But now there's a new -01 version of the spec that will require some changes to FindSecureCookie() and associated tests. I'm tempted to just check this in as-is and then file a separate bug to implement the evolved spec, but that may not be done before the merge next week in which case bad behavior will leak out in Firefox 51.

Tempted as I am I think we have to implement the updated spec instead -- I'm very sorry!

::: netwerk/cookie/nsCookieService.cpp
@@ +3432,5 @@
> +  if (mLeaveSecureAlone && aCookie->IsSecure() && !isSecure) {
> +    COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
> +      "non-https cookie can't set secure flag");
> +    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
> +      SECURE_SET_FROM_HTTP);

SECURE_SET_FROM_HTTP should line up under the first argument, Telemetry::COOKIE_LEAVE_SECURE_ALONE

@@ +3463,5 @@
> +        }
> +        return;
> +      } else {
> +        // a secure site is allowed to downgrade a secure cookie but we want to measure anyway
> +        Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,DOWNGRADE_SECURE_FROM_SECURE);

The three Telemetry::Accumulate calls above this need to have the second argument put on the next line and lined up under the first argument.

@@ +4482,5 @@
> +    }
> +
> +    // return the oldest non-secure cookie
> +    aIter = nonSecurityIter;
> +    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,EVICTED_NEWER_INSECURE);

Wrap this call and align like the two EVICTED_ ones above

@@ +4488,5 @@
> +    // return true oldest if "leave secure alone" disabled,
> +    // or all cookies are secure
> +    aIter =  oldestIter;
> +    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
> +                                  EVICTED_OLDEST_SECURE_COOKIE);

EVICTED is too far right; line up the start of the arguments, not the ends.

	Daniel Veditz [:dveditz]
	Comment 47 • 8 years ago

Tests of the new requirements will be essential. Given an existing secure cookie with path "/foo", an insecure site should be able to set an insecure cookie on path "/" and "/bar", but not a path of "/foo" or "/foo/bar". Also important to test that the insecure site _can_ set a cookie on the path "/foobar": you can't use a simple "starts with" check.

	Amy Chung [:Amy] Assignee
	Comment 48 • 8 years ago

I have modified code and test cases from new sepc as below:
[nsCookieService.cpp]
1. Modified FindSecureCookie() for matching new spec 3.2, and removed "exact".
2. Modified typo "FinSecureCookie()" on comment.
3. Modified some format errors with setting Telemetry.
4. Removed EVICTED_NEWER_INSECURE Telemetry to avoid to call it twice.

[TestCookie.cpp]
1. Added the new test case for new sepc 3.2.
   i.  test case 3:
       Set a new cookie with same host, same name and partially matching path to the existing secure cookie.
       And the new cookie can't modify the existing security cookie, because it from non-secure site.
   ii. test case 4:
       Create a new cookie with same host, same name and non-matching path to the existing secure cookie.
       And the new cookie can create, because it would like to deemed as not match to the existing secure cookie.

Please help me to review my path, thanks!

	Amy Chung [:Amy] Assignee
	Comment 49 • 8 years ago

Hi Daniel,
I have finished to implement the spec of new version.
Please help me to review my patch.
Thanks!

Hi Josh,
I have merged my patch after updating the version of 43a5351815ee from your patch.
Please help me to review my patch.
Thank you!

	Josh Matthews [:jdm]
	Comment 50 • 8 years ago

Comment on attachment 8790268 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8790268 [details] [diff] [review]:
-----------------------------------------------------------------

This is largely fine with minor changes, but we're missing the important domain-matching step, and violating the specification in the case where we need to evict a cookie and all existing cookies are secure.

::: netwerk/cookie/nsCookieService.cpp
@@ +3436,5 @@
> +  if (aHostURI && NS_FAILED(aHostURI->SchemeIs("https", &isSecure))) {
> +    isSecure = false;
> +  }
> +
> +  // If the new cookie is non-https and wants to set sercure flag,

s/sercure/secure/

@@ +3450,5 @@
>    nsListIter matchIter;
> +  bool foundCookie = false;
> +  if (mLeaveSecureAlone && !aCookie->IsSecure()) {
> +    // Step1, call FindSecureCookie(). FindSecureCookie() would
> +    // find the existing cookie which with security flag and has

s/which with/with the/

@@ +3484,1 @@
>      aCookie->Name(), aCookie->Path(), matchIter);

nit: indent to match the ( on the previous line.

@@ +4498,5 @@
>        isPrimaryEvictionCandidate = !pathMatches || !domainMatches;
>      }
>  
>      int64_t lastAccessed = cookie->LastAccessed();
> +    // Ensure that a non-secure cookie can't never cause

s/can't never/will never/

@@ +4502,5 @@
> +    // Ensure that a non-secure cookie can't never cause
> +    // a "secure" cookie to be evicted.
> +    // (draft-ietf-httpbis-cookie-alone section 3.3)
> +    if (mLeaveSecureAlone && !cookie->IsSecure()) {
> +      // confirm secure flag and last access time.

I don't think this comment adds anything.

@@ +4511,2 @@
>        }
> +    } else {

This else isn't necessary; we want to update all the other data as well.

@@ +4511,3 @@
>        }
> +    } else {
> +      if (cookie->IsSession()) {

This can be `else if`.

@@ +4540,5 @@
>        oldestCookie.index = i;
>      }
>    }
>  
>    // Prefer to evict the oldest session cookies with a non-matching path/domain,

Let's update this comment with
"Prefer to evict the oldest insecure cookie,
followed by the oldest session cookie with a non-matching path/domain,"

@@ +4563,5 @@
>    } else if (oldestNonMatchingNonSessionCookie.entry) {
>      aIter = oldestNonMatchingNonSessionCookie;
>    } else {
> +    // return true oldest if "leave secure alone" disabled,
> +    // or all cookies are secure

We're actually violating the specification here if we evict a secure cookie. The spec proposes a design where the insecure cookie is inserted before choosing a cookie to evict, ensuring that a secure cookie will never be evicted. We should make the caller check for the case where no cookie is evicted and reject the new cookie instead.

@@ +4789,5 @@
> +  nsCookieEntry *entry = mDBState->hostTable.GetEntry(aKey);
> +  if (!entry)
> +    return false;
> +
> +  bool  pathMatch = false;

nit: remove the extra space after `bool` here.

@@ +4794,5 @@
> +  const nsCookieEntry::ArrayType &cookies = entry->GetCookies();
> +  for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) {
> +    nsCookie *cookie = cookies[i];
> +    if (cookie->IsSecure()
> +        && aHost.Equals(cookie->Host())

This won't match for the case where the existing cookie has domain=.google.com and the new cookie has domain=google.com (https://tools.ietf.org/html/rfc6265#section-5.1.3). We need to perform a domain match against both cookies; see the `bool domainMatches` I added in bug 1264192 for an example.

@@ +4795,5 @@
> +  for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) {
> +    nsCookie *cookie = cookies[i];
> +    if (cookie->IsSecure()
> +        && aHost.Equals(cookie->Host())
> +        && aName.Equals(cookie->Name())) {

Let's invert this condition and continue, to avoid the extra indentation.

@@ +4797,5 @@
> +    if (cookie->IsSecure()
> +        && aHost.Equals(cookie->Host())
> +        && aName.Equals(cookie->Name())) {
> +      // If the path of new cookie and the path of existing cookie
> +      // aren't "/", and this situation needs to compare paths to

s/and/then/

@@ +4800,5 @@
> +      // If the path of new cookie and the path of existing cookie
> +      // aren't "/", and this situation needs to compare paths to
> +      // ensure only that a newly-created non-secure cookie does not
> +      // overlay an existing secure cookie.
> +      // Example: That is, given an existing secure cookie named "a"

s/That is,//

@@ +4807,5 @@
> +      // "path" of "/login" or "/login/en".
> +      if (aPath.Length() > 1 && cookie->Path().Length() > 1) {
> +        if (aPath.Length() == cookie->Path().Length()) {
> +          if (aPath.Equals(cookie->Path())) {
> +            pathMatch = true;

pathMatch = aPath.Equals(cookie->Path();

@@ +4811,5 @@
> +            pathMatch = true;
> +          }
> +        } else if (aPath.Length() > cookie->Path().Length()) {
> +          if (aPath.Find(cookie->Path().get()) == 0) {
> +            pathMatch = true;

pathMatch = ...

@@ +4815,5 @@
> +            pathMatch = true;
> +          }
> +        } else {
> +          if (cookie->Path().Find(aPath.get()) == 0) {
> +            pathMatch = true;

pathMatch = ...

::: netwerk/test/TestCookie.cpp
@@ +719,5 @@
> +      SetACookie(cookieService, "https://www.security.test/", nullptr, "test=security3", nullptr);
> +      GetACookieNoHttp(cookieService, "http://www.security.test/", getter_Copies(cookie));
> +      rv[5] = CheckResult(cookie.get(), MUST_EQUAL, "test=security3");
> +      SetACookie(cookieService, "http://www.security.test/", nullptr, "test=non-security2; domain=security.test", nullptr);
> +      GetACookieNoHttp(cookieService, "http://www.security.test/", getter_Copies(cookie));

We will need multiple tests for domain matching - one where the existing cookie is a non-domain cookie and the new cookie is a domain cookie, and one where they are reversed.

@@ +857,5 @@
>        rv[0] = CheckResult(cookie.get(), MUST_EQUAL, expected.get());
>  
>        allTestsPassed = PrintResult(rv, 1) && allTestsPassed;
>  
> +      // *** eviction and creation ordering tests after enable network.cookie.security

network.cookie.leave-secure-alone

@@ +858,5 @@
>  
>        allTestsPassed = PrintResult(rv, 1) && allTestsPassed;
>  
> +      // *** eviction and creation ordering tests after enable network.cookie.security
> +      sBuffer = PR_sprintf_append(sBuffer, "*** Beginning eviction and creation tests after enable nework.cookie.security...\n");

network.cookie.leave-secure-alone

@@ +868,5 @@
> +        name = NS_LITERAL_CSTRING("test");
> +        name.AppendInt(i);
> +        name += NS_LITERAL_CSTRING("=delete_non_security");
> +
> +        // Create 49 cookies for including security flag.

Create 49 cookies that include the secure flag.

@@ +881,5 @@
> +          }
> +        }
> +      }
> +      GetACookie(cookieService, "http://creation.ordering.tests/", nullptr, getter_Copies(cookie));
> +      rv[0] = CheckResult(cookie.get(), MUST_EQUAL, expected_rm_non_security.get());

This test should fail according to the specification.

::: toolkit/components/telemetry/Histograms.json
@@ +7572,5 @@
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Measuring the effects of draft-ietf-httpbis-cookie-alone blocking. 0=http setting secure cookie; 1=http downgrading secure cookie (exact); 2=http downgrading secure cookie (inexact); 3=https downgrading secure cookie; 4=evicting newer insecure cookie"

The values in the cpp file go 0, 1, 3, 4, 5. One of these should be changed.

	Josh Matthews [:jdm]
	Comment 51 • 8 years ago

Comment on attachment 8785931 [details] [diff] [review]
part 2 for modifying errors from tyr server.

Review of attachment 8785931 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/tests/browser/browser_storage_listings.js
@@ +349,5 @@
> +       if (secureCookie.isSecure) {
> +          ++cookiesLength;
> +       }
> +    }
> +    is(data.total, storeMap.cookies[host].length - cookiesLength,

Let's add a comment like "Any secure cookies did not get stored in the database."

::: testing/web-platform/meta/cookies/secure/set-from-dom.sub.html.ini
@@ +1,1 @@
>  [set-from-dom.sub.html]

This file can be removed.

::: testing/web-platform/meta/cookies/secure/set-from-http.sub.html.ini
@@ +1,1 @@
>  [set-from-http.sub.html]

This file can be removed.

	Daniel Veditz [:dveditz]
	Comment 52 • 8 years ago

Comment on attachment 8790268 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8790268 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still working through how this interacts with the new eviction logic Josh added in bug 1264192 but there are things that need to be fixed.

::: netwerk/cookie/nsCookieService.cpp
@@ +122,5 @@
> +
> +// For telemetry COOKIE_LEAVE_SECURE_ALONE
> +#define SECURE_SET_FROM_HTTP         0
> +#define DOWNGRADE_SECURE_EXACT       1
> +#define DOWNGRADE_SECURE_FROM_SECURE 3

You don't need to leave a gap for the former #2; please renumber. If we had already landed this and gathered telemetry then we would want to leave the gap so we could compare to existing data. In this case, however, let's start with a cleaner list.

drop the _EXACT part -- it's making a distinction that's not needed (or true) anymore.

@@ +3461,5 @@
> +    // then ignore the new cookie.
> +    // (draft-ietf-httpbis-cookie-alone section 3.2)
> +    foundCookie = FindSecureCookie(aKey, aCookie->Host(),aCookie->Name(),
> +                                   aCookie->Path(), matchIter);
> +    if (foundCookie) {

If we're no longer finding exact matches then we don't need to set foundCookie or return matchIter -- they're not doing us any good. This could simply be 

  if (FindSecureCookie(...)) {

If that's too messy and you prefer using foundCookie as an intermediate that's fine, but we absolutely can't use it later.

I suppose if we're not using the cookie that's found this could be renamed to HasSecureCookie() or something. Or not -- either works.

@@ +3466,5 @@
> +      if (!isSecure) {
> +        COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
> +           "cookie can't save because older cookie is secure cookie but newer cookie is non-secure cookie");
> +        Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
> +                              DOWNGRADE_SECURE_EXACT);

as mentioned above this can be just DOWNGRADE_SECURE now.

@@ +3479,5 @@
> +  }
> +
> +  // If we didn't find secure cookie, we have to search non-secure existing cookie.
> +  if (!foundCookie) {
> +    foundCookie = FindCookie(aKey, aCookie->Host(),

Get rid of this if () -- if we're not finding exact matches we can't use any matchIter and we must _always_ call FindCookie() at this point. If we're a secure site setting a non-secure cookie and there happens to be some other secure cookie on our site then we will replace the wrong cookie, possibly breaking sites.

@@ +4803,5 @@
> +      // overlay an existing secure cookie.
> +      // Example: That is, given an existing secure cookie named "a"
> +      // with a "path" of "/login", a non-secure cookie named "a"
> +      // could be set for a "path" of "/" or "/foo", but not for a
> +      // "path" of "/login" or "/login/en".

this comment is a bit confused. a path of /login should match a path of /login. where did /foo come from here?

@@ +4807,5 @@
> +      // "path" of "/login" or "/login/en".
> +      if (aPath.Length() > 1 && cookie->Path().Length() > 1) {
> +        if (aPath.Length() == cookie->Path().Length()) {
> +          if (aPath.Equals(cookie->Path())) {
> +            pathMatch = true;

Since we don't need to return aIter--it's useless now--you could just "return true;" here (and below).

You could combine the conditionals. Since you don't need the else clauses (that are missing here) it would be a bit more readable. That is something like

   if ((aPath.Length() == cookie->Path().Length())
        && aPath.Equals(cookie->Path()) {
     return true;
   } else if ((	...

@@ +4819,5 @@
> +            pathMatch = true;
> +          }
> +        }
> +      } else {
> +        pathMatch = true;

This is wrong in the case one of the paths is length==1 and the other isn't. it fails the first test but clearly they don't match.

@@ +4827,5 @@
> +        aIter = nsListIter(entry, i);
> +        return true;
> +      }
> +    }
> +  }

I'd really like to simplify this whole thing, and maybe document with the spec bits we're checking against. something like

    nsAFlatCString newpath(aPath);
    if (!newpath.EndsWith("/")) {
      // ensure new path ends with a slash for comparisons
      newpath.Append("/");
    }
    if (cookie->IsSecure()
        && cookie->Name().Equals(aName)
        && (cookie->Host().Equals(aHost)
           || cookie->Host().EndsWith(aHost)
           || aHost.EndsWith(cookie->Host()))
        && (cookie->Path().Equals(aPath)
           || cookie->Path().StartsWith(newpath))) {
      return true;
    }

Please double and triple check me against the spec. Haven't compiled it, that's just what I came up with right now.

The relevant spec bits (currently) 

1.  Their "name" matches the "name" of the newly created
    cookie.
2.  Their "secure-only-flag" is set.
3.  Their "domain" domain-matches the "domain" of the newly
    created cookie, or vice-versa.
4.  The "path" of the newly created cookie path-matches the
    "path" of the existing cookie.

I thought CheckPath() would always result in a path that ends with a slash, but that's apparently not true because two of my cookies have paths that don't end in slash. One of them is the browserid_state cookie from login.persona.org so I guess we should investigate.

That's unfortunate as it creates more work in the above.

	Daniel Veditz [:dveditz]
	Comment 53 • 8 years ago

I was wrong about our code ensuring paths end with a slash. Not counting paths that are only "/" (most of them), I have paths that do and don't end with a "/".

For point 4 of the spec I'm not sure my example code actually tests that, or what he means by path-matches. The point was to keep a newly created cookie from hiding an existing secure one. That should mean if the new cookie matches or is /longer/ (more specific) than an existing one. My attempt in comment 52 is actually testing for the opposite, that the new path is shorter than the old. I guess I didn't need to reverse the tests (which I did so the path check wouldn't stand out).

also note I improved the domain match check so notfoo.com doesn't match a foo.com host cookie

    // isn't a match if insecure or a different name
    if (!cookie->IsSecure() || !aName.Equals(cookie->Name()))
      continue;

    // The host must "domain-match" an existing cookie or vice-versa
    if ( aHost.Equals(cookie->Host())
         || ( cookie->IsDomain() && aHost.EndsWith(cookie->Host()) )
         || ( aHost[0] == '.' && cookie->Host().EndsWith(aHost) ) ) {
      // and the path of the new cookie must path-match an existing cookie
      if ( aPath.Equals(cookie->Path())
           || ( aPath.StartsWith(cookie->Path())
                && ( cookie->Path().EndsWith("/")
                     || aPath[cookie->Path().Length()] == "/") ) ) {
        return true;
      }
    }

This is hideously complex and confusing.

	Amy Chung [:Amy] Assignee
	Comment 54 • 8 years ago

Hi Daniel,
I have some questions for your suggestions,
(In reply to Daniel Veditz [:dveditz] from comment #52)
> 
> @@ +3461,5 @@
> > +    // then ignore the new cookie.
> > +    // (draft-ietf-httpbis-cookie-alone section 3.2)
> > +    foundCookie = FindSecureCookie(aKey, aCookie->Host(),aCookie->Name(),
> > +                                   aCookie->Path(), matchIter);
> > +    if (foundCookie) {
> 
> If we're no longer finding exact matches then we don't need to set
> foundCookie or return matchIter -- they're not doing us any good. This could
> simply be 
> 
>   if (FindSecureCookie(...)) {
> 
> If that's too messy and you prefer using foundCookie as an intermediate
> that's fine, but we absolutely can't use it later.
> 
> I suppose if we're not using the cookie that's found this could be renamed
> to HasSecureCookie() or something. Or not -- either works.
If the test case as below:
       SetACookie(cookieService, "https://www.security.test/", nullptr, "test_domain=security-is-domain; secure; domain=.sec    urity.test", nullptr);
       GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
       rv[0] = CheckResult(cookie.get(), MUST_EQUAL, "test_domain=security-is-domain");
       SetACookie(cookieService, "https://www.security.test/", nullptr, "test_domain=security-set-domain; domain=www.securit    y.test", nullptr);
       GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
       rv[1] = CheckResult(cookie.get(), MUST_EQUAL, "test_domain=security-set-domain");
       
In test case 0, I created a secure cookie whose name is test_domain, and I create a new cookie to downgrade cookie test_domain.
It calls FindSecureCookie() get return value "true", but it get false of return value when calling FindCookie().
So the cookie "test_domain=security-set-domain" would be considered to be created and doesn't modify the existing cookie.
Do I need to add "RemoveCookieFromList(existing cookie)" when FindSecureCookie returns true?

Thanks!

	Amy Chung [:Amy] Assignee
	Comment 55 • 8 years ago

nsCookieService.cpp
1. comment typo
	i.  // If the new cookie is non-https and wants to set sercure flag,
             =>  // If the new cookie is non-https and wants to set secure flag,
2. modify the grammar of comment
	i.   // find the existing cookie which with security flag and has
              => // find the existing cookie with the security flag and has
        ii.  // Ensure that a non-secure cookie can't never cause
              => // Ensure that a non-secure cookie will never cause
	iii. // aren't "/", and this situation needs to compare paths to
              => // aren't "/", then this situation needs to compare paths to
       ii-ii. // Example: That is, given an existing secure cookie named "a"
              => // Example: Given an existing secure cookie named "a"
3.  modify nit
	i.  Move “aCookie->Name(), aCookie->Path(), matchIter);” to align “aKey, aCookie->Host(),”
4. Removed “confirm secure flag and last access time”
5. Modified else to else if on line 4522
6. Modified comment on 4552
7. Modified the FindStaleCookie to if we need to delete the oldest cookie and it is secure cookie that have to confirm the new cookie is also a secure cookie.
8. Removed bool pathMatch = false
9. Modified the define number of the Telemetry 
10. Drop _EXACT
11. Modified FindSecureCookie()
      i.  Modified the if condition of path match and domain match
      ii. In my view, the new cookie that set to secure cookie just need to use path match and domain match to compare to the existing cookie that also is secure cookie.  It doesn't have to exact match to existing cookie
      iii. Modified matchIter to existIter.
12. Modified DomainMatches(), added a condition "aHost[0] == '.' && cookie->Host().EndsWith(aHost)"

TestCookie.cpp
1. Added test cases for domain matching.
2. Modified comment
3. Modified the test case for sepc 3.3.

	Amy Chung [:Amy] Assignee
	Comment 56 • 8 years ago

modified from Josh's suggestions, r+.

	Amy Chung [:Amy] Assignee
	Comment 57 • 8 years ago

Modified typo.

	Amy Chung [:Amy] Assignee
	Comment 60 • 8 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=976073#c55
Beside the modifications as comment 55,
I found if all existing cookies are non-secure cookies, the cookie which will be delete isn't session cookie after calls FindStaleCookie().
For this reason,
1. I added a varies for counting non-secure cookies.
2. If the counting number < cookie number of DB, It means at least exist one secure cookie in DB, need to remove the oldest non-secure cookie first.
3. If the counting number == cookie number of DB, It means all cookies are non-secure cookies in DB, need to remove the oldest session cookie first.

	Josh Matthews [:jdm]
	Comment 62 • 8 years ago

Comment on attachment 8793909 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8793909 [details] [diff] [review]:
-----------------------------------------------------------------

Getting closer! I think I've made suggestions which will reduce some of the complexity of these changes.

::: netwerk/cookie/nsCookieService.cpp
@@ +3059,5 @@
>    // or ".google.com"; second a subdomain match, e.g.
>    // host = "mail.google.com", cookie domain = ".google.com".
> +  return aCookie->RawHost() == aHost
> +         || (aCookie->IsDomain() && StringEndsWith(aHost, aCookie->Host()))
> +         || (aHost[0] == '.' && StringEndsWith(aCookie->Host(), aHost));

Rather than making this change (which affects all users of DomainMatches), I recommend we follow my suggestions later in the file.

@@ +3470,5 @@
> +    // (draft-ietf-httpbis-cookie-alone section 3.2)
> +    foundCookie = FindSecureCookie(aKey, aCookie->Host(), aCookie->Name(),
> +                                  aCookie->Path(), existIter);
> +    if (foundCookie) {
> +      if (!isSecure && !aCookie->IsSecure()) {

Let's move the common `!aCookie->IsSecure()` check into the `if (foundCookie)` instead.

@@ +3583,5 @@
> +        COOKIE_LOGEVICTED(oldCookie, "Too many cookies for this domain");
> +        purgedList = CreatePurgeList(oldCookie);
> +      } else {
> +        COOKIE_LOGEVICTED(aCookie,
> +          "Too many cookies for this domain and the new cookie is not seucre cookie");

s/seucre/a secure/

@@ +4524,5 @@
> +        nonSecurityIter.entry = aEntry;
> +        nonSecurityIter.index = i;
> +        oldestNonSecurityTime = lastAccessed;
> +      }
> +      countNonSecureCookies++;

This will do the wrong thing if we disable the leave-secure-alone preference. Instead of counting insecure cookies, let's have a boolean flag that indicates the presence of at least one secure cookie instead, so we have:
bool preferEvictInsecure = false;
...
if (mLeaveSecureAlone) {
  if (!cookie->IsSecure()) {
    ...
  } else {
    // Secure cookies exist, so we must prioritize evicting insecure ones.
    preferEvictInsecure = true;
  }
}

@@ +4558,5 @@
>      }
>    }
>  
> +  // Prefer to evict the oldest insecure cookie and at least exist one secure
> +  // cookie in cookie DB,

// Prefer to evict the oldest insecure cookie (if there is at least one secure cookie present),

@@ +4564,5 @@
>    // followed by the oldest non-session cookie with a non-matching path/domain,
>    // resorting to the oldest non-session cookie with a matching path/domain.
> +  if (nonSecurityIter.entry
> +      && mLeaveSecureAlone
> +      && (countNonSecureCookies < cookies.Length())) {

We can simplify this to `if (preferEvictInsecure && nonSecurityIter.entry)` with my earlier suggestions.

@@ +4577,5 @@
> +    // return the oldest non-secure cookie
> +    aIter = nonSecurityIter;
> +  } else if (aCookie->IsSecure()
> +             || !mLeaveSecureAlone
> +             || (countNonSecureCookies == cookies.Length())) {

We can simplify this based on my previous suggestions:
else if (!preferEvictInsecure || aCookie->IsSecure())

@@ +4802,5 @@
>  }
>  
> +// find an secure cookie specified by host and name
> +bool
> +nsCookieService::FindSecureCookie(const nsCookieKey    &aKey,

Let's accept an nsCookie* argument instead of aHost, aName, and aPath directly. This will allow us to call DomainMatches twice - once with the cookie passed in here, and once with the existing cookie currently being considered.

@@ +4822,5 @@
> +    if (!cookie->IsSecure() || !aName.Equals(cookie->Name()))
> +      continue;
> +
> +    // The host must "domain-match" an existing cookie or vice-versa
> +    if (aHost.Equals(cookie->Host())

DomainMatches already takes care of this check.

@@ +4823,5 @@
> +      continue;
> +
> +    // The host must "domain-match" an existing cookie or vice-versa
> +    if (aHost.Equals(cookie->Host())
> +        || DomainMatches(cookie, aHost)) {

We should add another call to DomainMatches that passes aCookie and cookie->Host().

@@ +4828,5 @@
> +      // If the path of new cookie and the path of existing cookie
> +      // aren't "/", then this situation needs to compare paths to
> +      // ensure only that a newly-created non-secure cookie does not
> +      // overlay an existing secure cookie.
> +      if (aPath.Equals(cookie->Path())

PathMatches already takes care of this check.

	Amy Chung [:Amy] Assignee
	Comment 63 • 8 years ago

I have modified my patch from Josh's suggestions as below:
1.  move the common `!aCookie->IsSecure()` check into the `if (foundCookie)` instead.
2. "Too many cookies for this domain and the new cookie is not secure cookie"
   => "Too many cookies for this domain and the new cookie is not a secure cookie"
3. Modified countNonSecureCookie to use "bool preferEvictInsecure" for avoiding leave-secure-aloe set to false.
   And set preferEvictInsecure to true when DB exists at least one secure cookie.
4. Modified comment "// Prefer to evict the oldest insecure cookie and at least exist one secure"
  => // Prefer to evict the oldest insecure cookie (if there is at least one secure cookie present),
5. Modified conditions 
  if (nonSecurityIter.entry
      && mLeaveSecureAlone
      && (countNonSecureCookies < cookies.Length())) 
  =>
   if (preferEvictInsecure && nonSecurityIter.entry)
6. Modified conditions 
   } else if (aCookie->IsSecure()
             || !mLeaveSecureAlone
             || (countNonSecureCookies == cookies.Length())) {
    =>  if (!preferEvictInsecure || aCookie->IsSecure())
7. Modified argument of FindSecureCookie to nsCookie, the reasons is it can call DomainMathes twice and avoid to modify DomainMathes function.
8. Removed aHost.Equals condition, because it is already included in DomainMathes.
9. Removed aPath.Equals condition, because it is already included in PathMathes.

Please help me to review my patch, thanks!

	Josh Matthews [:jdm]
	Comment 64 • 8 years ago

Comment on attachment 8794998 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8794998 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

::: netwerk/cookie/nsCookieService.cpp
@@ +4588,5 @@
> +      aIter = oldestNonMatchingNonSessionCookie;
> +    } else {
> +      aIter = oldestCookie;
> +      Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
> +                            EVICTED_OLDEST_SECURE_COOKIE);

Let's change this telemetry value to EVICTED_PREFERRED_COOKIE and move it outside of this if/else chain so it always triggers no matter what kind of cookie we evict.

@@ +4802,5 @@
> +// find an secure cookie specified by host and name
> +bool
> +nsCookieService::FindSecureCookie(const nsCookieKey    &aKey,
> +                                 nsCookie              *aCookie,
> +                                 nsListIter            &aIter)

nit: indent these two lines by one extra space.

@@ +4819,5 @@
> +      continue;
> +
> +    // The host must "domain-match" an existing cookie or vice-versa
> +    if (DomainMatches(cookie, aCookie->Host())
> +        || DomainMatches(aCookie, cookie->Host())) {

nit: || goes on the previous line.

@@ +4824,5 @@
> +      // If the path of new cookie and the path of existing cookie
> +      // aren't "/", then this situation needs to compare paths to
> +      // ensure only that a newly-created non-secure cookie does not
> +      // overlay an existing secure cookie.
> +      if (PathMatches(cookie, aCookie->Path())) {

I think the arguments here should be reversed - we want to see if the new cookie matches the path of the existing cookie instead.

::: toolkit/components/telemetry/Histograms.json
@@ +7612,5 @@
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Measuring the effects of draft-ietf-httpbis-cookie-alone blocking. 0=http setting secure cookie; 1=http downgrading secure cookie (exact); 2=http downgrading secure cookie (inexact); 3=https downgrading secure cookie; 4=evicting newer insecure cookie"

This is missing a description for the value 5.

	Amy Chung [:Amy] Assignee
	Comment 65 • 8 years ago

Hi Josh,
I have modified from your suggestions, the modifications as below:
1. Changed EVICTED_OLDEST_SECURE_COOKIE to EVICTED_PREFERRED_COOKIE and move it outside of this if/else.
2. Fixed tow nits.
3. Added description of telemetry on Histograms.json.

But PathMatches(cookie, aCookie->Path()) I didn't modify it, because I found if I modified it to PathMatches(aCookie, cookie->Path()) that caused test 3 fail in TestCookie. 
(test 3:  
  SetACookie(cookieService, "http://www.security.test/path/foo/", nullptr, "test=non-security; path=/path/foo", nullptr    );
  GetACookieNoHttp(cookieService, "https://www.security.test/path/foo/", getter_Copies(cookie));
  rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=security2");)
And I confirmed the sepc, it gives a example on Path-Mathes: "given an existing secure cookie named "a" with a "path" of "/login", a non-secure cookie named "a" could be set for a "path" of "/" or "/foo", but not for a "path" of "/login" or "/login/en"."
Please give me your suggestion for my patch, thanks!

	Josh Matthews [:jdm]
	Comment 66 • 8 years ago

Comment on attachment 8797626 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8797626 [details] [diff] [review]:
-----------------------------------------------------------------

You're right; I got the arguments to PathMatches backwards in my last suggestion. I worked through the current implementation using the example from the specification and verified that it's correct. This looks good to me!

	Daniel Veditz [:dveditz]
	Comment 67 • 8 years ago

Comment on attachment 8797626 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8797626 [details] [diff] [review]:
-----------------------------------------------------------------

have to minus this because it does the wrong thing with a near-match secure cookie, and concerns over how this bypasses the new cookie expiration logic.

::: netwerk/cookie/nsCookieService.cpp
@@ +121,5 @@
> +static const char kCookieLeaveSecurityAlone[] = "network.cookie.leave-secure-alone";
> +
> +// For telemetry COOKIE_LEAVE_SECURE_ALONE
> +#define SECURE_SET_FROM_HTTP         0
> +#define DOWNGRADE_SECURE             1

Telemetry here could get confusing if some values are blocked cookies and others aren't. I suggest renaming the above two to
BLOCKED_SECURE_FROM_HTTP
BLOCKED_DOWNGRADE_SECURE

(also see in later comments I add another BLOCKED_ value)

@@ +3458,5 @@
> +  bool foundCookie = false;
> +  if (mLeaveSecureAlone) {
> +    // Step1, call FindSecureCookie(). FindSecureCookie() would
> +    // find the existing cookie with the security flag and has
> +    // the same name, host and path of the new cookie, if there is any.

FindSecureAlone() checks to see if there exists a cookie with the security flag and the same name, and a "close enough" match on domain and path. Comments in FindSecureAlone() are correct, these are misleading. You don't want to duplicate all the details here--it would probably be too much to comment--so it's OK to write less. Maybe something like

"Step1, FindSecureCookie() returns true if there's an existing secure cookie with the same name and a 'close enough' match for domain and path."

@@ +3467,5 @@
> +    // of the "request-uri" does not denote a "secure" protocol,
> +    // then ignore the new cookie.
> +    // (draft-ietf-httpbis-cookie-alone section 3.2)
> +    foundCookie = FindSecureCookie(aKey, aCookie, existIter);
> +    if (foundCookie && !aCookie->IsSecure()) {

See what I wrote in comment 52 about trusting foundCookie after this point and the lack of use for existIter. If we don't know whether it's an exact match or not then we can't use it. You could shorten this to

  if (FindSecureCookie(aKey, aCookie) && !aCookie->IsSecure()) {

If that's too cluttered then keep setting foundCookie here, just don't use the same value later on.

@@ +3484,5 @@
> +    }
> +  }
> +
> +  if (!foundCookie) {
> +    foundCookie = FindCookie(aKey, aCookie->Host(),

Because FindSecureCookie() may return a non-exact match we cannot safely replace it: we might have "found" a cookie with the wrong path or domain. Skip the "if (!found" check and unconditionally set

  foundCookie = FindCookie(...

You could switch back from existIter to matchIter and reduce the diff a little (but it doesn't really matter either way).

@@ +4594,1 @@
>    }

There's one case we're not measuring here: there's an implied "else {}" on the end here that represents the case of failing to set an IN-secure cookie when all the existing cookies have the secure flag. Please make that else explicit and put another telemetry probe in there BLOCKED_INSECURE_EVICTING_SECURE  (or maybe BLOCKED_EVICTING_SECURE). We could also put the telemetry out where you log "Too many cookies for this domain and the new cookie is not a secure cookie", though I think it makes more sense here where the other telemetry probes are.

[You could probably break a lot of popular sites if you were able to inject enough garbage secure cookies to prevent the site from using any of it's legitimate insecure cookies. Will need to raise that as a spec issue.]

Another potential problem: the complicated logic Josh added isn't used if a site has a mixture of insecure and secure cookies (unless LeaveSecureAlone is disabled). The site they were trying to fix was Twitter, and Twitter has a mixture of insecure and secure cookies! You'll need to talk this over with Josh for a solution that melds both. Maybe one branch that's if (preferEvictInsecure){} that goes down his list "if (foo.entry && !foo.Cookie()->IsSecure())" evicting the first test that's not secure, or only then falling back to generic nonSecurityIter. The "else" would do what his current code does. This is pretty confusing, please work with Josh.

@@ +4802,5 @@
> +// find an secure cookie specified by host and name
> +bool
> +nsCookieService::FindSecureCookie(const nsCookieKey    &aKey,
> +                                  nsCookie              *aCookie,
> +                                  nsListIter            &aIter)

Get rid of the nsListIter argument. It's not guaranteed to be an exact match so we can't safely use it outside this function.

::: netwerk/test/TestCookie.cpp
@@ +720,5 @@
> +      GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
> +      rv[5] = CheckResult(cookie.get(), MUST_EQUAL, "test_domain=security-is-domain");
> +      SetACookie(cookieService, "https://www.security.test/", nullptr, "test_domain=security-set-domain; domain=www.security.test", nullptr);
> +      GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
> +      rv[6] = CheckResult(cookie.get(), MUST_EQUAL, "test_domain=security-set-domain");

This test is confusing, maybe needs a comment or at least a different test_domain value. I'm not sure what you're testing, and I'm not sure why it passes. test 5 sets a secure cookie from a secure site; test 6 sets a cookie without the secure attribute from a secure site, so that should be allowed too. So the result in rv[6] should contain both cookies, and the MUST_EQUAL should fail. Maybe I'm misunderstanding the test (which again argues for more explanation).

@@ +726,5 @@
> +      GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
> +      rv[7] = CheckResult(cookie.get(), MUST_CONTAIN, "test_domain2=security-set-domain");
> +      SetACookie(cookieService, "https://www.security.test/", nullptr, "test_domain2=security-is-domain; secure; domain=.security.test", nullptr);
> +      GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
> +      rv[8] = CheckResult(cookie.get(), MUST_CONTAIN, "test_domain2=security-is-domain");

Not sure what you're testing here. All the setting sites are secure so all the cookies should be set.

@@ +730,5 @@
> +      rv[8] = CheckResult(cookie.get(), MUST_CONTAIN, "test_domain2=security-is-domain");
> +      // Secure site can downgrade secure level
> +      SetACookie(cookieService, "https://www.security.test/", nullptr, "test=security3", nullptr);
> +      GetACookieNoHttp(cookieService, "http://www.security.test/", getter_Copies(cookie));
> +      rv[9] = CheckResult(cookie.get(), MUST_CONTAIN, "test=security3");

This is a downgrade because of the cookies left over from test 0, otherwise you're just setting a cookie. What happens if someone changes the earlier tests? This test could keep passing, but it's no longer testing anything.

Either you should Get the cookie before setting to assert that the original value is still there, or you should just change domains (https://domaindowngrade.security.test/ ?) and set two cookies (first secure, then the same name/domain/path without the secure attribute) and make sure the second one changed the value and attribute.

::: toolkit/components/telemetry/Histograms.json
@@ +7608,5 @@
>      "description": "How often are secure cookies set from non-secure origins, and vice-versa? 0=nonsecure/http, 1=nonsecure/https, 2=secure/http, 3=secure/https"
>    },
> +  "COOKIE_LEAVE_SECURE_ALONE": {
> +    "alert_emails": ["seceng@mozilla.org"],
> +    "expires_in_version": "55",

Since this is only landing in version 52 let's give this a few more versions to gather data. 59 is the next ESR, let's go with that. 57 also works for me.

@@ +7612,5 @@
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Measuring the effects of draft-ietf-httpbis-cookie-alone blocking. 0=http setting secure cookie; 1=http downgrading secure cookie (exact); 2=https downgrading secure cookie; 3=evicting newer insecure cookie; 4=evicting the oldest secure cookie; 5=evicting the preferred cookie"

Add the description "blocked" to values 0 and 1. Add the telemetry value for failing to set a cookie because too many pre-existing secure ones.

	Daniel Veditz [:dveditz]
	Comment 68 • 8 years ago

Josh: this looks like it bypasses your new FindStaleCookie() logic in the case you designed it for (e.g. Twitter). TBH I'm not sure I'm sold on your logic (sometimes the important auth cookies are session cookies for security reasons!), but if it was helping we probably don't want to just dump it.

	Amy Chung [:Amy] Assignee
	Comment 69 • 8 years ago

(In reply to Daniel Veditz [:dveditz] from comment #67)
> Comment on attachment 8797626 [details] [diff] [review]
> part1 For implementing spec.
> 
> Review of attachment 8797626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> have to minus this because it does the wrong thing with a near-match secure
> cookie, and concerns over how this bypasses the new cookie expiration logic.
> 
> ::: netwerk/cookie/nsCookieService.cpp
> @@ +121,5 @@
> > +static const char kCookieLeaveSecurityAlone[] = "network.cookie.leave-secure-alone";
> > +
> > +// For telemetry COOKIE_LEAVE_SECURE_ALONE
> > +#define SECURE_SET_FROM_HTTP         0
> > +#define DOWNGRADE_SECURE             1
> 
> Telemetry here could get confusing if some values are blocked cookies and
> others aren't. I suggest renaming the above two to
> BLOCKED_SECURE_FROM_HTTP
> BLOCKED_DOWNGRADE_SECURE
> 
> (also see in later comments I add another BLOCKED_ value)
> 
> @@ +3458,5 @@
> > +  bool foundCookie = false;
> > +  if (mLeaveSecureAlone) {
> > +    // Step1, call FindSecureCookie(). FindSecureCookie() would
> > +    // find the existing cookie with the security flag and has
> > +    // the same name, host and path of the new cookie, if there is any.
> 
> FindSecureAlone() checks to see if there exists a cookie with the security
> flag and the same name, and a "close enough" match on domain and path.
> Comments in FindSecureAlone() are correct, these are misleading. You don't
> want to duplicate all the details here--it would probably be too much to
> comment--so it's OK to write less. Maybe something like
> 
> "Step1, FindSecureCookie() returns true if there's an existing secure cookie
> with the same name and a 'close enough' match for domain and path."
> 
> @@ +3467,5 @@
> > +    // of the "request-uri" does not denote a "secure" protocol,
> > +    // then ignore the new cookie.
> > +    // (draft-ietf-httpbis-cookie-alone section 3.2)
> > +    foundCookie = FindSecureCookie(aKey, aCookie, existIter);
> > +    if (foundCookie && !aCookie->IsSecure()) {
> 
> See what I wrote in comment 52 about trusting foundCookie after this point
> and the lack of use for existIter. If we don't know whether it's an exact
> match or not then we can't use it. You could shorten this to
> 
>   if (FindSecureCookie(aKey, aCookie) && !aCookie->IsSecure()) {
> 
> If that's too cluttered then keep setting foundCookie here, just don't use
> the same value later on.
> 
> @@ +3484,5 @@
> > +    }
> > +  }
> > +
> > +  if (!foundCookie) {
> > +    foundCookie = FindCookie(aKey, aCookie->Host(),
> 
> Because FindSecureCookie() may return a non-exact match we cannot safely
> replace it: we might have "found" a cookie with the wrong path or domain.
> Skip the "if (!found" check and unconditionally set
> 
>   foundCookie = FindCookie(...
> 
> You could switch back from existIter to matchIter and reduce the diff a
> little (but it doesn't really matter either way).
> 
Hi Daniel,
In my https://bugzilla.mozilla.org/show_bug.cgi?id=976073#c54, the test case as below:
SetACookie(cookieService, "https://www.security.test/", nullptr, "test_domain=security-is-domain; secure; domain=.sec    urity.test", nullptr);
GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
rv[0] = CheckResult(cookie.get(), MUST_EQUAL, "test_domain=security-is-domain");
SetACookie(cookieService, "https://www.security.test/", nullptr, "test_domain=security-set-domain; domain=www.securit    y.test", nullptr);
GetACookieNoHttp(cookieService, "https://www.security.test/", getter_Copies(cookie));
rv[1] = CheckResult(cookie.get(), MUST_EQUAL, "test_domain=security-set-domain");
If call FindCookie() after FindSecureCookie everytime, the cookie test_domain=security-set-domain will set to new cookie on test case 1.

In my view, maybe we can create two various foundSecureCookie & foundCookie.
Such as:
nsListIter eactIter;
nsListIter existSecureIter;
nsListIter existIter;
existIter.entry = nullptr;
bool foundCookie = false;
bool foudSecureCookie = false;

foundSecureCookie = FindSecureCookie(existSecureIter);
foundCookie = FindCookie(eactIter);

if (foundCookie) {
  exsitIter = eactIter;
} else if (foudSecureCookie) {
  exsitIter = existSecureIter;
}

if (exsitIter.entry) {
  oldCookie = existIter.Cookie();

  
  ....
}

Please reply your suggestions, thanks!

	Daniel Veditz [:dveditz]
	Comment 70 • 8 years ago

(In reply to Amy Chung [:Amy] from comment #69)
> SetACookie(cookieService, "https://www.security.test/", nullptr,
> "test_domain=security-set-domain; domain=www.securit    y.test", nullptr);
  ...
> If call FindCookie() after FindSecureCookie everytime, the cookie
> test_domain=security-set-domain will set to new cookie on test case 1.

Yes, that's what we want. Secure sites are allowed to downgrade a secure cookie. Whether we need to keep that exception is one reason we've added telemetry for that case. From §3.2.1

  "Note: This allows "secure" pages to override "secure" cookies with
  non-secure variants.  Perhaps we should restrict that as well?"

> } else if (foudSecureCookie) {
>   exsitIter = existSecureIter;

That wouldn't fix the fact that sometimes FindSecureCookie() finds a non-exact match, and we must not delete/replace/modify the non-exact matches. The value from FindSecureCookie() is only useful in determining if we want to block a non-secure site from setting a cookie that's "too similar" to an existing secure cookie.

	Amy Chung [:Amy] Assignee
	Comment 71 • 8 years ago

Hi Daniel,
I have modified partial code from your suggestions beside the potential problem, and I will discus the potential problem to Josh.
The modification as below:
netwerk/cookie/nsCookieService.cpp
1. SECURE_SET_FROM_HTTP => BLOCKED_SECURE_FROM_HTTP
   DOWNGRADE_SECURE => BLOCKED_DOWNGRADE_SECURE
2. Removed ListIter argument of FindSecureSecure()
3. Call FindCookie every time, and if the existing cookie is exactly same to new cookie and the new cookie is secure cookie, then the new cookie will set and replace to the existing cookie.
4. Added a BLOCKED_EVICTING_SECURE telemetry on FindStaleCookie() for setting an IN-secure cookie when all the existing cookies have the secure flag.

TestCookie.cpp
1. modified test cases 5 and 6 to downgrade secure level.
2. removed original test cases 7 and 8.

Histograms.json
1. Modified expired version.
2. Modified description.

Please reply me your suggestions on my patch, thanks!

	Amy Chung [:Amy] Assignee
	Comment 72 • 8 years ago

Hi Josh,
I have some view of the potential problem of FindStaleCookie() as below:
1. Create four ListIter
        OldestNoMatchingInSecureSessionCookie
        OldestInSecureSessionCookie
        OldestNoMatchingInSecureNonSessionCookie
        OldestInSecureNonSessionCookie
2. Create four various of time
        OldestNoMatchingInSecureSessionTime
        OldestInSecureSessionTime
        OldestNoMatchingInSecureNonSessionTime
        OldestInSecureNonSessionTime
3.
if (mLeaveSecureAlone) {
  if (!cookie->IsSecure()) {
    if (cookie->IsSession) {
      if (isPrimaryEvictionCandidate &&   
         (!OldestNoMatchingInSecureSessionCookie.entry || 
          OldestNoMatchingInSecureSessionTime > lastAccessed)) {
         OldestNoMatchingInSecureSessionCookie.entry = aEntry;
         ... 
       }

       if (!OldestInSecureSessionCookie.entry || 
            OldestInSecureSessionTime > lastAccessed)) {
         OldestInSecureSessionCookie.entry = aEntry;
         ... 
       }

    } else {
      if (isPrimaryEvictionCandidate &&   
         (!OldestNoMatchingInSecureNonSessionCookie.entry || 
          OldestInSecureNonSessionTime > lastAccessed)) {
         OldestNoMatchingInSecureNonSessionCookie.entry = aEntry;
         ... 
       }       

       if (!OldestInSecureNonSessionCookie.entry || 
            OldestInSecureSessionTime > lastAccessed)) {
         OldestInSecureNonSessionCookie.entry = aEntry;
         ... 
       }
    }
   
   } else {
      perfrerEvictInsecure = true;
   }
}

...
Your patch
...

if (perferEvictInsecure) {
  if (oldestNonSecurityTime > oldestCookieTime) {
    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
                          EVICTED_NEWER_INSECURE);
  } else {
     Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
                           EVICTED_OLDEST_COOKIE);
  }

  if (OldestNoMatchingInSecureSessionCookie.entry) {
    aIter = OldestNoMatchingInSecureSessionCookie;
  } else if (OldestInSecureSessionCookie.entry) {
    aIter = OldestInSecureSessionCookie;
  } else if (OldestNoMatchingInSecureNonSessionCookie.entry) {
    aIter = OldestNoMatchingInSecureNonSessionCookie;
  } else if (OldestInSecureNonSessionCookie.entry) {
    aIter = OldestInSecureNonSessionCookie;
  }     
} else if (aCookie->IsSecure() || !preferEvictInsecure)
{
...
Your patch
...
}

Please give me your suggestions, thanks!

	Josh Matthews [:jdm]
	Comment 73 • 8 years ago

I propose keeping the priotization of secure vs. insecure in the caller of FindStaleCookie, and adding an argument to FindStaleCookie that controls whether to return cookies that match a particular security value. Then the caller can call FindStaleCookie first to find an insecure cookie to evict; failing that, the caller can optionally call it again to find a secure cookie to evict if the new cookie is also secure.

	Daniel Veditz [:dveditz]
	Comment 74 • 8 years ago

Comment on attachment 8802566 [details] [diff] [review]
bug-976073v14.patch

Review of attachment 8802566 [details] [diff] [review]:
-----------------------------------------------------------------

Leaving aside the eviction order you're discussing with Josh, this looks good. Thanks for all your work on it.
r=dveditz

::: netwerk/cookie/nsCookieService.cpp
@@ +123,5 @@
> +// For telemetry COOKIE_LEAVE_SECURE_ALONE
> +#define BLOCKED_SECURE_SET_FROM_HTTP 0
> +#define BLOCKED_DOWNGRADE_SECURE     1
> +#define BLOCKED_EVICTING_SECURE      2
> +#define DOWNGRADE_SECURE_FROM_SECURE 3

I believe it will be easier to read the telemetry graphs if the downgrading buckets are together and the evicting buckets are together. If grouping by matching prefixes is aesthetically important you could rename "BLOCKED_EVICTING_SECURE" to "EVICTING_SECURE_BLOCKED" and move it to the end.

	Amy Chung [:Amy] Assignee
	Comment 75 • 8 years ago

Hi Daniel and Josh,
My modifications as below:
1. Modified BLOCKED_EVICTING_SECURE to EVICTING_SECURE_BLOCKED
2. Modified FindStaleCookie function.

Please help me to review my patch, thanks a lot!

	Josh Matthews [:jdm]
	Comment 76 • 8 years ago

Comment on attachment 8804012 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8804012 [details] [diff] [review]:
-----------------------------------------------------------------

I think the logic for finding a stale cookie is still more complicated than it needs to be. I think the original design I proposed in my email will do that.

::: netwerk/cookie/nsCookieService.cpp
@@ +4522,5 @@
>      int64_t lastAccessed = cookie->LastAccessed();
> +    // Ensure that a non-secure cookie will never cause
> +    // a "secure" cookie to be evicted.
> +    // (draft-ietf-httpbis-cookie-alone section 3.3)
> +    preferEvictInsecure =  !aIsSecure && !cookie->IsSecure();

Why do we need this logic? My proposal made FindStaleCookie take a tri-state value (Maybe<bool>):
* Nothing - no need to check whether a cookie is secure at all
* Some(true) - only check cookies that are secure
* Some(false) - only check cookies that are not secure

The resulting code could be:
if (aIsSecure.isSome()) {
    if (cookie.IsSecure() != aIsSecure.value()) {
        continue;
    }
}

@@ +4549,3 @@
>        }
>  
> +      // Check if we've found the oldest cookie so far.

This should happen earlier or we might skip the actual oldest cookie.

@@ -4471,4 @@
>      }
>    }
>  
> -  // Prefer to evict the oldest session cookies with a non-matching path/domain,

We lost this comment.

@@ +4574,4 @@
>      aIter = oldestCookie;
>    }
> +
> +  // if mLeaveSecureAlone set to true, and exist a oldest insecure cookie

I would prefer to move this into the caller, and pass oldestCookieTime as an outparam so the caller can use it.

	Amy Chung [:Amy] Assignee
	Comment 77 • 8 years ago

Hi Josh,
I have modified the FindStaleCookie() from your suggestions as below:
1. Modified the argument of aIsSecure to Maybe<bool>
2. Modified the logic to :
   if (aIsSecure.isSome()) {
    if (cookie.IsSecure() != aIsSecure.value()) {
        continue;
    }
   }
3. I didn't modify the logic of telemetry in FindStaleCookie(), because it is my view that the function is created for telemetry which needs four outparam: (i) aIsSecure, (ii) oldestEvictingCookieTime, (iii) oldestCookieTime, (ii-ii) aCookie->IsSecure. In my option, the function seems didn't make code more concise.

Please help me to review my patch, thanks!

	Josh Matthews [:jdm]
	Comment 78 • 8 years ago

(In reply to Amy Chung [:Amy] from comment #77)
> 3. I didn't modify the logic of telemetry in FindStaleCookie(), because it
> is my view that the function is created for telemetry which needs four
> outparam: (i) aIsSecure, (ii) oldestEvictingCookieTime, (iii)
> oldestCookieTime, (ii-ii) aCookie->IsSecure. In my option, the function
> seems didn't make code more concise.

Consider:
* aCookie is only used in the telemetry code, so it can be removed
* oldestEvictingCookieTime is the same as the LastAccessed() result for the cookie that is selected by FindStaleCookie
* the only new outparam is oldestCookieTime

I believe that moving the logic of which telemetry value to record into the caller will make that code much clearer; right now we're using aIsSecure to infer what the caller wants, instead.

	Josh Matthews [:jdm]
	Comment 79 • 8 years ago

Comment on attachment 8804622 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8804622 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

::: netwerk/cookie/nsCookieService.cpp
@@ +4506,5 @@
>      }
>  
> +    // Ensure that a non-secure cookie will never cause
> +    // a "secure" cookie to be evicted.
> +    // (draft-ietf-httpbis-cookie-alone section 3.3)

This comment doesn't make sense here - this code is ensuring that we only consider insecure or secure cookies separately, and never mix our eviction logic.

@@ +4527,5 @@
>        isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost);
>      }
>  
>      int64_t lastAccessed = cookie->LastAccessed();
> +    // Check if we've found the oldest cookie so far.

This needs to happen before the cookie is skipped due to its security value.

@@ -4472,3 @@
>    }
>  
> -  // Prefer to evict the oldest session cookies with a non-matching path/domain,

This comment is still missing.

@@ +4576,5 @@
>      aIter = oldestCookie;
>    }
> +
> +  // if mLeaveSecureAlone set to true, and exist a oldest insecure cookie
> +  if (!aIsSecure.value() && (aIter.entry != nullptr)) {

Let's move this to the caller, as suggested separately.

	Amy Chung [:Amy] Assignee
	Comment 80 • 8 years ago

Hi Josh and Daniel,
My modifications as below:
1. Removed comments.
2. Moved int64_t lastAccessed = cookie->LastAccessed(); before     
   if (aIsSecure.isSome()) {
     if (cookie->IsSecure() != aIsSecure.value()) {
       continue;
      }
   }
3. Added a TelemetryForEvictingStaleCookie().
4. Added comment // Prefer to evict the oldest session cookies with a non-matching path/domain,


Please help me to review my patch, thanks!

	Josh Matthews [:jdm]
	Comment 81 • 8 years ago

Comment on attachment 8806272 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8806272 [details] [diff] [review]:
-----------------------------------------------------------------

There are a couple final details to get right before we merge this..

::: netwerk/cookie/nsCookieService.cpp
@@ +3579,5 @@
> +      if (iter.entry == nullptr) {
> +        if (aCookie->IsSecure()) {
> +          optionalSecurity = Some(true);
> +          // It's valid to evict a secure cookie for another secure cookie.
> +          oldestCookieTime = FindStaleCookie(entry, currentTime, aHostURI, optionalSecurity, iter);

We can pass Some(true) directly here, since we don't need to pass it to the telemetry any more.

@@ +3581,5 @@
> +          optionalSecurity = Some(true);
> +          // It's valid to evict a secure cookie for another secure cookie.
> +          oldestCookieTime = FindStaleCookie(entry, currentTime, aHostURI, optionalSecurity, iter);
> +        } else {
> +          COOKIE_LOGEVICTED(aCookie,

We should do Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE, EVICTING_SECURE_BLOCKED); here, otherwise we won't get a chance to record it.

@@ +3593,2 @@
>        oldCookie = iter.Cookie();
> +      if (oldestCookieTime > 0) {

Let's also check for mLeaveSecureAlone.

@@ +4530,5 @@
>        isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost);
>      }
>  
> +    // Check if we've found the oldest cookie so far.
> +    if (!oldestCookie.entry || oldestCookieTime > lastAccessed) {

This check still needs to happen before we compare cookie->IsSecure() against aIsSecure(), otherwise we will only record the oldest cookie that has a matching security flag.

@@ +4562,4 @@
>    }
>  
>    // Prefer to evict the oldest session cookies with a non-matching path/domain,
> +  // followed by the oldest session cookie with a non-matching path/domain,

This comment shouldn't include "non-".

@@ +4578,5 @@
> +  return oldestCookieTime;
> +}
> +
> +void
> +nsCookieService::TelemetryForEvictingStaleCookie(mozilla::Maybe<bool> aIsSecure,

What if we pass `nsCookie *aEvicted` instead of aIsSecure and oldestEvictingCookieTime? Then it will look like this:
if (!aEvicted->IsSecure()) {
  if (aEvicted->LastAccessed() > oldestCookieTime) {
    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,	
                          EVICTED_NEWER_INSECURE);
  } else {
    Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,	
                          EVICTED_OLDEST_COOKIE);
  }
} else {
  Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,	
                        EVICTED_PREFERRED_COOKIE);
}

@@ +4584,5 @@
> +                                                 int64_t oldestEvictingCookieTime,
> +                                                 int64_t oldestCookieTime)
> +{
> +  // if mLeaveSecureAlone set to true (aIsSecure set to false),
> +  // and exist a oldest insecure cookie

This comment isn't useful any more.

::: toolkit/components/telemetry/Histograms.json
@@ +7612,5 @@
> +    "expires_in_version": "57",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Measuring the effects of draft-ietf-httpbis-cookie-alone blocking. 0=blocked http setting secure cookie; 1=blocked http downgrading secure cookie; 2=blocked evicting secure cookie; 3=evicting newer insecure cookie; 4=evicting the oldest secure cookie; 5=evicting the preferred cookie; 6=evicting the secure blocked"

The description for 4 says "oldest secure cookie", but in the code we record it for the oldest insecure cookie instead.

	Amy Chung [:Amy] Assignee
	Comment 82 • 8 years ago

(In reply to Josh Matthews [:jdm] from comment #81)
> @@ +4530,5 @@
> >        isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost);
> >      }
> >  
> > +    // Check if we've found the oldest cookie so far.
> > +    if (!oldestCookie.entry || oldestCookieTime > lastAccessed) {
> 
> This check still needs to happen before we compare cookie->IsSecure()
> against aIsSecure(), otherwise we will only record the oldest cookie that
> has a matching security flag.
Hi Josh,
The aIter will always not equal to nullptr when we move "if (!oldestCookie.entry || oldestCookieTime > lastAccessed)" before we compare cookie->IsSecure().
If we have to move it, do I also need to add condition "if (oldestCookie.Cookie()->IsSecure() == aIsSecure().value)" before set oldestCookie to aIter?

Thanks your help :)

	Josh Matthews [:jdm]
	Comment 83 • 8 years ago

Oh, I see what you mean. I propose we leave the oldestCookie-related code the way it is, and do this:
int64_t actualOldestCookieTime = cookies.Length() ? cookies[0]->LastAccessed() : 0;

then inside the loop, before the check that skips cookies with different security flags, we do:
if (actualOldestCookieTime > lastAccessed) {
  actualOldestCookieTime = lastAccessed;
}

and we return actualOldestCookieTime instead of oldestCookieTime. Does that make sense?

	Amy Chung [:Amy] Assignee
	Comment 84 • 8 years ago

Hi Josh and Daniel,
I have moved "if (!oldestCookie.entry || oldestCookieTime > lastAccessed)" first, and the modifications as below:
1. removed "optionalSecurity = Some(true);" and directly assigned Some(true) in FindStaleCookie().
2. moved "Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE, EVICTING_SECURE_BLOCKED);" when evicting the current cookie because all the existing cookies set to secure beside the current cookie.
3. Added condition "mLeaveSecureAlone." to "if (oldestCookieTime > 0)".
4. Added actualOldestCookieTime to record time of the oldest cookie and return it.
5. modified and removed comment.
6. passed `nsCookie *aEvicted` instead of aIsSecure and oldestEvictingCookieTime.
7. modified the description of telemetry.

Please help me to review my patch.

	Josh Matthews [:jdm]
	Comment 85 • 8 years ago

Comment on attachment 8806441 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8806441 [details] [diff] [review]:
-----------------------------------------------------------------

My remaining suggestions are all straightforward changes. Thanks for working on this!

::: netwerk/cookie/nsCookieService.cpp
@@ +3569,5 @@
>  
>      // check if we have to delete an old cookie.
>      nsCookieEntry *entry = mDBState->hostTable.GetEntry(aKey);
>      if (entry && entry->GetCookies().Length() >= mMaxCookiesPerHost) {
> +      int64_t oldestCookieTime = 0;

We can remove this separate declaration.

@@ +3574,5 @@
>        nsListIter iter;
> +      // Prioritize evicting insecure cookies.
> +      // (draft-ietf-httpbis-cookie-alone section 3.3)
> +      mozilla::Maybe<bool> optionalSecurity = mLeaveSecureAlone ? Some(false) : Nothing();
> +      oldestCookieTime = FindStaleCookie(entry, currentTime, aHostURI, optionalSecurity, iter);

int64_t oldestCookieTime =

@@ +4513,5 @@
> +    int64_t lastAccessed = cookie->LastAccessed();
> +    if (aIsSecure.isSome()) {
> +      // actualOldestCookieTime records the oldest cookie time and insteads
> +      // oldestCookieTime, because oldestCookieTime may not be recorded time
> +      // of the cookie whose secure flag isn't the same as aIsSecure.value().

Let's rephrase this:
// Record the age of the oldest cookie that is stored for this host.
// oldestCookieTime is the age of the oldest cookie with a matching
// secure flag, which may be more recent than an older cookie with
// a non-matching secure flag.

@@ +4514,5 @@
> +    if (aIsSecure.isSome()) {
> +      // actualOldestCookieTime records the oldest cookie time and insteads
> +      // oldestCookieTime, because oldestCookieTime may not be recorded time
> +      // of the cookie whose secure flag isn't the same as aIsSecure.value().
> +      if (actualOldestCookieTime > lastAccessed) {

Let's move this out of this if block. This will ensure that we always return a meaningful value, even if mLeaveSecureAlone is false.

@@ +4538,5 @@
>      if (aSource) {
>        isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost);
>      }
>  
> +    // Check if we've found the oldest cookie so far.

We can now move this check back to where it was originally, to reduce the amount of change in this patch.

	Amy Chung [:Amy] Assignee
	Comment 86 • 8 years ago

Josh, thanks for your review!
Daniel, please help me to review my patch.

Thanks a lot!

	Daniel Veditz [:dveditz]
	Comment 87 • 8 years ago

Comment on attachment 8806632 [details] [diff] [review]
part1 For implementing spec.

Review of attachment 8806632 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. My comments below make the code clearer but don't change how it functions. r=dveditz

::: netwerk/cookie/nsCookieService.cpp
@@ +4518,5 @@
> +      actualOldestCookieTime = lastAccessed;
> +    }
> +    if (aIsSecure.isSome()) {
> +      // We olny want to focus the oldest non-secure cookie first,
> +      // then focus the oldest secure cookie.

typo: "only"--or you could delete that word entirely and it would be clearer.

Not sure "focus" is the right word; maybe "find"? "Look for"?

Not clear from the comment that "first, then" are referring to the routine being called two separate times. Maybe "first time through," and "then find the oldest secure cookie the second time we are called." ?

@@ +4521,5 @@
> +      // We olny want to focus the oldest non-secure cookie first,
> +      // then focus the oldest secure cookie.
> +      if (cookie->IsSecure() != aIsSecure.value()) {
> +        continue;
> +      }

This inner "if" is confusing without working through the context and realizing that the only time it's called with Some(true) is if there are zero insecure cookies. But in that case this inner test is unnecessary.

Could be clearer as

if (aIsSecure.isSome() && !aIsSecure.value()) {
  // first time through we only want to find non-secure cookies
  if (cookie->IsSecure()) {
    continue;
  }
}

	Amy Chung [:Amy] Assignee
	Comment 88 • 8 years ago

I have modified the comment and if condition as Daniel's suggestions.
Thanks Daniel & Josh's help!

	Amy Chung [:Amy] Assignee
	Comment 89 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d80f2c3abe5af4a447a7a7fc24f18f2a216880&exclusion_profile=false&selectedJob=30622147
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9de9e84a0992c06b9a0b4d4256de22c21a11751

	Amy Chung [:Amy] Assignee
	Comment 91 • 8 years ago

Comment on attachment 8808950 [details] [diff] [review]
Part 2: Modify errors from try server, r=jdm

># HG changeset patch
># User amy <amchung@mozilla.com>
># Date 1478683563 -28800
>#      Wed Nov 09 17:26:03 2016 +0800
># Node ID 7fd4f388dad33699b178b4f3b5f25610033e5398
># Parent  e6764b1be721f76ee55c6927040fd73f16923532
> Bug 976073 - Part 2: Modify errors from try server, r=jdm
>
>diff --git a/devtools/client/storage/test/storage-listings.html b/devtools/client/storage/test/storage-listings.html
>--- a/devtools/client/storage/test/storage-listings.html
>+++ b/devtools/client/storage/test/storage-listings.html
>@@ -14,17 +14,17 @@ Bug 970517 - Storage inspector front end
> "use strict";
> let partialHostname = location.hostname.match(/^[^.]+(\..*)$/)[1];
> let cookieExpiresTime1 = 2000000000000;
> let cookieExpiresTime2 = 2000000001000;
> // Setting up some cookies to eat.
> document.cookie = "c1=foobar; expires=" +
>   new Date(cookieExpiresTime1).toGMTString() + "; path=/browser";
> document.cookie = "cs2=sessionCookie; path=/; domain=" + partialHostname;
>-document.cookie = "c3=foobar-2; secure=true; expires=" +
>+document.cookie = "c3=foobar-2; expires=" +
>   new Date(cookieExpiresTime2).toGMTString() + "; path=/";
> // ... and some local storage items ..
> localStorage.setItem("ls1", "foobar");
> localStorage.setItem("ls2", "foobar-2");
> // ... and finally some session storage items too
> sessionStorage.setItem("ss1", "foobar-3");
> dump("added cookies and storage from main page\n");
> 
>diff --git a/devtools/client/storage/test/storage-unsecured-iframe.html b/devtools/client/storage/test/storage-unsecured-iframe.html
>--- a/devtools/client/storage/test/storage-unsecured-iframe.html
>+++ b/devtools/client/storage/test/storage-unsecured-iframe.html
>@@ -4,16 +4,16 @@
> Iframe for testing multiple host detetion in storage actor
> -->
> <head>
>   <meta charset="utf-8">
> </head>
> <body>
> <script>
> "use strict";
>-document.cookie = "uc1=foobar; domain=.example.org; path=/; secure=true";
>+document.cookie = "uc1=foobar; domain=.example.org; path=/";
> localStorage.setItem("iframe-u-ls1", "foobar");
> sessionStorage.setItem("iframe-u-ss1", "foobar1");
> sessionStorage.setItem("iframe-u-ss2", "foobar2");
> dump("added cookies and storage from unsecured iframe\n");
> </script>
> </body>
> </html>
>diff --git a/devtools/server/tests/browser/browser_storage_listings.js b/devtools/server/tests/browser/browser_storage_listings.js
>--- a/devtools/server/tests/browser/browser_storage_listings.js
>+++ b/devtools/server/tests/browser/browser_storage_listings.js
>@@ -30,17 +30,17 @@ const storeMap = {
>       },
>       {
>         name: "c3",
>         value: "foobar-2",
>         expires: 2000000001000,
>         path: "/",
>         host: "test1.example.org",
>         isDomain: false,
>-        isSecure: true,
>+        isSecure: false,
>       },
>       {
>         name: "uc1",
>         value: "foobar",
>         host: ".example.org",
>         path: "/",
>         expires: 0,
>         isDomain: true,
>@@ -339,17 +339,24 @@ function* testStores(data) {
> function testCookies(cookiesActor) {
>   is(Object.keys(cookiesActor.hosts).length, 2, "Correct number of host entries for cookies");
>   return testCookiesObjects(0, cookiesActor.hosts, cookiesActor);
> }
> 
> var testCookiesObjects = Task.async(function* (index, hosts, cookiesActor) {
>   let host = Object.keys(hosts)[index];
>   let matchItems = data => {
>-    is(data.total, storeMap.cookies[host].length,
>+    let cookiesLength = 0;
>+    for (let secureCookie of storeMap.cookies[host]) {
>+       if (secureCookie.isSecure) {
>+          ++cookiesLength;
>+       }
>+    }
>+    // Any secure cookies did not get stored in the database.
>+    is(data.total, storeMap.cookies[host].length - cookiesLength,
>        "Number of cookies in host " + host + " matches");
>     for (let item of data.data) {
>       let found = false;
>       for (let toMatch of storeMap.cookies[host]) {
>         if (item.name == toMatch.name) {
>           found = true;
>           ok(true, "Found cookie " + item.name + " in response");
>           is(item.value.str, toMatch.value, "The value matches.");
>diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json
>--- a/testing/web-platform/meta/MANIFEST.json
>+++ b/testing/web-platform/meta/MANIFEST.json
>@@ -14077,28 +14077,20 @@
>         "path": "content-security-policy/svg/svg-policy-with-resource.html",
>         "url": "/content-security-policy/svg/svg-policy-with-resource.html"
>       },
>       {
>         "path": "cookies/secure/set-from-dom.https.sub.html",
>         "url": "/cookies/secure/set-from-dom.https.sub.html"
>       },
>       {
>-        "path": "cookies/secure/set-from-dom.sub.html",
>-        "url": "/cookies/secure/set-from-dom.sub.html"
>-      },
>-      {
>         "path": "cookies/secure/set-from-http.https.sub.html",
>         "url": "/cookies/secure/set-from-http.https.sub.html"
>       },
>       {
>-        "path": "cookies/secure/set-from-http.sub.html",
>-        "url": "/cookies/secure/set-from-http.sub.html"
>-      },
>-      {
>         "path": "cookies/secure/set-from-ws.https.sub.html",
>         "url": "/cookies/secure/set-from-ws.https.sub.html"
>       },
>       {
>         "path": "cookies/secure/set-from-wss.https.sub.html",
>         "url": "/cookies/secure/set-from-wss.https.sub.html"
>       },
>       {
>diff --git a/testing/web-platform/meta/cookies/secure/set-from-dom.sub.html.ini b/testing/web-platform/meta/cookies/secure/set-from-dom.sub.html.ini
>deleted file mode 100644
>--- a/testing/web-platform/meta/cookies/secure/set-from-dom.sub.html.ini
>+++ /dev/null
>@@ -1,5 +0,0 @@
>-[set-from-dom.sub.html]
>-  type: testharness
>-  ['secure' cookie not sent in HTTP request]
>-    expected: FAIL
>-
>diff --git a/testing/web-platform/meta/cookies/secure/set-from-http.sub.html.ini b/testing/web-platform/meta/cookies/secure/set-from-http.sub.html.ini
>deleted file mode 100644
>--- a/testing/web-platform/meta/cookies/secure/set-from-http.sub.html.ini
>+++ /dev/null
>@@ -1,5 +0,0 @@
>-[set-from-http.sub.html]
>-  type: testharness
>-  ['secure' cookie not sent in HTTP request]
>-    expected: FAIL
>-
>diff --git a/testing/web-platform/tests/cookies/secure/set-from-dom.sub.html b/testing/web-platform/tests/cookies/secure/set-from-dom.sub.html
>deleted file mode 100644
>--- a/testing/web-platform/tests/cookies/secure/set-from-dom.sub.html
>+++ /dev/null
>@@ -1,47 +0,0 @@
>-<!doctype html>
>-<html>
>-<head>
>-  <meta charset=utf-8>
>-  <title>Set 'secure' cookie from `document.cookie` on a non-secure page</title>
>-  <meta name=help href="https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone">
>-  <script src="/resources/testharness.js"></script>
>-  <script src="/resources/testharnessreport.js"></script>
>-  <script src="/cookies/resources/testharness-helpers.js"></script>
>-</head>
>-<body>
>-<div id=log></div>
>-<script>
>-  var tests = [
>-    [
>-      "'secure' cookie not set in `document.cookie`",
>-      function () {
>-        var originalCookie = document.cookie;
>-        document.cookie = "secure_from_nonsecure_dom=1; secure; path=/";
>-        assert_equals(document.cookie, originalCookie);
>-        this.done();
>-      }
>-    ],
>-    [
>-      "'secure' cookie not sent in HTTP request",
>-      function () {
>-        document.cookie = "secure_from_nonsecure_dom=1; secure; path=/";
>-        fetch("https://{{host}}:{{ports[https][0]}}/cookies/resources/echo-json.py", { "credentials": "include" })
>-          .then(this.step_func(function (r) {
>-              return r.json();
>-          }))
>-          .then(this.step_func_done(function (j) {
>-              assert_equals(j["secure_from_nonsecure_dom"], undefined);
>-          }));
>-      }
>-    ]
>-  ];
>-
>-  function clearKnownCookie() {
>-    document.cookie = "secure_from_nonsecure_dom=0; Secure; expires=Thu, 01 Jan 1970 00:00:01 GMT; path=/";
>-  }
>-
>-  executeTestsSerially(tests, clearKnownCookie, clearKnownCookie);
>-</script>
>-</body>
>-</html>
>-
>diff --git a/testing/web-platform/tests/cookies/secure/set-from-http.sub.html b/testing/web-platform/tests/cookies/secure/set-from-http.sub.html
>deleted file mode 100644
>--- a/testing/web-platform/tests/cookies/secure/set-from-http.sub.html
>+++ /dev/null
>@@ -1,36 +0,0 @@
>-<!doctype html>
>-<html>
>-<head>
>-  <meta charset=utf-8>
>-  <title>Set 'secure' cookie from `Set-Cookie` HTTP header on a non-secure page</title>
>-  <meta name=help href="https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone">
>-  <script src="/resources/testharness.js"></script>
>-  <script src="/resources/testharnessreport.js"></script>
>-  <script src="/cookies/resources/testharness-helpers.js"></script>
>-</head>
>-<body>
>-<div id=log></div>
>-<script>
>-  function clearKnownCookie() {
>-    document.cookie = "secure_from_nonsecure_http=0; Secure; expires=Thu, 01 Jan 1970 00:00:01 GMT; path=/";
>-  }
>-
>-  test(function () {
>-    assert_equals(document.cookie.match(/secure_from_nonsecure_http=1/), null);
>-  }, "'secure' cookie not present in `document.cookie`");
>-
>-  promise_test(function (t) {
>-    t.add_cleanup(clearKnownCookie);
>-    return fetch("https://{{host}}:{{ports[https][0]}}/cookies/resources/echo-json.py",
>-                 { "credentials": "include" })
>-      .then(function (r) {
>-        return r.json();
>-      })
>-      .then(function (j) {
>-        assert_equals(j["secure_from_nonsecure_http"], undefined);
>-      });
>-  }, "'secure' cookie not sent in HTTP request");
>-</script>
>-</body>
>-</html>
>-

	Pulsebot
	Comment 93 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a0b004f799
Part 1 : Implement spec, r=jdm r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b86094c4eafc
Part 2: Modify errors from try server, r=jdm

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 94 • 8 years ago

Did you mean to r+ the removal of those *tests*?

	Josh Matthews [:jdm]
	Comment 95 • 8 years ago

Comment on attachment 8808950 [details] [diff] [review]
Part 2: Modify errors from try server, r=jdm

Nope! I presume removing the WPT test files was an accident, Amy?

	Josh Matthews [:jdm]
	Comment 96 • 8 years ago

Oh, I see - my "this file can be removed" comment was referring to the ini file that it was commenting on, rather than the associated test file.

	Carsten Book [:Tomcat]
	Comment 97 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/97a0b004f799
https://hg.mozilla.org/mozilla-central/rev/b86094c4eafc

	Amy Chung [:Amy] Assignee
	Comment 98 • 8 years ago

Hi Josh,
I thought the comment "this file can be removed" which meant the ini files and the test files all need to remove.
Do I have to open a new bug for reverting set-from-http.sub.html and set-from-dom.sub.html?

Thanks!

	Georg Fritzsche [:gfritzsche]
	Comment 99 • 8 years ago

Comment on attachment 8808951 [details] [diff] [review]
Part 1: Implement spec, r= jdm & dveditz

Review of attachment 8808951 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +7728,5 @@
>      "n_values": 10,
>      "releaseChannelCollection": "opt-out",
>      "description": "How often are secure cookies set from non-secure origins, and vice-versa? 0=nonsecure/http, 1=nonsecure/https, 2=secure/http, 3=secure/https"
>    },
> +  "COOKIE_LEAVE_SECURE_ALONE": {

This is a new data collection that needs data collection review (even more so for opt-out probes):
https://wiki.mozilla.org/Firefox/Data_Collection

::: toolkit/components/telemetry/histogram-whitelists.json
@@ +902,5 @@
>      "COMPOSITE_FRAME_ROUNDTRIP_TIME",
>      "COMPOSITE_TIME",
>      "CONTENT_DOCUMENTS_DESTROYED",
>      "COOKIE_SCHEME_SECURITY",
> +    "COOKIE_LEAVE_SECURE_ALONE",

You should add a bug numbers field to the histogram entry instead of adding it to the whitelist.
The whitelist is only for old probes that didn't have the field yet.

	Amy Chung [:Amy] Assignee
	Comment 100 • 8 years ago

Hi Georg,
I will open a new bug and ask data collection peer to review the patch of telemetry.
Thanks your suggestions.

	Amy Chung [:Amy] Assignee
	Comment 101 • 8 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1316542
Bug 1316542 - Add a bug number field to a histogram entry "COOKIE_LEAVE_SECURE_ALONE"

Have to ask data collection peer to review the patch, and add a add a bug number field to a histogram entry "COOKIE_LEAVE_SECURE_ALONE".

	Amy Chung [:Amy] Assignee
	Comment 102 • 8 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1316532
Bug 1316532 - Revert the test cases "set-from-dom.sub.html" and "set-from-http.sub.html" of web-platform.

Have to revert the test cases "set-from-dom.sub.html" and "set-from-http.sub.html" of web-platform.

	Daniel Veditz [:dveditz]
	Comment 103 • 7 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]:
Protects cookies with the secure flag from tampering by an http MITM spoofing an insecure sibling domain. May affect sites which set secure cookies from an insecure site (which is no longer allowed) or which use a mixture of secure and insecure cookies with the same name.

[Affects Firefox for Android]:
Yes

[Suggested wording]:
Implemented the Strict Secure Cookies spec which forbids insecure (http:) sites from setting cookies with the "secure" attribute, and in some cases prevents an insecure site from setting a cookie with the same name as an existing "secure" cookie from the same base domain.

[Links (documentation, blog post, etc)]:
We will update the MDN pages:
  https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
  https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies
Spec:
  https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone/

	Julien Cristau [:jcristau]
	Comment 104 • 7 years ago

added to the release notes for 52

	Florian Scholz (Open Web Docs)
	Comment 105 • 7 years ago

Added the spec to 
https://developer.mozilla.org/en-US/docs/Web/HTTP/Resources_and_specifications

Added an entry to the developer release notes for 52
https://developer.mozilla.org/en-US/Firefox/Releases/52#HTTP

Updated
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Secure_and_HttpOnly_cookies