1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Jonathan Hao (inactive) [:jhao]]

Quote from David in https://bugzilla.mozilla.org/show_bug.cgi?id=1323644#c36

> The isSecureHost instances can be replaced with isSecureURI. If any add-ons really need unsafeProcessHeader, they can use processHeader with a fake SSL status like our tests do: https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/security/manager/ssl/tests/unit/head_psm.js#668
> Since this bug (1323644) will add some complexity to nsISiteSecurityService/nsSiteSecurityService, I think removing some unnecessary aspects beforehand would be beneficial.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1336867 - Move STSParserTest.cpp to test_sts_parser.js

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

Comment 2

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

Comment on attachment 8833952 [details]
Bug 1336867 - Move STSParserTest.cpp to test_sts_parser.js

https://reviewboard.mozilla.org/r/110028/#review111210

Great! Thanks for doing this.
My one overall comment would be that the tests aren't very consistent with regard to whether they're making http:// or https:// URIs for use with the nsISiteSecurityService APIs. Technically it doesn't matter, but it would be nice to pick one and stick with it. Maybe https:// would be best so we don't get any false-positives for "this might be referring to a thing that's insecure and so needs to be looked at" in the future?
Anyway, lgtm with comments addressed.

::: browser/base/content/browser.js:3302
(Diff revision 1)
>    certErrorDetails += "\r\n\r\n" + errors.getErrorMessage(errors.getXPCOMFromNSSError(code));
>  
>    const sss = Cc["@mozilla.org/ssservice;1"]
>                   .getService(Ci.nsISiteSecurityService);
>    // SiteSecurityService uses different storage if the channel is
> -  // private. Thus we must give isSecureHost correct flags or we
> +  // private. Thus we must give isSecureURI correct flags or we

We should get a browser/devtools peer to review these changes as well.

::: devtools/client/debugger/new/debugger.js:8970
(Diff revision 1)
>  	        // might get incorrect results.
>  	        let flags = (httpActivity.private) ?
>  	                      Ci.nsISocketProvider.NO_PERMANENT_STORAGE : 0;
>  	
>  	        let host = httpActivity.hostname;
> -	
> +	        let uri = Services.io.newURI("https://" + uri);

This should be `"https://" + host`, right?

::: security/manager/ssl/SSLServerCertVerification.cpp:515
(Diff revision 1)
>              mFdForLogging, this));
>      return new SSLServerCertVerificationResult(mInfoObject,
>                                                 mDefaultErrorCodeToReport);
>    }
> -  nsresult nsrv = sss->IsSecureHost(nsISiteSecurityService::HEADER_HSTS,
> -                                    mInfoObject->GetHostName(),
> +  nsCOMPtr<nsIURI> uri;
> +  NS_NewURI(getter_AddRefs(uri),

nit: we should check the return value from NS_NewURI (and return a similar SSLServerCertVerificationResult like below)

::: security/manager/ssl/tests/unit/test_sts_fqdn.js:25
(Diff revision 1)
> -
>    ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0));
> -  uri = Services.io.newURI("https://example.com.");
> -  ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0));
> -  uri = Services.io.newURI("https://example.com..");
> +  ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri1, 0));
> +  ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri2, 0));
> +
>    ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0));

Since isSecureHost is now isSecureURI, these are actually duplicate testcases, so we don't need them.

::: security/manager/ssl/tests/unit/test_sts_holepunch.js:31
(Diff revision 1)
> -                            "sub.example.apis.google.com", 0));
> -  ok(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> -                            "SUB.EXAMPLE.APIS.GOOGLE.COM", 0));
> -  // also check isSecureURI
> -  let chartURI = Services.io.newURI("http://chart.apis.google.com");
> -  ok(!SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, chartURI, 0));
> +                           Services.io.newURI("http://example.apis.google.com"),
> +                           0));
> +  ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                           Services.io.newURI("http://EXAMPLE.APIS.GOOGLE.COM"),
> +                           0));
> +  ok(SSService.isSecureURI(

Same thing - these are duplicate testcases now.

::: security/manager/ssl/tests/unit/test_sts_ipv4_ipv6.js:17
(Diff revision 1)
>      str += "]";
>    }
>    str += "/";
>  
>    let uri = Services.io.newURI(str);
> +  ok(!s.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0));

