1047177 - mozilla::pkix does not accept x509v4 (v4) certificates

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Mike Kaply [:mkaply]]

Attachment: ikey_tomcat_serverca.pem

I have a customer that has reported that they get an invalid CA error with the new pkix library.

If they set the preference security.use_mozillapkix_verification back to true, it works.

I'm attaching Certificates as well as screenshots from the working case, as well as server configurations.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: ikey_tomcat_cert.pem

Comment 2

[Mike Kaply [:mkaply]]

Attachment: server.xml

Comment 3

[Mike Kaply [:mkaply]]

Attachment: 1.JPG

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Connection Info

Comment 5

[Mike Kaply [:mkaply]]

Attachment: General Certificate Info

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Details view

Comment 7

[Mike Kaply [:mkaply]]

These screenshots are from Firefox 24.

Based on what I can infer, they have their own CA that they are using for signing and that's what is not working on Firefox 31.

Comment 8

[Daniel Veditz [:dveditz]]

Firefox 24.7 does not have the new pkix library (only Firefox 31 does), but it did pick up a new version of NSS. So what did we change in NSS that affects Classic verification and new pkix but not the old pkix verification?

Failure to find a valid path probably isn't a security bug, it fails safe by preventing the connection.

Comment 9

[Daniel Veditz [:dveditz]]

Setting the pref security.use_mozillapkix_verification to true will turn ON the new code (and does nothing in ESR-24) which is the opposite of the summary of this bug. Are you sure about that?

Comment 11

[Camilo Viecco (:cviecco)]

Ouch, this certificate is claiming version 4 which is non-existent.  Mozilla::pkix is much stricter about certificate versions and thus this is part of the issue. A similar bug (but with allowing invalid v1 certs) is bug 1045973.

Mike what utility generated this cert?

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #11)
> Ouch, this certificate is claiming version 4 which is non-existent. 
> Mozilla::pkix is much stricter about certificate versions and thus this is
> part of the issue. A similar bug (but with allowing invalid v1 certs) is bug
> 1045973.
> 
> Mike what utility generated this cert?

This probably happens because the encoding of v3 is 0x02 but some people have written code thinking that v3 should be encoded as 0x03. Gecko should probably allow cert error overrides for this problem.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Actually, we should just change mozilla::pkix so that it accepts v4 certificates and treats them like v3 certificates.

Comment 14

[Mike Kaply [:mkaply]]

Since this contains the clients certificates, can we leave it hidden or open a new bug (or does it matter?)

Comment 15

[Mike Kaply [:mkaply]]

Remarking as secure because of attachments.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike Kaply (:mkaply) from comment #15)
> Remarking as secure because of attachments.

Tip: eventually all security-group bugs become public. If you want your attachments to stay private permanently (as is the case here), you can mark them as "private" when you attach them, or afterwards. I have marked all your attachments and Dan's comment that lists the contents of a certificate as private, and I'm reopening the bug.

cviecco, this appears to be Zenworks. There is at least one other bug, if not two, about incompatibilities between Firefox 31 and Zenworks.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

[Tracking Requested - why for this release]: Compatibility regression with many Novell Zenworks servers.

Comment 18

[Camilo Viecco (:cviecco)]

Brian. For v4 certs I think the best option would be to make this error: SEC_ERROR_UNTRUSTED_CERT, which is overridable allowing users to accept this certificate. This would not change the user behavior when facing a self-signed cert but will force uses CA's to actually use the correct values.

Is that acceptable?

Comment 19

[Mike Kaply [:mkaply]]

> If you want your attachments to stay private permanently (as is the case here), you can mark them as "private" when you attach them, or afterwards. I have marked all your attachments and Dan's comment that lists the contents of a certificate as private, and I'm reopening the bug.

Thanks Brian. I'll remember that for future reference.

Comment 20

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]:

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #18)
> Brian. For v4 certs I think the best option would be to make this error:
> SEC_ERROR_UNTRUSTED_CERT, which is overridable allowing users to accept this
> certificate. This would not change the user behavior when facing a
> self-signed cert but will force uses CA's to actually use the correct values.
> 
> Is that acceptable?

If the user added the custom ZenWorks-generated root CA as a trust anchor, then they shouldn't get any error. In particular, we shouldn't force these users into the cert error override UI. So, I think mozilla::pkix just needs to accept these "v4" certificates.

