Closed Bug 1282279 Opened 4 years ago Closed 3 years ago

Make user certificates Origin Attribute aware

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: pauljt, Assigned: jhao)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [domsecurity-backlog3][userContextId][tor][OA])

Attachments

(7 files)

User certificate flow is not separated by containers. IE if you choose a user certificate for a given site, that choice is not reset for a new container. I'm unclear if there is any ambient leak of information here (if there is we should definitely fix this) but either way, having a separate flow might be a useful developer feature (and possibly expected user behavior?)

See also this discussion [1]

[1] https://groups.google.com/a/mozilla.com/forum/#!msg/containers/D-2tOdQl00c/kXrnlQAQBAAJ
This sounds like a P1 or P2.  We can discuss in our next meeting.  Setting to P2 for now.
Component: Security → DOM: Security
Priority: -- → P2
Whiteboard: [OA][domsecurity-backlog][userContextId]
Flags: needinfo?(kjozwiak)
Attached file poc.zip
I've managed to reproduce this as well. When you're selecting a client certificate in a particular container, the selected certificate will be used in each container as well as the default container. As Paul mentioned above in comment#0, it would definitely be nice if the client certificates were being segregated per each container. This doesn't affect PB as the user is prompted to select a new client certificate even though the none-PB mode is already using one that has been selected earlier.

* I've attached a test case that can be used with the STR listed below

STR:

* launch the server via "node poc.js"
* visit https://localhost:8000 in a personal container
* add an exception for the self signed certificate via "Advanced -> Add Exception -> Confirm Security Exception"
* you'll receive a "SSL_ERROR_HANDSHAKE_FAILURE_ALERT" error due to the server requiring a client certificate for authentication
* import the user certificate (userA.p12) into FX via "about:preferences#advanced -> Certificates -> View Certificates -> Your Certificates" (pass: mozilla)
* refresh https://localhost:8000 which should still be opened in a personal container
* you'll receive a "This site has requested that you identify yourself with a certificate", use the one we imported and select "OK"
* you should receive a "Hello World User1" webpage
* open a work container via File -> New Container Tab -> Work
* load https://localhost:8000
* you'll receive a "Hello World User1" webpage which means it's using the same client certificate as the personal container
Flags: needinfo?(kjozwiak)
As per IRC, I've attached screenshots of the Certificate UI:

* importClientCert.png - importing client certificates
* ClientCertList.png - listing client certificates that are currently imported
* ClientCertPrompt.png - prompt that's displayed when visiting a website that requires a client certificate and FX found one in the certificate store
Do we have any telemetry around the use of user certificates?
Priority: P2 → P3
Summary: Separate user certificates by containers → Make user certificates Origin Attribute aware
Whiteboard: [OA][domsecurity-backlog][userContextId] → [OA][domsecurity-backlog][userContextId][tor]
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #7)
> Do we have any telemetry around the use of user certificates?

