887052 - refactor nsStrictTransportSecurityService

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: patch

Eventually OCSP-must-staple will exist, and as I understand it, it will look a whole lot like Strict-Transport-Security. So, nsStrictTransportSecurityService should be refactored into a more general site security info service.

My plan is to put the handling of private mode / not private mode, parent domains that set includeSubdomains, expiration, and preload lists into a utility class that nsStrictTransportSecurityService and the eventual nsOCSPMustStapleService (or whatever) can make use of.

I wrote this on top of my patch for bug 846825. It isn't entirely necessary, but we will want to complete that before OCSP-must-staple (because the header parsing is still specific to what type of security info we're talking about, and the refactoring in that bug helps with this).

Comment 1

[Camilo Viecco (:cviecco)]

Comment on attachment 767494 [details] [diff] [review]
patch

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

David, I wished that I could use something like this for the Pinning information, however the call to get the pinning data happens not in the main thread. What do you think we could do to make the getsecurityinfo call fast off the main thread? (I think the latency would be of around 100 ms)

Comment 2

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

To really make that work, we would probably need a thread-safe data store for this security information we're collecting. I think there's a plan to stop using the permission manager anyway, right?

Comment 3

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

Comment on attachment 767494 [details] [diff] [review]
patch

We talked about how we're going to do this a little differently, so clearing feedback for now.

Comment 4

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

Attachment: patch 1/2

This renames nsIStrictTransportSecurityService to nsISiteSecurityService in preparation of refactoring it to handle multiple types of security header.

Comment 5

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

Attachment: patch 2/2

This patch refactors nsISiteSecurityService to be able to handle multiple types of security headers. Currently, only HSTS is supported.

Comment 6

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

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

Review to Camilo instead.

Comment 7

[Camilo Viecco (:cviecco)]

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

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

Looks good (did not comlie it)

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 769755 [details] [diff] [review]
patch 2/2

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

The interface you have might not be the right one for HKPK but lets fix that later

Comment 9

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

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

Hi, Patrick - I was wondering if you could do a sr on these patches. The purpose of this bug is to refactor nsIStrictTransportSecurityService into a more generalized nsISiteSecurityService that can handle multiple types of security property (where we currently support HSTS and will eventually support key pinning and OCSP-must-staple).

This patch (1 of 2) consists of a number of renames like nsIStrictTransportSecurityService to nsISiteSecurityService.

Comment 10

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

Comment on attachment 769755 [details] [diff] [review]
patch 2/2

This patch (2 of 2) generalizes the functions in nsISiteSecurityService (previously nsIStrictTransportSecurityService) to take a parameter that specifies which type of security property to operate on. Currently only HSTS is supported.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 769755 [details] [diff] [review]
patch 2/2

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

I'll give you an r+ for the netwerk/* bits but I'm not a sr'r, so I can't do that.

::: netwerk/base/public/nsISiteSecurityService.idl
@@ +55,5 @@
>      /**
>       * Checks if the given security info is for an STS host with a broken
>       * transport layer (certificate errors like invalid CN).
>       */
> +    boolean shouldIgnoreHeader(in nsISupports aSecurityInfo);

why does this one not need a type param?

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

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

I'll give you an r+ for the netwerk/* bits but I'm not a sr'r, so I can't do that.

Comment 13

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

Thank you for the quick review. My understanding was that the meaning of a super-review changed to be that interface changes require sr+ from the module owner (hence, every module owner is the super-reviewer for that module).