I agree it would be good to put some pressure on Novell to correct their software, but that ship has sailed. Even if Novell fixed that bug soon (which they won't), and even if all the deployments of it updated soon (which they won't), the sites using these certificates would still have to regenerate their roots AND probably every other certificate.

Being pedantic here doesn't buy us any security. Any, one could even argue that it is more correct to accept v4+ certificates assuming v3 rules; I'm not going to dig through the spec to find out, since it seems like we don't have any reasonable choice regardless.

Comment 22

[Camilo Viecco (:cviecco)]

Agreed for this one  process v4 certs with v3 rules. I will take this.

Comment 23

[Camilo Viecco (:cviecco)]

Brian. Do you have(or know someone who can provide) an example of a ZenWorks root CA?

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #23)
> Brian. Do you have(or know someone who can provide) an example of a ZenWorks
> root CA?

No. I think the first attachment in this bug may be one though.

Comment 25

[Camilo Viecco (:cviecco)]

Brian, thanks no, the first attachment is an end-entity cert.

Comment 26

[Camilo Viecco (:cviecco)]

The certs attached to this bug do not match the details view certs. Can you post (as a private message) the actual CA and end-entity cert generated by ZenWorks?

Comment 27

[Mike Kaply [:mkaply]]

There's more information about this problem then I realized.

The client has three servers with problem.

The first two, deploy1 and deploy2 are zenworks servers. They have the stated problem and I'll be attaching their certs.

The third server, auth1, doesn't work even with security.use_mozillapkix_verification set to true on Firefox 31 (It worked on Firefox 24).

The attached certificates were to demonstrate that problem (not the zenworks problem). That's why the screenshots were done on Firefox 25.

Is there anything that changed about cert verification that would break both the new AND the old on Firefox 31?

Comment 28

[Mike Kaply [:mkaply]]

When I go to attach the certs, i don't have the ability to make them private. It's nowhere in the attach process.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

We probably won't release a new 31 for this but we might take a patch for ESR 31.

Comment 30

[Camilo Viecco (:cviecco)]

Attachment: allow-v4-overridable

option 1. Will give us the overridability, but for ALL cases.

Comment 31

[Camilo Viecco (:cviecco)]

Attachment: make-v4-ok-for-self-signed-ee

option 2: v4 allowed only for self-signed ee with no errors (could modify to allow non-self-signed to be overridable).

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8467399 [details] [diff] [review]
allow-v4-overridable

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

We shouldn't consider a v4 certificate to be invalid just because it is a v4 certificate instead of a v3 certificate. In particular, if a v4 certificate is a trust anchor then it should be accepted without any cert error override, just like it was in Firefox 30.

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike Kaply (:mkaply) from comment #28)
> When I go to attach the certs, i don't have the ability to make them
> private. It's nowhere in the attach process.

On the attachment page, there should be a checkbox "Make attachment and comment private (visible only to members of the core-security group)" at the bottom of the page. If there isn't, then please just email cviecco the certificates and he can attach them to the bug.

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8467400 [details] [diff] [review]
make-v4-ok-for-self-signed-ee

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

I think it is fine to limit the exception for supporting v4 certificates to just self-signed certificates if we think that those are the only ones claiming to be v4. However, I think it is also fine to just treat 0x03 as a synonym for 0x04, if that doesn't resolve the compatibility issue completely.

::: security/pkix/lib/pkixcert.cpp
@@ +112,5 @@
>    rv = der::ExpectTagAndGetTLV(tbsCertificate, der::SEQUENCE, subject);
>    if (rv != Success) {
>      return rv;
>    }
> +

I recommend putting the check above down here so that all the special code for v4 certificates is together.