Keeler, do you know if we have any telemetry data around user certificates? If we don't have any data available, do you have a general idea if the feature is widely used? From what I read, it looks like organizations try to stay clear from client certificates to avoid more complexity.
Flags: needinfo?(dkeeler)
As far as I can tell we don't have any telemetry on client certificates (it would be nice to have). It's not very widely used, but neither is it nonexistent (until recently, startssl (a certificate authority) required it to log in and do anything with your account, but it looks like they've changed to some kind of OTP thing). Maybe Chrome folks have data they would be willing to share? (at least at a high level)
Flags: needinfo?(dkeeler)
Removing [oa] since this is more containers than [oa] work. (it will involve new UX).
Whiteboard: [OA][domsecurity-backlog][userContextId][tor] → [domsecurity-backlog][userContextId][tor]
(In reply to Paul Theriault [:pauljt] from comment #10)
> Removing [oa] since this is more containers than [oa] work. (it will involve
> new UX).

Adding [oa] back since apparently this _is_ and oa bug.
Whiteboard: [domsecurity-backlog][userContextId][tor] → [domsecurity-backlog][userContextId][tor][oa]
Whiteboard: [domsecurity-backlog][userContextId][tor][oa] → [domsecurity-backlog][userContextId][tor][OA]
Whiteboard: [domsecurity-backlog][userContextId][tor][OA] → [domsecurity-backlog3][userContextId][tor][OA]
Hi Arthur,

Does Tor browser have this feature (isolate user certificates by first-party URL)?
If so, I would suggest we file another file to track it because Containers and Tor integration projects might have different aspects on this feature.
Flags: needinfo?(arthuredelstein)
(In reply to Ethan Tseng [:ethan] from comment #12)
> Hi Arthur,
> 
> Does Tor browser have this feature (isolate user certificates by first-party
> URL)?

No, Tor Browser does not currently have this feature, but I think it would be desirable.

> If so, I would suggest we file another file to track it because Containers
> and Tor integration projects might have different aspects on this feature.

That's certainly possible -- good idea.
Flags: needinfo?(arthuredelstein)
Arthur,

This appears to be a way to use certificates as an authentication credential.  Has the Tor Browser done anything related to isolating this?
Flags: needinfo?(arthuredelstein)
(In reply to Dave Huseby [:huseby] from comment #14)
> Arthur,
> 
> This appears to be a way to use certificates as an authentication
> credential.  Has the Tor Browser done anything related to isolating this?

We haven't, but probably we should, unless Mozilla gets to it first! :)
Flags: needinfo?(arthuredelstein)
This is an Origin Attributes bug.  It relates to Containers, but for the Containers use case it is really low priority (particularly if the browser does require user interaction each time it is requested).  So we haven't prioritized it.  Sounds like its not a high priority for Tor either, so we'll keep it as a P3 and see who has cycles for it first.

Paul - can you verify that the browser won't provide the user certificate to a website without user interaction, even if the user has previously granted access to that website?
Flags: needinfo?(ptheriault)
Blocks: meta_tor
Priority: P3 → P2
Assignee: nobody → jhao
I'm testing with Kamil's STR and add logs to see in which function we reuse previous chosen user certificate, but I notice that when I load the page in the second container, the server doesn't send me "certificate request" message at all.  It confuses me because I thought it's the browser that remembers previous chosen user certificate and resends again, but it looks like the server itself decides that it doesn't need a certificate again?

Hi David, is this behavior due to server remembering something of the client? Or is there some information that we send to the server that caused this?
Status: NEW → ASSIGNED
Flags: needinfo?(dkeeler)
After further examination, I found that when doing the SSL handshake in the second container, we send the SSL session id of the first container to the server, and therefore all the rest handshake operations are skipped.

Tanvi, we should separate SSL session ids by origin attributes, right?
Flags: needinfo?(tanvi)
Right - we probably shouldn't do session resumption across different origin attributes.
Flags: needinfo?(dkeeler)
We already store firstPartyDomain in nsNSSSocketInfo in bug 1264562.  In bug 1315143, we will replace the firstPartyDomain with the whole origin attributes.  Then, in nsNSSSocketInfo::DriveHandShake(), we should be able to pass the origin attributes to SSL helpers.
Depends on: 1315143
Depends on: 1316283
(In reply to David Keeler [:keeler] (use needinfo?) from comment #19)
> Right - we probably shouldn't do session resumption across different origin
> attributes.

Agreed.
Flags: needinfo?(tanvi)
Flags: needinfo?(ptheriault)
Comment on attachment 8814808 [details]
Bug 1282279 - Tests that make sure user certificates are Origin Attribute aware.

https://reviewboard.mozilla.org/r/95910/#review96594

::: security/manager/pki/resources/content/clientauthask.js:103
(Diff revision 2)
>      }
>    }
>  
>    setDetails();
> +
> +  Services.obs.notifyObservers(document.getElementById("certAuthAsk"), "cert-dialog-loaded", null);

nit: this line is a bit long (we generally keep to <80 characters even in JS code in PSM)
Comment on attachment 8815183 [details]
Bug 1282279 - Make user certificates Origin Attribute aware.

https://reviewboard.mozilla.org/r/96212/#review96596

LGTM. My only concerns are basically about how nsClientAuthRemember.h/cpp are in a terrible state w.r.t. style (there's some other cleanup that can happen, too, so we can defer that to a follow-up but if you like).

::: security/manager/ssl/nsClientAuthRemember.h:122
(Diff revision 1)
>  
>    nsClientAuthRememberService();
>  
>    nsresult Init();
>  
>    static void GetHostWithCert(const nsACString & aHostName, 

This is a preexisting problem, but this function is poorly named. Maybe let's rename it `GetEntryKey` or something? (and rename `_retval` to `entryKey` and annotate it as /* out */ with a comment)

::: security/manager/ssl/nsClientAuthRemember.h:123
(Diff revision 1)
>    nsClientAuthRememberService();
>  
>    nsresult Init();
>  
>    static void GetHostWithCert(const nsACString & aHostName, 
> +                              const mozilla::NeckoOriginAttributes &

Matching the existing style is fine, but if you feel like it, maybe make these changes where it makes sense to do so:

- '&/*' go to the left (so no space before them)
- remove all trailing whitespace
- for functions declarations where the types end up going long, maybe just break the lines early like so:
```
static void GetEntryKey(
  const nsACString& aHostName,
  const mozilla::NeckoOriginAttributes& originAttributes,
  const nsACString& nickname,
  /* out */ nsACString& entryKey);

```
Attachment #8815183 - Flags: review?(dkeeler) → review+
I added many style fixes.  David, do you want to take a look again?
Flags: needinfo?(dkeeler)
Comment on attachment 8815183 [details]
Bug 1282279 - Make user certificates Origin Attribute aware.

https://reviewboard.mozilla.org/r/96212/#review96826

::: security/manager/ssl/nsClientAuthRemember.h:124
(Diff revisions 1 - 2)
>  
>    nsClientAuthRememberService();
>  
>    nsresult Init();
>  
> -  static void GetHostWithCert(const nsACString & aHostName, 
> +  static void GetEntryKey(const nsACString& aHostName,

I guess I missed another style issue - we have a mix of 'a'-prefixed argument names and non-'a'-prefixed names. Currently the style guide says to use the prefixed names, and it looks like this file uses it more often than not. Again, this isn't a big deal if you just want to get this landed - we can address it later.

::: security/manager/ssl/nsClientAuthRemember.h:126
(Diff revisions 1 - 2)
>  
>    nsresult Init();
>  
> -  static void GetHostWithCert(const nsACString & aHostName, 
> -                              const mozilla::NeckoOriginAttributes &
> -                                originAttributes,
> +  static void GetEntryKey(const nsACString& aHostName,
> +                          const NeckoOriginAttributes& originAttributes,
> +                          const nsACString& nickname,

Let's also update the 'nickname' parameter name to 'fingerprint' or something (it's not a nickname - it's the hash of the certificate)
Looks good - thanks!
Flags: needinfo?(dkeeler)
I put style fixes that are unrelated to isolation in a separate patch.
Comment on attachment 8815183 [details]
Bug 1282279 - Make user certificates Origin Attribute aware.

https://reviewboard.mozilla.org/r/96212/#review97248

::: security/manager/ssl/nsClientAuthRemember.h:127
(Diff revision 4)
>    nsresult Init();
>  
> -  static void GetHostWithCert(const nsACString & aHostName, 
> -                              const nsACString & nickname, nsACString& _retval);
> -
> -  nsresult RememberDecision(const nsACString & aHostName, 
> +  static void GetEntryKey(const nsACString& aHostName,
> +                          const NeckoOriginAttributes& aOriginAttributes,
> +                          const nsACString& aFingerprint,
> +                          /* out */ nsACString& aEntryKey);

One last thing, sorry - turns out we actually use "/*out*/" without the spaces in the comment (although do have a space after the comment, like you have now).

::: security/manager/ssl/nsClientAuthRemember.cpp:11
(Diff revision 4)
>  
>  #include "nsClientAuthRemember.h"
>  
>  #include "nsIX509Cert.h"
>  #include "mozilla/RefPtr.h"
> +#include "mozilla/BasePrincipal.h"

nit: actually, I think this would sort before the RefPtr include above it
Comment on attachment 8815992 [details]
Bug 1282279 - Fix style in nsClientAuthRemember.h/cpp

https://reviewboard.mozilla.org/r/96752/#review97256

Looks good.
Attachment #8815992 - Flags: review?(dkeeler) → review+
Comment on attachment 8814808 [details]
Bug 1282279 - Tests that make sure user certificates are Origin Attribute aware.

https://reviewboard.mozilla.org/r/95910/#review98538
Attachment #8814808 - Flags: review?(amarchesini) → review+
Thanks, Baku.
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/533ad81299f7
Make user certificates Origin Attribute aware. r=keeler
https://hg.mozilla.org/integration/autoland/rev/73a3bdf14ac1
Tests that make sure user certificates are Origin Attribute aware. r=baku
https://hg.mozilla.org/integration/autoland/rev/ba0a546dc404
Fix style in nsClientAuthRemember.h/cpp r=keeler
https://hg.mozilla.org/mozilla-central/rev/533ad81299f7
https://hg.mozilla.org/mozilla-central/rev/73a3bdf14ac1
https://hg.mozilla.org/mozilla-central/rev/ba0a546dc404
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi Georg and Arthur,

Does Tor Browser need this patch in the next ESR?
Flags: needinfo?(gk)
Flags: needinfo?(arthuredelstein)
(In reply to Jonathan Hao [:jhao] from comment #46)
> Hi Georg and Arthur,
> 
> Does Tor Browser need this patch in the next ESR?

Hi Jonathan,

If possible, yes! Thank you.
Flags: needinfo?(arthuredelstein)
Comment on attachment 8815183 [details]
Bug 1282279 - Make user certificates Origin Attribute aware.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Tor Browser wouldn't get this in the next ESR.
[Is this code covered by automated tests?]: Yes, by browser/components/originattributes/test/browser/browser_clientAuth.js.
[Has the fix been verified in Nightly?]: No busted tests so far.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: The other two patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: First party isolation and containers are all disabled by default.
[String changes made/needed]: None
Flags: needinfo?(gk)
Attachment #8815183 - Flags: approval-mozilla-aurora?
Comment on attachment 8815183 [details]
Bug 1282279 - Make user certificates Origin Attribute aware.

OA awareness for user certs, take in aurora52
Attachment #8815183 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8814808 [details]
Bug 1282279 - Tests that make sure user certificates are Origin Attribute aware.

OA awareness for user certs, take in aurora52
Attachment #8814808 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.