Hmmm - seems like this should go after the call to processHeader.

::: security/manager/ssl/tests/unit/test_sts_parser.js:1
(Diff revision 1)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-

It would be nice for the sake of code history to `hg mv` the original test to this file and then just overwrite its contents with how this file is now.

::: security/manager/ssl/tests/unit/test_sts_parser.js:71
(Diff revision 1)
> +    // trailing semicolon)
> +    testSuccess("max-age=100;includeSubdomains;", 100, true);
> +
> +    // these are weird tests, but are testing that some extended syntax is
> +    // still allowed (but it is ignored)
> +    testSuccess("max-age=100 ; includesubdomainsSomeStuff", 100, false);

Technically we're losing coverage of the NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA cases, but nothing actually depends on this feature, so I think that's fine (we should probably just remove it, but that can be a follow-up for later).

::: security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js:123
(Diff revision 1)
>    //     |   `-- another.subdomain.bugzilla.mozilla.org              IS sts host
>    //     `-- sibling.bugzilla.mozilla.org                            IS sts host
> -  ok(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> -                             "bugzilla.mozilla.org", 0));
> -  ok(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> -                             "subdomain.bugzilla.mozilla.org", 0));
> +  ok(gSSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                            Services.io.newURI("http://bugzilla.mozilla.org"),
> +                            0));
> +  ok(gSSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0));

For consistency and clarity, it might be best to use `Services.io.newURI("http://subdomain.bugzilla.mozilla.org")` here instead of `uri`.

