240628 - Problem with showing origin of privilege request

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Caillon, this may be fallout from your nsPrincipal landing.

STEPS TO REPRODUCE:

1)  Load https://secure.arcamax.com/moztest/amirootca.cacert and accept the
    certificate.  Allow it to "identify software makers".
2)  Load jar:https://secure.arcamax.com/moztest/moztest2.jar!/test.xul

EXPECTED RESULTS: Dialog requesting UniversalXPConnect shows useful site name

ACTUAL RESULTS: Said dialog shows "" as the site name.

I've traced the code that does this.  The relevant code is in 
nsScriptSecurityManager::CheckConfirmDialog where we check whether the principal
has a cert and then call GetCommonName if it does, and GetOrigin otherwise.

Now the problem is that GetCommonName seems to be returning the empty string,
suggesting that nsNSSComponent::VerifySignature was never called or didn't do
the right thing...

Marking critical, since not giving the user an idea of who's requesting
priveleges is totally uncool.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should figure out the cause of this for 1.7, in my opinion.  The current
situation is unacceptable from a security standpoint.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

OK, NSS is doing the right thing.  But we're cloning a cert, and we're not
doing a very good job of it.  This fixes the bug (as in, I get "arcamax.com"
instead of "").

Caillon, it really would be nice if the GetCertificatePrincipal() required a
common name... people would be a lot less likely to misuse it then, no?

Comment 3

[Darin Fisher]

Comment on attachment 146229 [details] [diff] [review]
Fix

patch makes sense to me, but i don't know the caps code that well.

sr=darin assuming caillon is happy.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Hold your horses!  

We don't change mozilla's cert handling every time we come across a cert 
with which it doens't work.  9 times out of 10, the problem is caused by a
cert that doesn't conform to the requirements for a cert for the intended
usage.  In those cases the correct thing is for mozilla to continue to 
expect certs to conform, and for certs that don't conform to be replaced 
with ones that do.  

Comment 5

[Boris Zbarsky [:bzbarsky]]

Nelson, have you read the patch?  The problem is not with cert-handling. The
problem is that the nsIPrincipal object (which is what the browser uses to keep
track of things like permissions) associated with the jar: uri is gotten by
taking the nsIPrincipal associated with the data channel that loaded the jar
(which is the nsIPrincipal that NSS created), cloning it, and replacing the
https: URI in the object with the jar: URI.

The cloning process was implemented incorrectly, though (or made incorrect by
the nsIPrincipal changes that happened a while ago).  It didn't copy over all
the member variables that the principal NSS created had.  In particular, it did
not copy over the member variable that is used as the "for human consumption"
identifier of the issuer of the certificate associated with the principal.

There is absolutely no cert in the world that our current code could possibly
work with, as long as you stick to signed jars served over HTTP.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

OK, release the horses!  

Yes, I had read the patch, and was confused by the term "CommonName" in it,
since your previous post had mentioned organization name.

PSM's certificatePrincipal object uses the term "CommonName" to mean the 
object's name string.  The value for it is taken from the underlying cert's
Subject name's Organization attribute.  

But the term "CommonName" is a term of art, referring to an attribute of a 
cert's DistinguishedNames (a.k.a. directoryName), and is defined in an ISO
standard.  

Now that I know that a principal object's "CommonName" is really the cert's
"Organization" name, I'm ok with this patch.  I'd really like it if the 
principal object's name property could be renamed to something less confusing
like, say, "PrincipalName".  

Thanks for reviewing this with me, Borris.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I've filed bug 240661 on the naming issue.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146229 [details] [diff] [review]
Fix

Could this please be approved for 1.7?	See comment 0 and comment 1 for the
reasons.

This change is very safe.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Patch checked into trunk.  Now what, if anything, do we want to do to prevent
this happening again?  Could we make the pretty name an arg to
GetCertificatePrincipal()?

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 146229 [details] [diff] [review]
Fix

Shouldn't the real fix be in GetCertificatePrincipal?

GetCertificatePrincipal follows two paths, did you test with and without a URI
passed in?

Comment 11

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #10)
> Shouldn't the real fix be in GetCertificatePrincipal?

That's what I keep asking; see comment 2 and comment 9.  Waiting on the owner of
this code to respond, so far.  For 1.7, though, I think that would be too
dangerous a change at this point.  A localized change to fix this specific
problem is better for the branch, in my opinion.

> GetCertificatePrincipal follows two paths, did you test with and without a URI
> passed in?

The code I changed will _always_ pass in a URI.  But yes, the total pageload on
this page tests both codepaths (first PSM gets a principal without a URI when
verifying the jar signature, then the jar channel gets a principal with a URI
when asked for its owner).

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 146229 [details] [diff] [review]
Fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in to 1.7.  I think I'll resolve the GetCertificatePrincipal issue in
bug 240661.  It's a pretty scary change that's totally not 1.7 material.