1124076 - Enabling e10s in Nightly breaks PSM's handling of application/x-x509-user-cert MIME type (<keygen> replies)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kaspar]

Attachment: Sample user cert, with MIME type set to application/x-x509-user-cert

With e10s enabled in Nightly, an end-entity certificate served with an application/x-x509-user-cert MIME type is no longer handled properly by PSM, i.e. as specified under

https://wiki.mozilla.org/CA:Certificate_Download_Specification#Importing_Certificates_into_Firefox

Instead of importing the cert into the DB, it is just treated as a standard download when e10s is enabled. This breaks browser-based enrollment of user certificates in Firefox (which has been supported since the implementaion of bug 77983 back in 2001).

It looks like the NS_CONTENT_LISTENER_CATEGORYMANAGER_ENTRY definitions in

http://hg.mozilla.org/mozilla-central/file/tip/security/manager/ssl/src/nsNSSModule.cpp

are no longer effective when e10s is enabled.

To reproduce, simply try to "open" the attachment in Firefox in an e10s enabled window - and compare this with the behavior in a non-e10s window: in the latter case, Fx will display the "This personal certificate can't be installed because you do not own the corresponding private key which was created when the certificate was requested." prompt (which is expected, as the private key is not available for obvious reasons).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=615decc7cfa2

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Clean up some trivial style nits

Jason, this ended up being pretty much entirely about stream listeners and IPC so you came up as a recommended reviewer.

I noticed a couple of very small things that could be updated.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Replace manual char* manipulation with a string.

More cleanup here: no need to manually manage our own memory when we have strings to do it for us.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Implement IPC forwarding

This is the meat of the patch. If the channel implements nsIDivertable then we use that to divert the load to the parent. Otherwise, we pipe the various nsIStreamListener notifications from the child up to the parent.

One bug I had to fix was to pull the actual cert importing out of OnStopRequest. Because it can pop up modal dialogs, it spins the event loop, which allows necko to collect all of the objects involved.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8593052 [details] [diff] [review]
Clean up some trivial style nits

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

This one is style nits, so it's fine for me to review. And it's right that you ask me or Jason when it's to do with ChannelDiverter. Just run it by someone from PSM (keeler?) when you roll the patches up just to be sure.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8593053 [details] [diff] [review]
Replace manual char* manipulation with a string.

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

::: security/manager/ssl/src/PSMContentListener.cpp
@@ -125,3 @@
>    }
> -  
> -  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CertDownloader::OnDataAvailable\n"));

Please add this log back in.

@@ +137,5 @@
>    }
>  
>    switch (mType) {
>    case PSMContentDownloader::X509_CA_CERT:
> +    return certdb->ImportCertificates(reinterpret_cast<uint8_t*>(mByteData.BeginWriting()),

I thought reinterpret_cast was not allowed?

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8593061 [details] [diff] [review]
Implement IPC forwarding

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

LGTM r=me - comments are all nits :)

Please ask keeler for a quick look.

::: dom/ipc/ContentChild.cpp
@@ +1614,5 @@
>      return true;
>  }
>  
> +PPSMContentDownloaderChild*
> +ContentChild::AllocPPSMContentDownloaderChild(const uint32_t& aCertType)

nit: Please add a comment:
// aCertType is unused by the child actor.