@@ +114,5 @@
>      return rv;
>    }
> +
> +  // Version 4 certs should never be used for non-self-signed certs
> +  if (version == der::Version::v4 && !InputsAreEqual(issuer, subject)) {

This is not the correct check for a self-signed certificate. A certificate is self-signed if the subject public key is equal to the issuer public key. So, in order to determine if a certificate is self-signed, you need to check if it is self-issued (which you are doing now) AND check the signature using the subject public key. Since this problem is rare and only affects (as far as this patch is assuming) certificates that do not chain up to any trust anchor, it is OK performance-wise to do the self-signed check.

Note in particular that a self-issued (same issuer and subject name) certificate may be either an intermediate or a root.

@@ +115,5 @@
>    }
> +
> +  // Version 4 certs should never be used for non-self-signed certs
> +  if (version == der::Version::v4 && !InputsAreEqual(issuer, subject)) {
> +    return Result::ERROR_BAD_DER; // Non-overridable

It would be nicer to have a specific error code for this problem.

@@ +161,5 @@
> +  // for compality reasons only when used as end-entity certs.
> +  if (version == der::Version::v3 ||
> +      (version == der::Version::v4 &&
> +       endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
> +       InputsAreEqual(issuer, subject))) {

Aren't the InputsAreEqual and endEntityOrCA == EndEntityOrCA::MustBeEndEntity checks redundant with the previous checks? AFAICT, you can then change this check to be just (version >= der::Version::v3).

Comment 35

[Camilo Viecco (:cviecco)]

Attachment: make-v4-ok-for-self-signed-ee

Comment 36

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

Comment on attachment 8468060 [details] [diff] [review]
make-v4-ok-for-self-signed-ee

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

My feedback is more or less what Brian has already said, plus that it would be easier to understand this patch if it were in two parts: handling v4 and then handling v1/v2.

::: security/pkix/lib/pkixcert.cpp
@@ +116,5 @@
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +      return Result::ERROR_CA_CERT_INVALID;
> +    } else {
> +      // allow v1 certs to continue
> +      if (version != der::Version::v1) {

I'm finding this logic a little hard to follow - maybe re-phrase these ifs as filtering out exactly what we don't allow? Or perhaps the issue is that you're handling both v1/v2 and v4 in the same patch. Maybe it would be easier to follow if these were two separate patches.

@@ +117,5 @@
> +      return Result::ERROR_CA_CERT_INVALID;
> +    } else {
> +      // allow v1 certs to continue
> +      if (version != der::Version::v1) {
> +        return Result::ERROR_BAD_DER; // Non-overridable

we can add new errors to pkixnss.cpp

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Camilo, do you think you could address that before 33 moves to beta?

Comment 38

[Lawrence Mandel [:lmandel] (use needinfo)]

Further to comment 37, if there is a good case for taking this on beta, the fix needs to land in beta9, which goes to build on Thu. I also saw mention of taking this on ESR31, which will need a fix this week as well.

Comment 39

[Camilo Viecco (:cviecco)]

Sylvestre: Yes landing this now is my top priority.

Comment 40

[Camilo Viecco (:cviecco)]

Attachment: make-v4-ok-for-self-signed-ee (v2)

Comment 41

[Camilo Viecco (:cviecco)]

Attachment: use-new-error-message

Comment 42

[Camilo Viecco (:cviecco)]

Attachment: tests-v4-only

Comment 43

[Camilo Viecco (:cviecco)]

Comment on attachment 8476203 [details] [diff] [review]
use-new-error-message

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

The name of the error is too long, suggestions appreciated.

Comment 44

[Camilo Viecco (:cviecco)]

Comment on attachment 8476205 [details] [diff] [review]
tests-v4-only

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

Missing the self-signed tests, this changes only address the new error code

Comment 45

[Camilo Viecco (:cviecco)]

Please review this ASAP so that we can get it into beta. Thanks

Comment 46

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

Comment on attachment 8476203 [details] [diff] [review]
use-new-error-message

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

This looks good, but see comments. I think this patch should be folded with the other non-test one.

::: security/pkix/include/pkix/Result.h
@@ +78,5 @@
>    ERROR_UNSUPPORTED_KEYALG = 37,
>    ERROR_EXPIRED_ISSUER_CERTIFICATE = 38,
>    ERROR_CA_CERT_USED_AS_END_ENTITY = 39,
>    ERROR_INADEQUATE_KEY_SIZE = 40,
> +  ERROR_INVALID_CERTIFICATE_VERSION_IN_HIERARCHY = 41,

How about just "ERROR_INVALID_CERTIFICATE_VERSION"? We don't have to be specific or mention hierarchies or anything.

::: security/pkix/lib/pkixnss.cpp
@@ +307,5 @@
>      { "MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE",
>        "The server presented a certificate with a key size that is too small "
> +      "to establish a secure connection." },
> +    { "MOZILLA_PKIX_ERROR_INVALID_CERTIFICATE_VERSION_IN_A_HIERARCHY",
> +      "The server presentat a certificate with an invalid ceriticate version "

How about something like this: "The server presented a certificate with an invalid version number. The version should be v3, which has a representational value of 2."

Comment 47

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

Comment on attachment 8476201 [details] [diff] [review]
make-v4-ok-for-self-signed-ee (v2)

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

Looks good, but I have a few concerns. r- for now.

::: security/pkix/lib/pkixcert.cpp
@@ +112,5 @@
> +
> +  // Version 4 certs should not be accepted but self-signed
> +  // However at this point of the code we can only check for being self-issued
> +  // Which is a good enough proxy for this condition (if the signature fails
> +  // it should be catched by another error).

nit: this comment could use some cleanup

@@ +117,5 @@
> +  if (version == der::Version::v4 && !InputsAreEqual(issuer, subject)) {
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +      return Result::ERROR_CA_CERT_INVALID;
> +    } else {
> +      return Result::ERROR_BAD_DER; // Non-overridable

Neither of these are overridable. I'm not sure I see the value in using different error values for CAs vs. end-entities. Just use the same error value added in the other patch.

@@ +158,5 @@
>      }
>    }
>  
>    // Extensions were added in v3, so only accept extensions in v3 certificates.
> +  // However there are a bunch of self-signed certs that claim to be version 4

I don't know about "a bunch". Maybe "some".

@@ +172,5 @@
>      if (rv != Success) {
>        return rv;
>      }
>    }
> +  // We need to return an overridable error when findinding v4 certs

I thought the idea was to only allow v4 certs to act like v3 certs if they were self-signed. Otherwise, they're invalid, right? Otherwise, we may as well just alias v4 to v3. Another option would be to make the newly added error overridable and just return that error when we encounter any v4 certs. Either way, we should document exactly what kinds of v4 certs are allowed/allowed with an override/not allowed at all in one place.

Comment 48

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

Comment on attachment 8476205 [details] [diff] [review]
tests-v4-only

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

Looks good so far (be sure to use the updated error name).

Comment 49

[Camilo Viecco (:cviecco)]

Attachment: make-v4-ok-for-self-signed-ee (v3)

Comment 50

[Camilo Viecco (:cviecco)]

Attachment: tests-v4-only (v2)

keeping r+ from keeler (added an 4 extra tests and certs)

Comment 51

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=55b9b62ee66e

Comment 52

[Camilo Viecco (:cviecco)]

Attachment: tests-v4-only (v2.1)

Keeping r+, Fixed extra spaces.

Comment 53

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

Comment on attachment 8476322 [details] [diff] [review]
tests-v4-only (v2.1)

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

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +65,4 @@
>    CertUtils.generate_ca_cert(db_dir, dest_dir, noise_file,  name, version, do_bc)
>    generate_intermediates_and_ee_set(db_dir, dest_dir, noise_file, name)
>  
> +def generate_self_signed_ee(db_dir, dest_dir, noise_file, nam, version):

nit: "name", not "nam"

@@ +71,3 @@
>  def generate_certs():
>    [noise_file, pwd_file] = CertUtils.init_nss_db(db)
> +  generate_ca(db, srcdir, noise_file, "v1_ca", 1, False)

nit: if you're fixing this line, might as well fix the others, below

@@ +77,5 @@
>    generate_ca(db, srcdir, noise_file, "v3_ca", 3, True )
>    generate_ca(db, srcdir, noise_file, "v3_ca_missing_bc", 3, False)
>  
> +  CertUtils.generate_self_signed_cert(db, srcdir, noise_file, "v3_self_signed",
> +                                      3, False, False);

nit: no semicolons in python

Comment 54

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

Comment on attachment 8476313 [details] [diff] [review]
make-v4-ok-for-self-signed-ee (v3)

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

I must have misunderstood what you were saying on irc. I think we should just make any v4 certificate return INVALID_CERTIFICATE_VERSION and make it overridable.

::: security/pkix/lib/pkixcert.cpp
@@ +109,5 @@
>    if (rv != Success) {
>      return rv;
>    }
> +
> +  // Version 4 certificates should only be accepted when self signed. We reject

nit: "self-signed"

@@ +112,5 @@
> +
> +  // Version 4 certificates should only be accepted when self signed. We reject
> +  // here all version 4 certificates that are not self-issued. The real check
> +  // should be for self-signed but at this point of the code we cannot check it
> +  // so we check for self-issued with is a requirement for self-signed (the

nit: s/with/which/

@@ +113,5 @@
> +  // Version 4 certificates should only be accepted when self signed. We reject
> +  // here all version 4 certificates that are not self-issued. The real check
> +  // should be for self-signed but at this point of the code we cannot check it
> +  // so we check for self-issued with is a requirement for self-signed (the
> +  // signature is checked in other part of the code).

nit: s/other/another/

@@ +162,5 @@
> +  // when passing as a CA the returned error must be overridable in order for
> +  // the override UI to work correctly. Because of this we will process v3
> +  // extensions for v4 certificates when being evalutated as end-entities and
> +  // mark them as untrusted when being used as CAs (skipping decoding).
> +  // Notice that at this point v4 certificates are guaranteed to be self-issued.

I see what you were saying. I think I meant that we should just make INVALID_CERTIFICATE_VERSION overridable and don't worry about special-casing all of this.

Comment 55

[Lawrence Mandel [:lmandel] (use needinfo)]

Camilo - Firefox 32 beta9 (final beta) goes to build today. I'm going to need to hear a strong case for taking this at the last minute and see an r+ patch that is relatively safe and has preferably landed on m-c before approving for uplift. Is there a possibility that you're going to have this ready to go today?

Comment 57

[Camilo Viecco (:cviecco)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #54)
> Comment on attachment 8476313 [details] [diff] [review]
> make-v4-ok-for-self-signed-ee (v3)
> 
> Review of attachment 8476313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I must have misunderstood what you were saying on irc. I think we should
> just make any v4 certificate return INVALID_CERTIFICATE_VERSION and make it
> overridable.
> 
> ::: security/pkix/lib/pkixcert.cpp
> @@ +109,5 @@
> >    if (rv != Success) {
> >      return rv;
> >    }
> > +
> > +  // Version 4 certificates should only be accepted when self signed. We reject
> 
> nit: "self-signed"
> 
> @@ +112,5 @@
> > +
> > +  // Version 4 certificates should only be accepted when self signed. We reject
> > +  // here all version 4 certificates that are not self-issued. The real check
> > +  // should be for self-signed but at this point of the code we cannot check it
> > +  // so we check for self-issued with is a requirement for self-signed (the
> 
> nit: s/with/which/
> 
> @@ +113,5 @@
> > +  // Version 4 certificates should only be accepted when self signed. We reject
> > +  // here all version 4 certificates that are not self-issued. The real check
> > +  // should be for self-signed but at this point of the code we cannot check it
> > +  // so we check for self-issued with is a requirement for self-signed (the
> > +  // signature is checked in other part of the code).
> 
> nit: s/other/another/
> 
> @@ +162,5 @@
> > +  // when passing as a CA the returned error must be overridable in order for
> > +  // the override UI to work correctly. Because of this we will process v3
> > +  // extensions for v4 certificates when being evalutated as end-entities and
> > +  // mark them as untrusted when being used as CAs (skipping decoding).
> > +  // Notice that at this point v4 certificates are guaranteed to be self-issued.
> 
> I see what you were saying. I think I meant that we should just make
> INVALID_CERTIFICATE_VERSION overridable and don't worry about special-casing
> all of this.

Keeler, given that:
1. The logic here is the same needed for bug 1045973 (see https://bugzilla.mozilla.org/attachment.cgi?id=8476336)
2. We have no automated tests for testing that the override is working as expected
3. We are in a rush for pushing this as soon as possible to beta.

can you r+ this version?

Comment 58

[Camilo Viecco (:cviecco)]

Keeler ping (see comment 57)

Comment 59

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8476313 [details] [diff] [review]
make-v4-ok-for-self-signed-ee (v3)

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

::: security/pkix/lib/pkixcert.cpp
@@ +112,5 @@
> +
> +  // Version 4 certificates should only be accepted when self signed. We reject
> +  // here all version 4 certificates that are not self-issued. The real check
> +  // should be for self-signed but at this point of the code we cannot check it
> +  // so we check for self-issued with is a requirement for self-signed (the

I object to the sentence "The real check should be...but..we cannot check it." It could be done but we chose not to do it. Big difference. But, see my next comment.

@@ +114,5 @@
> +  // here all version 4 certificates that are not self-issued. The real check
> +  // should be for self-signed but at this point of the code we cannot check it
> +  // so we check for self-issued with is a requirement for self-signed (the
> +  // signature is checked in other part of the code).
> +  if (version == der::Version::v4 && !InputsAreEqual(issuer, subject)) {

Given the other bug reports, including the one I just duped to this, I think it is not worth the pain for us to limit this to just self-issued/self-signed certificates. So, I think we should just treat v4 (0x03) as a synonym for v3 and move on--i.e. no self-signed/issued restriction.

@@ +162,5 @@
> +  // when passing as a CA the returned error must be overridable in order for
> +  // the override UI to work correctly. Because of this we will process v3
> +  // extensions for v4 certificates when being evalutated as end-entities and
> +  // mark them as untrusted when being used as CAs (skipping decoding).
> +  // Notice that at this point v4 certificates are guaranteed to be self-issued.

I disagree with David. Whether we accept or reject these 'v4" certificates isn't really a significant security issue for users, so we shouldn't make them deal with cert error overrides for this problem.

Also, remember that this error is detected *very* early, before other checks are done. So, if INVALID_CERTIFICATE_VERSION is returned here, then all other--more security-sensitive--checks will be skipped. In particular, consider the case of a INVALID_CERTIFICATE_VERSION certificate that is also UNKNOWN_ISSUER or even worse UNTRUSTED_ISSUER; in that case, the trust error must be returned instead of the INVALID_CERTIFICATE_VERSION error. Once you consider that, it becomes clear that we must NEVER return error results for overridable errors from this function. We should document this, because it is quite non-obvious.

Instead, let's just accept v4 certificates as though they were v3 and move on.

::: security/pkix/lib/pkixnss.cpp
@@ +309,5 @@
> +      "to establish a secure connection." },
> +    { "MOZILLA_PKIX_ERROR_INVALID_CERTIFICATE_VERSION",
> +      "The server presented a certificate with an invalid version number. "
> +      "The version should be version 3, which has a representational value "
> +      "of 2." }

I'd like to suggest an alternative here for these error messages. Instead of putting the error text here, put that text in the l10n/i18n string bundles, and update the NSSErrorWhateverWhatever implementation to grab the error text out of the string bundle. Otherwise, you are creating significant localization problems.

Also note that when you fix a bug that adds a new error message, you need to plan on having several weeks lead time in order to get your error message localized. (This is why the uplift questionnaire asks if you added any new strings.)

This is another reason why we should just transparently treat v4 as v3; there's not enough time to localize the strings for Firefox 32 or even 33.

Comment 60

[Camilo Viecco (:cviecco)]

Attachment: make-v4-ok-for-self-signed-ee (v4)

Comment 61

[Camilo Viecco (:cviecco)]

Attachment: make-v4-ok-for-self-signed-ee (v4.1)

Comment 62

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

Camilo, I can't seem to raise you on irc. In light of bug 1051727 and Brian's comments (comment 59), I'm wondering what you think of the security impacts of essentially aliasing v3 and v4? While we wouldn't be as actively encouraging people to fix their implementations, I don't think we're giving up a significant amount of security. In return we gain back a great deal of interoperability.

Comment 63

[Camilo Viecco (:cviecco)]

Attachment: make-v4-equal-v3

Comment 64

[Camilo Viecco (:cviecco)]

Attachment: tests-v4-eq-v3

Comment 65

[Camilo Viecco (:cviecco)]

Comment on attachment 8476901 [details] [diff] [review]
tests-v4-eq-v3

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

This is basically the same r+ed one.

Comment 66

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

Comment on attachment 8476899 [details] [diff] [review]
make-v4-equal-v3

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

Looks good.

Comment 67

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

Comment on attachment 8476901 [details] [diff] [review]
tests-v4-eq-v3

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

r=me with comments addressed.

::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py
@@ +213,5 @@
>      # create nss db
>      os.system("certutil -d sql:" + db_dir + " -N -f " + pwd_file);
>      return [noise_file, pwd_file]
>  
> +def generate_self_signed_cert(db_dir, dest_dir, noise_file, name, version, do_bc, is_ca):

If you change the test to not include the additional tests, these changes to the python files probably aren't necessary.

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +491,5 @@
>    check_cert_err(cert_from_file('v2_bc_ee-v3_int-v3_ca_missing_bc.der'), SEC_ERROR_BAD_DER);
> +  check_cert_err(cert_from_file('v4_bc_ee-v3_int-v3_ca_missing_bc.der'), ee_error);
> +
> +  // self-signed
> +  check_cert_err(cert_from_file('v3_self_signed.der'), SEC_ERROR_UNKNOWN_ISSUER);

Are these tests necessary? I think what would be more useful is a test that a v4 certificate issued by a trusted issuer verifies successfully.

Comment 68

[Camilo Viecco (:cviecco)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #67)
> Comment on attachment 8476901 [details] [diff] [review]
> tests-v4-eq-v3
> 
> Review of attachment 8476901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py
> @@ +213,5 @@
> >      # create nss db
> >      os.system("certutil -d sql:" + db_dir + " -N -f " + pwd_file);
> >      return [noise_file, pwd_file]
> >  
> > +def generate_self_signed_cert(db_dir, dest_dir, noise_file, name, version, do_bc, is_ca):
> 
> If you change the test to not include the additional tests, these changes to
> the python files probably aren't necessary.

This bug showed lack of coverage, I would leave these tests unless you strongly believe that we should not have them.

> 
> ::: security/manager/ssl/tests/unit/test_cert_version.js
> @@ +491,5 @@
> >    check_cert_err(cert_from_file('v2_bc_ee-v3_int-v3_ca_missing_bc.der'), SEC_ERROR_BAD_DER);
> > +  check_cert_err(cert_from_file('v4_bc_ee-v3_int-v3_ca_missing_bc.der'), ee_error);
> > +
> > +  // self-signed
> > +  check_cert_err(cert_from_file('v3_self_signed.der'), SEC_ERROR_UNKNOWN_ISSUER);
> 
> Are these tests necessary? I think what would be more useful is a test that
> a v4 certificate issued by a trusted issuer verifies successfully.

We have that one.

Comment 69

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

Comment on attachment 8476901 [details] [diff] [review]
tests-v4-eq-v3

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

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +138,5 @@
>    check_ok(cert_from_file('v3_missing_bc_ee-v3_int-v1_ca.der'));
>    check_ok(cert_from_file('v3_bc_ee-v3_int-v1_ca.der'));
>    check_cert_err(cert_from_file('v1_bc_ee-v3_int-v1_ca.der'), SEC_ERROR_BAD_DER);
>    check_cert_err(cert_from_file('v2_bc_ee-v3_int-v1_ca.der'), SEC_ERROR_BAD_DER);
> +  check_ok(cert_from_file('v4_bc_ee-v3_int-v1_ca.der'));

I missed this the first time around. Looks good.

Comment 70

[Camilo Viecco (:cviecco)]

 central try: https://tbpl.mozilla.org/?tree=Try&rev=10b54216f2b1

Comment 71

[Camilo Viecco (:cviecco)]

Attachment: make-v4-equal-v3-beta

Comment 72

[Camilo Viecco (:cviecco)]

Attachment: tests-v4-eq-v3-beta

Comment 73

[Camilo Viecco (:cviecco)]

beta try:  https://tbpl.mozilla.org/?tree=Try&rev=517c5f71375f

Comment 74

[Camilo Viecco (:cviecco)]

Comment on attachment 8476975 [details] [diff] [review]
make-v4-equal-v3-beta

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

Keeping r+ from keeler. Rebased of the patch of the same name

Comment 75

[Camilo Viecco (:cviecco)]

Comment on attachment 8476976 [details] [diff] [review]
tests-v4-eq-v3-beta

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

keeping r+ from keeler, rebasing for beta

Comment 76

[Camilo Viecco (:cviecco)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/739a19828378
https://hg.mozilla.org/integration/mozilla-inbound/rev/948ff43797d4

Comment 77

[Camilo Viecco (:cviecco)]

Comment on attachment 8476899 [details] [diff] [review]
make-v4-equal-v3

Approval Request Comment
[Feature/regressing bug #]: Bug 915930 (Mozilla::pkix by default)
[User impact if declined]: Users wont be able to connect to certificates that claim to be v4
[Describe test coverage new/current, TBPL]: new tests added for self signed certs, modified tests to expect new behavior.
[Risks and why]: None, the code makes some behavior revert to something similar to NSS certverifier (dont pay attention to the certificate version) 
[String/UUID change made/needed]: None

Comment 78

[Camilo Viecco (:cviecco)]

Comment on attachment 8476975 [details] [diff] [review]
make-v4-equal-v3-beta

Approval Request Comment
[Feature/regressing bug #]: Bug 915930 (Mozilla::pkix by default)
[User impact if declined]: Users wont be able to connect to certificates that claim to be v4
[Describe test coverage new/current, TBPL]: new tests added for self signed certs, modified tests to expect new behavior.
[Risks and why]: None, the code makes some behavior revert to something similar to NSS certverifier (dont pay attention to the certificate version) 
[String/UUID change made/needed]: None

Comment 79

[Camilo Viecco (:cviecco)]

Attachment: make-v4-equal-v3-aurora

keeping r+ from keeler

Comment 80

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8476899 [details] [diff] [review]
make-v4-equal-v3

Clearing the aurora and beta noms on this patch as this patch is specific for m-c. There are separate patches for beta and aurora specific patches are on their way.

Comment 81

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8476975 [details] [diff] [review]
make-v4-equal-v3-beta

Approved for beta. Dropping the aurora approval request as an aurora specific patch is coming.

Comment 82

[Camilo Viecco (:cviecco)]

Attachment: tests-v4-eq-v3-aurora

keeping r+ from keeler

Comment 83

[Camilo Viecco (:cviecco)]

Comment on attachment 8477014 [details] [diff] [review]
make-v4-equal-v3-aurora


Approval Request Comment
[Feature/regressing bug #]: Bug 915930 (Mozilla::pkix by default)
[User impact if declined]: Users wont be able to connect to certificates that claim to be v4
[Describe test coverage new/current, TBPL]: new tests added for self signed certs, modified tests to expect new behavior.
[Risks and why]: None, the code makes some behavior revert to something similar to NSS certverifier (dont pay attention to the certificate version) 
[String/UUID change made/needed]: None

Comment 84

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8477014 [details] [diff] [review]
make-v4-equal-v3-aurora

Approved for aurora.

Comment 85

[Camilo Viecco (:cviecco)]

aurora push:
https://hg.mozilla.org/releases/mozilla-aurora/rev/912f3087ec0c
https://hg.mozilla.org/releases/mozilla-aurora/rev/dfdc5879d992

Comment 86

[Camilo Viecco (:cviecco)]

beta push:
https://hg.mozilla.org/releases/mozilla-beta/rev/6049537c2510
https://hg.mozilla.org/releases/mozilla-beta/rev/74a58e14d1d3

Comment 87

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

https://hg.mozilla.org/mozilla-central/rev/739a19828378
https://hg.mozilla.org/mozilla-central/rev/948ff43797d4

Comment 88

[Lukas Blakk [:lsblakk] use ?needinfo]

As this wasn't tracked for a specific ESR 31 release, it didn't make it into 31.2.0 ESR so tracking for ESR 31.3.0. Please nominate a patch for approval-mozilla-esr31 (either the current one, if it lands cleanly, or an adjusted fix).

Comment 90

[Camilo Viecco (:cviecco)]

Attachment: v4-as-v3-esr31

Comment 91

[Camilo Viecco (:cviecco)]

Attachment: tests-esr31

Comment 92

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

Comment on attachment 8505300 [details] [diff] [review]
v4-as-v3-esr31

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

This looks good.

::: security/pkix/lib/pkixbuild.cpp
@@ +39,2 @@
>    }
>    // We only decode v3 extensions for v3 certificates for two reasons.

We should update this comment as well.

Comment 93

[Benjamin Kerensa [:bkerensa]]

(In reply to Camilo Viecco (:cviecco) from comment #91)
> Created attachment 8505301 [details] [diff] [review]
> tests-esr31

Could you nominate for approval when this is ready?

Comment 94

[dian]

This is still not fixed. Just upgraded to 33.0 and I still get this error:
Secure Connection Failed
An error occurred during a connection to 192.168.2.1. The key does not support the requested operation. (Error code: sec_error_invalid_key)
___
my local router is with a self signed cert - what could possible be a security concern here and why should I pay Verisign $$ for a recognised cert, to make Firefox 33.0 work with my router that has been working for years?!?
Can you please fix this? This is ridiculous...