::: services/common/tests/unit/test_blocklist_pinning.js:95
(Diff revision 1)
>  
>    let sss = Cc["@mozilla.org/ssservice;1"]
>                .getService(Ci.nsISiteSecurityService);
>  
>    // ensure our pins are all missing before we start
> -  ok(!sss.isSecureHost(sss.HEADER_HPKP, "one.example.com", 0));
> +  ok(!sss.isSecureURI(sss.HEADER_HPKP,

Looks like :leplatrem reviewed this originally, so it might be good to get sign-off from him.

::: services/common/tests/unit/test_blocklist_pinning.js:102
(Diff revision 1)
> -  ok(!sss.isSecureHost(sss.HEADER_HPKP, "three.example.com", 0));
> -  ok(!sss.isSecureHost(sss.HEADER_HSTS, "five.example.com", 0));
> +  ok(!sss.isSecureURI(sss.HEADER_HPKP,
> +                      Services.io.newURI("https://two.example.com"), 0));
> +  ok(!sss.isSecureURI(sss.HEADER_HPKP,
> +                      Services.io.newURI("https://three.example.com"),
> +                      0));
> +  ok(!sss.isSecureURI(sss.HEADER_HSTS,

Shouldn't we also be checking four.example.com here?

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

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

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8833952 [details]
Bug 1336867 - Move STSParserTest.cpp to test_sts_parser.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110028/diff/1-2/

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8833952 [details]
Bug 1336867 - Move STSParserTest.cpp to test_sts_parser.js

https://reviewboard.mozilla.org/r/110028/#review111210

> Same thing - these are duplicate testcases now.

I don't think these are duplicate testcases.  There are 8 isSecureURI() calls left, which correspond to the 8 isSecureHost() calls before change.

> Hmmm - seems like this should go after the call to processHeader.

This corresponds to the isSecureHost in line 5 before the call to processHeader.  I could add another after the call to processHeader if that's what you think.

> It would be nice for the sake of code history to `hg mv` the original test to this file and then just overwrite its contents with how this file is now.

Good idea.  It seems git-cinnabar has a bug about modifying after renaming, so I put them in different commits.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Hi Panos, could you take a look at the browser/devtools part?

Hi Mathieu, would you sign off this patch for us?

Thanks.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

https://reviewboard.mozilla.org/r/110242/#review111476

::: devtools/client/debugger/new/debugger.js:8970
(Diff revision 1)
>  	        // might get incorrect results.
>  	        let flags = (httpActivity.private) ?
>  	                      Ci.nsISocketProvider.NO_PERMANENT_STORAGE : 0;
>  	
>  	        let host = httpActivity.hostname;
> -	
> +	        let uri = Services.io.newURI("https://" + host);

There is a uri variable in the parent scope that you are now hiding. Why not reuse it if not null?

Also, you are manually prepending https, but what is the guarantee that this is not an http connection (the |info.state == "insecure"| branch above)?

Same comment for all 3 instances of this pattern in devtools/.

Comment 8

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

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

https://reviewboard.mozilla.org/r/110242/#review111814

::: security/manager/ssl/SSLServerCertVerification.cpp:520
(Diff revision 1)
> -                                    mInfoObject->GetHostName(),
> +  nsresult nsrv = NS_NewURI(getter_AddRefs(uri),
> +                            NS_LITERAL_CSTRING("https://") +
> +                            mInfoObject->GetHostName());
> +  if (NS_FAILED(nsrv)) {
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
> +           ("[%p][%p] Creating new URI failed\n", mFdForLogging, this));

nit: for new code, it would be nice to align things like this one more space to the right (the current pattern is an artifact of a search-and-replace change).

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

https://reviewboard.mozilla.org/r/110242/#review111476

> There is a uri variable in the parent scope that you are now hiding. Why not reuse it if not null?
> 
> Also, you are manually prepending https, but what is the guarantee that this is not an http connection (the |info.state == "insecure"| branch above)?
> 
> Same comment for all 3 instances of this pattern in devtools/.

Thanks Panos for pointing this out.  I didn't notice there is a uri in the parent scope, and in fact, isSecureURI only uses the host of URI and doesn't care about the scheme.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110242/diff/1-2/

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

https://reviewboard.mozilla.org/r/110242/#review111916

::: devtools/client/debugger/new/debugger.js:8969
(Diff revision 2)
> -	        // private. Thus we must give isSecureHost correct flags or we
> +	        // private. Thus we must give isSecureURI correct flags or we
>  	        // might get incorrect results.
>  	        let flags = (httpActivity.private) ?
>  	                      Ci.nsISocketProvider.NO_PERMANENT_STORAGE : 0;
>  	
>  	        let host = httpActivity.hostname;

Thanks, you should now move the |host| declaration inside the if branch, as it's not used elsewhere. r=me with this change in all 3 cases.

Comment 12

[Mathieu Leplatre [:leplatrem]]

I don't think I would be a relevant reviewer for these changes :( Could you transfer to someone else (like :mgoodwin)?

Comment 13

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110242/diff/2-3/

Comment 14

[Jonathan Hao (inactive) [:jhao]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #12)
> I don't think I would be a relevant reviewer for these changes :( Could you
> transfer to someone else (like :mgoodwin)?

Hi Mark, could you review the changes in test_blockpinning.js?

Comment 15

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

https://reviewboard.mozilla.org/r/110242/#review112492

From the bugzilla comments I see you're mostly interested in my review for the blocklist test; this looks good to me.

Comment 16

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110242/diff/3-4/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/837766805d75
Move STSParserTest.cpp to test_sts_parser.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/0a4f283638cb
Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService r=keeler,mgoodwin,past

Comment 18

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/e9cc5acc8481 for https://treeherder.mozilla.org/logviewer.html#?job_id=76168272&repo=autoland

Comment 19

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8834235 [details]
Bug 1336867 - Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110242/diff/4-5/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb8c18d185e9
Move STSParserTest.cpp to test_sts_parser.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/b3b730daa255
Remove unsafeProcessHeader and isSecureHost in nsISiteSecurityService r=keeler,mgoodwin,past

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb8c18d185e9
https://hg.mozilla.org/mozilla-central/rev/b3b730daa255