(In reply to Patrick McManus [:mcmanus] [afk until Aug 09] from comment #11)
> Comment on attachment 769755 [details] [diff] [review]
> ::: netwerk/base/public/nsISiteSecurityService.idl
> @@ +55,5 @@
> >      /**
> >       * Checks if the given security info is for an STS host with a broken
> >       * transport layer (certificate errors like invalid CN).
> >       */
> > +    boolean shouldIgnoreHeader(in nsISupports aSecurityInfo);
> 
> why does this one not need a type param?

Basically, this is a utility function that, given the security information of a connection, says whether or not any information from it is trustworthy. I'm actually not sure this belongs in nsISiteSecurityService, but that can be a follow-up bug.

Comment 14

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

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

bz - we spoke on IRC about reviewing these patches (this is 1 of 2). The comments asking mcmanus for sr should explain the purpose of these changes, but let me know if you need more context. Thanks!

Comment 15

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

Comment on attachment 769755 [details] [diff] [review]
patch 2/2

(2 of 2)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er, wait.  I thought you wanted sr on these, not detailed code review.  If you want detailed review, I'm probably the wrong choice; it would be better to get module owners/peers for the affected modules instead...

Comment 17

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

Sorry - my intention was to ask for a code review. I can get code reviews from owners/peers of the relevant modules instead. I thought mcmanus would be a good choice for sr, since he's the necko module owner, but there seems to be some confusion on who is able to give sr+ for these changes.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

David, please ask the people who own the relevant code, at least if review is desired before about two weeks from now.  :(

Comment 19

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

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

bz - sorry about the confusion. If you have time for a sr of the idl changes, that would be great.

jst - I can't seem to raise you on irc, so maybe you're reading bugmail. If you could review the changes in content and the docshell, I would appreciate it. Previous comments should explain the rationale for these changes, but let me know if you would like more context. Thanks!

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 769747 [details] [diff] [review]
patch 1/2

- In netwerk/base/public/nsISiteSecurityService.idl:

-[scriptable, uuid(c6138514-f212-4747-98c2-7abfce3be293)]
-interface nsIStrictTransportSecurityService : nsISupports
+[scriptable, uuid(1ca9de3d-26b8-4e0c-9641-62c380bdd9c7)]
+interface nsISiteSecurityService : nsISupports

Since there's no binary incompatible changes here (just a rename), I don't see a reason to have to update the uuid here.

Given how mechanical this change is, and that it's really just a rename, I'm going to r+sr this whole patch.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 769755 [details] [diff] [review]
patch 2/2

netwerk/base/public/nsISiteSecurityService.idl:

+    boolean shouldIgnoreHeader(in nsISupports aSecurityInfo);

Given that this method really answers the question whether all security headers should be ignored, I think we should make header in the name be plural, i.e. shouldIgnoreHeaders().

r+sr=jst on this patch too, given that necko and security folks have already looked at this as well.

Comment 22

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

https://tbpl.mozilla.org/?tree=Try&rev=89a4f7e8bf15

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/97fcb5a154d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8a88c6d0c8

Comment 24

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

Attachment: bogus patch 1/2, ignore me

Had to deal with merge conflicts. Here's patch 1/2 as checked in. Carrying over all the reviews.

Comment 25

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

Attachment: bogus patch 2/2, ignore me

Comment 26

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

This will need some documentation. The main points are:
1. nsIStrictTransportSecurityService is now nsISiteSecurityService
2. The functionality is (currently) the same, but nsISiteSecurityService::HEADER_HSTS must be specified as an additional parameter to each function in the interface (in the future, different headers will be supported)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX mochitest-other orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8102ebf36ce9

https://tbpl.mozilla.org/php/getParsedLog.php?id=26173750&tree=Mozilla-Inbound

Comment 28

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

Somehow I pushed an old copy of the patches. This should work:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b44c631a881a
https://hg.mozilla.org/integration/mozilla-inbound/rev/25593587283a

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b44c631a881a
https://hg.mozilla.org/mozilla-central/rev/25593587283a

Comment 30

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

Attachment: patch 1/2 as landed

Ok - now that this landed on central, here's the actual patches.

Comment 31

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

Attachment: patch 2/2 as landed

Comment 32

[Kai Engert [:KaiE:]]

In your commit
  http://hg.mozilla.org/mozilla-central/rev/25593587283a

you probably landed broken test code.

http://hg.mozilla.org/mozilla-central/rev/25593587283a#l10.14
   10.14 +      gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HSTA, uri,

I think that should have been HEADER_HSTS, because I cannot find symbol HEADER_HSTA anywhere else.

Comment 33

[Kai Engert [:KaiE:]]

Attachment: fix HSTA

Comment 34

[Kai Engert [:KaiE:]]

If you agree to the fix, please feel free to go ahead and check it in.

Comment 35

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

Comment on attachment 796285 [details] [diff] [review]
fix HSTA

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

Good catch.

Comment 36

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b401ae426727

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b401ae426727