::: security/manager/ssl/src/PSMContentListener.cpp
@@ +170,5 @@
> +
> +void
> +PSMContentStreamListener::ImportCertificate()
> +{
> +  nsNSSShutDownPreventionLock locker;

Add a main thread only assertion?

@@ +238,5 @@
> +bool
> +PSMContentDownloaderParent::RecvOnStopRequest(const nsresult &code)
> +{
> +  if (NS_SUCCEEDED(code)) {
> +    // Done, do this now, off of the IPC message.

Can you add more detail as to why it's ok to call this directly here (non-divertable channels), but not ok in OnStopRequest (divertable channels)?

@@ +276,5 @@
> +}
> +
> +NS_IMPL_ISUPPORTS(PSMContentDownloaderChild, nsIStreamListener)
> +
> +PSMContentDownloaderChild::PSMContentDownloaderChild()

nit: Can you add a multi-line break comment here:

/* ----------
PSMContentDownloaderChild
 ---------- */

Something like that. Same for parent.

@@ +300,5 @@
> +    return SendDivertToParentUsing(diverter) ? NS_OK : NS_ERROR_FAILURE;
> +  }
> +
> +  int64_t contentLength = ComputeContentLength(request);
> +  if (contentLength < 0) {

NS_WARN_IF in single process OnStartRequest but not here.

::: security/manager/ssl/src/PSMContentListener.h
@@ +27,3 @@
>  namespace mozilla { namespace psm {
>  
> +class PSMContentStreamListener : public nsIStreamListener

nit: /* PSMContentStreamListener for single process mode. */

@@ +43,5 @@
> +  nsCString mByteData;
> +  uint32_t mType;
> +};
> +
> +class PSMContentDownloaderParent : public PPSMContentDownloaderParent

nit: /* PSMContentDownloaderParent inherits from PSMContentStreamListener for multi-process mode. */

@@ +70,5 @@
> +  bool mIPCOpen;
> +};
> +
> +
> +class PSMContentDownloaderChild : public nsIStreamListener

nit: /* PSMContentDownloaderChild: for multiple process mode. */

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Is there a test for this somewhere?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #6)
> I thought reinterpret_cast was not allowed?

It is necessary here to convert between 'char *' and 'uint8_t *' (of note: they're both pointers). I could do two static casts, going first to 'void *', but that's basically what reinterpret_cast does anyway.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8593053 [details] [diff] [review]
Replace manual char* manipulation with a string.

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

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #9)
> (In reply to Steve Workman [:sworkman] (please use needinfo) from comment #6)
> > I thought reinterpret_cast was not allowed?
> 
> It is necessary here to convert between 'char *' and 'uint8_t *' (of note:
> they're both pointers). I could do two static casts, going first to 'void
> *', but that's basically what reinterpret_cast does anyway.

OK, sounds good. r=me

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #8)
> Is there a test for this somewhere?

I don't know how to test this. David, do you know how hard it would be to write a test for this (I'm happy to do the writing, I just don't know how to make sure that the certificate is correctly imported)?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rolled-up patch

Hey David, Steve has already reviewed the meat of this patch (which is mostly Necko stuff, forwarding to the parent process properly so that we can use NSS) but it'd be good to get your input as well.

Comment 13

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

Comment on attachment 8594868 [details] [diff] [review]
Rolled-up patch

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

Honestly, I've always thought this implementation was unnecessary. We should just special-case those mime types in the download manager instead of doing this. That said, this will make it a lot better than it was. I mostly have style nits and an observation that this doesn't actually depend on NSS directly, so we shouldn't need the NSS startup/shutdown mechanisms. r=me with those addressed.

::: security/manager/ssl/src/PSMContentListener.cpp
@@ +22,5 @@
>  
>  #include "nsCRT.h"
>  #include "nsNetUtil.h"
>  #include "nsNSSHelper.h"
>  #include "nsNSSShutDown.h"

Looks like this implementation doesn't directly use NSS, so we don't need any of the NSS shutdown mechanisms.

@@ -41,5 @@
> -  enum {UNKNOWN_TYPE = 0};
> -  enum {X509_CA_CERT  = 1};
> -  enum {X509_USER_CERT  = 2};
> -  enum {X509_EMAIL_CERT  = 3};
> -  enum {X509_SERVER_CERT  = 4};

What. even. was. going. on. here.

@@ +38,5 @@
>  const int32_t kDefaultCertAllocLength = 2048;
>  
> +enum {
> +  UNKNOWN_TYPE = 0,
> +  X509_CA_CERT  = 1,

Probably don't need two spaces before '=' on these.

@@ +80,5 @@
> +  }
> +
> +  int64_t contentLength;
> +  nsresult rv = channel->GetContentLength(&contentLength);
> +  if (NS_FAILED(rv) || contentLength <= 0)

nit: braces around conditional bodies

@@ +142,5 @@
> +
> +NS_IMETHODIMP
> +PSMContentStreamListener::OnDataAvailable(nsIRequest* request,
> +                                          nsISupports* context,
> +                                          nsIInputStream *aIStream,

nit: * to the left

@@ +176,5 @@
> +
> +void
> +PSMContentStreamListener::ImportCertificate()
> +{
> +  nsNSSShutDownPreventionLock locker;

This isn't necessary anymore (if it ever was).

@@ +186,5 @@
> +  switch (mType) {
> +  case X509_CA_CERT:
> +  case X509_USER_CERT:
> +  case X509_EMAIL_CERT:
> +    certdb = do_GetService(NS_X509CERTDB_CONTRACTID);

This call to do_GetService needs to be checked that it succeeded (i.e. that certdb != nullptr).

@@ +236,5 @@
> +  return true;
> +}
> +
> +bool
> +PSMContentDownloaderParent::RecvOnDataAvailable(const nsCString &data,

nit: & to the left

@@ +255,5 @@
> +    // ImportCertificate spins the event loop.
> +    ImportCertificate();
> +  }
> +
> +  if (mIPCOpen)

nit: braces around conditionals

@@ +261,5 @@
> +  return true;
> +}
> +
> +NS_IMETHODIMP
> +PSMContentDownloaderParent::OnStopRequest(nsIRequest *request, nsISupports *context, nsresult code)

nit: * to the left

@@ +308,5 @@
> +  nsCOMPtr<nsIDivertableChannel> divertable = do_QueryInterface(request);
> +  if (divertable) {
> +    mozilla::net::ChannelDiverterChild* diverter = nullptr;
> +    nsresult rv = divertable->DivertToParent(&diverter);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

I'm not a fan of NS_WARN_IF and related debugging output. It just gets lost in the mess.

@@ +328,5 @@
> +
> +NS_IMETHODIMP
> +PSMContentDownloaderChild::OnDataAvailable(nsIRequest* request,
> +                                           nsISupports* context,
> +                                           nsIInputStream *aIStream,

nit: * to the left

::: security/manager/ssl/src/PSMContentListener.h
@@ +52,5 @@
> +{
> +public:
> +  explicit PSMContentDownloaderParent(uint32_t type);
> +
> +  virtual bool RecvOnStartRequest(const uint32_t &contentLength) override;

nit: & to the left on all of these

@@ +61,5 @@
> +
> +  // We inherit most of nsIStreamListener from PSMContentStreamListener, but
> +  // we have to override OnStopRequest to know when we're done with our IPC
> +  // ref.
> +  NS_IMETHOD OnStopRequest(nsIRequest *request, nsISupports *aContext, nsresult code) override;

nit: * to the left

@@ +71,5 @@
> +
> +  virtual void ActorDestroy(ActorDestroyReason why) override;
> +  bool mIPCOpen;
> +};
> +

nit: probably only need one blank line between class declarations

@@ +87,5 @@
> +
> +private:
> +  ~PSMContentDownloaderChild();
> +};
> +

nit: extra blank line

::: security/manager/ssl/src/nsNSSModule.cpp
@@ +187,5 @@
>  NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsTLSSocketProvider)
>  NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsSecretDecoderRing)
>  NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsPK11TokenDB)
>  NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsPKCS11ModuleDB)
> +NS_NSS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nssEnsureChromeOrContent, PSMContentListener, init)

It looks like PSMContentListener doesn't actually depend on NSS directly, so I think we can use NS_GENERIC_FACTORY_CONSTRUCTOR_INIT instead.

Comment 14

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

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #11)
> (In reply to Steve Workman [:sworkman] (please use needinfo) from comment #8)
> > Is there a test for this somewhere?
> 
> I don't know how to test this. David, do you know how hard it would be to
> write a test for this (I'm happy to do the writing, I just don't know how to
> make sure that the certificate is correctly imported)?

You might be able to test this with a browser-chrome test. I think you can start a server that serves the right mime type, and then if you make a request to it, a XUL window should pop up asking if you want to import the certificate (or, e.g. an error message in the case of the certificate in this bug).

Comment 15

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

(In reply to David Keeler [:keeler] (use needinfo?) from comment #14)

... and to actually make sure the certificate gets imported in the success case, you can use nsIX509CertDB.getCerts(), loop through that list, and make sure you find one with the right fingerprint, for example.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a499ee730f1 contains the final patch here, in case anyone was worried that addressing the review comments caused a regression.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3cf84c11a9

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9173681&repo=mozilla-inbound

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b37a75cb4bf

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

I landed https://hg.mozilla.org/integration/mozilla-inbound/rev/64148714c156 with the problem fixed.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/64148714c156

Comment 23

[Nathan Toone [:toonetown]]

The patch that landed here on central breaks when building with PR_LOGGING not defined because of the if statement in security/manager/ssl/src/PSMContentListener.cpp on line 402.

 2:07.17 In file included from Unified_cpp_manager_ssl_src0.cpp:20:0:
 2:07.17 ../../../../../security/manager/ssl/src/PSMContentListener.cpp: In member function 'virtual nsresult mozilla::psm::PSMContentListener::DoContent(const nsACString_internal&, bool, nsIRequest*, nsIStreamListener**, bool*)':
 2:07.17 ../../../../../security/manager/ssl/src/PSMContentListener.cpp:402:7: error: 'gPIPNSSLog' was not declared in this scope
 2:07.17    if (gPIPNSSLog) {
 2:07.17        ^

Comment 24

[Nathan Toone [:toonetown]]

Attachment: Simple Patch

The if check should probably be wrapped with "#ifdef PP_LOGGING" - like it is in nsNSSComponent.cpp

Comment 25

[Nathan Toone [:toonetown]]

Sorry - "PR_LOGGING" - not "PP_LOGGING". :)

Comment 26

[Nathan Toone [:toonetown]]

Although - the PR_LOG macro already is a no-op when PR_LOGGING is defined, so perhaps a better solution would be to just remove the "if" check completely...

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Nathan Toone from comment #26)
> Although - the PR_LOG macro already is a no-op when PR_LOGGING is defined,
> so perhaps a better solution would be to just remove the "if" check
> completely...

The problem there is in e10s, in the child process, we don't initialize the log module even though PR_LOGGING is defined so gPIPNSSLog is null, which crashes.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8596778 [details] [diff] [review]
Simple Patch

This is definitely the way to go. Thanks for writing the patch!

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6e58f4965c

Comment 30

[Nathan Toone [:toonetown]]

Does this bug need to be reopened/tagged or something so that it gets updated before mozilla40 gets moved to aurora?

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Nope, both of the patches here will ride the trains onto Aurora and beyond.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c6e58f4965c