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

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1290529 - Clear HSTS and HPKP for subdomains in ForgetAboutSite.

Review commit: https://reviewboard.mozilla.org/r/105396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105396/

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Hi Matt and Mark, would you review this patch, please?  This patch depends on the one in bug 1115712.

Comment 3

[Matthew N. [:MattN]]

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.

Comment 4

[Jonathan Hao (inactive) [:jhao]]

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/1-2/

Comment 5

[Jonathan Hao (inactive) [:jhao]]

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 6

[Matthew N. [:MattN]]

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?

Comment 7

[Jonathan Hao (inactive) [:jhao]]

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/2-3/

Comment 8

[Jonathan Hao (inactive) [:jhao]]

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 9

[Matthew N. [:MattN]]

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"

Comment 10

[Jonathan Hao (inactive) [:jhao]]

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/3-4/

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Attachment: Clear HSTS and HPKP for subdomains in ForgetAboutSite.

The WIP patch for the new test that David suggested.

Comment 13

[Jonathan Hao (inactive) [:jhao]]

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/

Comment 14

[Jonathan Hao (inactive) [:jhao]]

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

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 16

[Jonathan Hao (inactive) [:jhao]]

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/5-6/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5346a3afb036
Clear HSTS and HPKP for subdomains in ForgetAboutSite. r=keeler,MattN

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5346a3afb036