Closed Bug 1290529 Opened 3 years ago Closed 3 years ago

clear HSTS and HPKP for subdomains as well when bug 1115712 is fixed

Categories

(Toolkit :: Data Sanitization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- affected
firefox54 --- fixed

People

(Reporter: keeler, Assigned: jhao)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

bug 1119778 will ensure that HSTS and HPKP will be cleared for the exact domain used with "Forget About This Site", but ideally it should clear that information for subdomains as well. Until bug 1115712 is fixed, this isn't possible.
Blocks: 1323644
Assignee: nobody → jhao
Status: NEW → ASSIGNED
Hi Matt and Mark, would you review this patch, please?  This patch depends on the one in bug 1115712.
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review106390

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:210
(Diff revision 1)
> +          let hostname = entry.QueryInterface(Ci.nsISiteSecurityState).hostname;
> +          if (hostname.endsWith("." + aDomain)) {
> +            let uri = caUtils.makeURI("https://" + hostname);
> +            sss.removeState(type, uri, 0);
> +          }

Please add a comment explaining what this is about.
Attachment #8827806 - Flags: review?(MattN+bmo)
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review106390

> Please add a comment explaining what this is about.

Done.  Please take a look again.  Thanks.
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review106518

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:212
(Diff revision 2)
> +          let hostname = entry.QueryInterface(Ci.nsISiteSecurityState).hostname;
> +          if (hostname.endsWith("." + aDomain)) {
> +            let uri = caUtils.makeURI("https://" + hostname);
> +            sss.removeState(type, uri, 0);
> +          }

I meant the 4 lines I quoted with the endsWith and makeURI. What is the `if` doing and why do we need ot make a new URI?
Attachment #8827806 - Flags: review?(MattN+bmo)
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review106518

> I meant the 4 lines I quoted with the endsWith and makeURI. What is the `if` doing and why do we need ot make a new URI?

I've added more comments.  Thanks.
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review106566

Thanks

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:213
(Diff revision 3)
> +        // information in the site security service.
> +        let enumerator = sss.enumerate(type);
> +        while (enumerator.hasMoreElements()) {
> +          let entry = enumerator.getNext();
> +          let hostname = entry.QueryInterface(Ci.nsISiteSecurityState).hostname;
> +          // If the hostname is aDomain's subdomain, we remove it's state.

Remove the apostrophe in "it's"
Attachment #8827806 - Flags: review?(MattN+bmo) → review+
Attachment #8827806 - Flags: review?(mgoodwin) → review?(dkeeler)
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review107594

PSM test looks good to me, although I noted an improvement that probably should have been there in the first version.

::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:95
(Diff revision 4)
>    yield ForgetAboutSite.removeDataFromDomain("example.com");
>  
> -  // TODO (bug 1290529):
> -  // Assert.ok(!sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> -  //                             "a.pinning2.example.com", 0),
> -  //           "a.pinning2.example.com should not be HSTS now (subdomain case)");
> +  Assert.ok(!sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                              "a.pinning2.example.com", 0),
> +            "a.pinning2.example.com should not be HSTS now (subdomain case)");
> +  Assert.ok(!sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP,

It occurs to me we should probably add a test that unrelated sites don't also get removed when example.com is forgotten (as in, also process headers for something like example.org and ensure that it isn't affected).
Attachment #8827806 - Flags: review?(dkeeler) → review+
The WIP patch for the new test that David suggested.
Attachment #8829698 - Attachment is obsolete: true
(In reply to Jonathan Hao [:jhao] from comment #13)
> Comment on attachment 8827806 [details]
> Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/105396/diff/4-5/

David, is this what you want?
Flags: needinfo?(dkeeler)
Comment on attachment 8827806 [details]
Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

https://reviewboard.mozilla.org/r/105396/#review107944

Yep - that's what I was thinking. LGTM (just the nit about the comments)

::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:97
(Diff revisions 4 - 5)
> +  let unrelatedURI = Services.io.newURI("https://example.org");
> +  sss.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, unrelatedURI,
> +                    GOOD_MAX_AGE, sslStatus, 0);
> +  Assert.ok(sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                             "example.org", 0),
> +            "example.org should be HSTS (subdomain case)");

nit: I would take out the "(subdomain case)" bit

::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:110
(Diff revisions 4 - 5)
>                                "a.pinning2.example.com", 0),
>              "a.pinning2.example.com should not be HPKP now (subdomain case)");
> +
> +  Assert.ok(sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                             "example.org", 0),
> +            "example.org should still be HSTS (subdomain case)");

same here
Flags: needinfo?(dkeeler)
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5346a3afb036
Clear HSTS and HPKP for subdomains in ForgetAboutSite. r=keeler,MattN
https://hg.mozilla.org/mozilla-central/rev/5346a3afb036
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.