1045973 - sec_error_extension_value_invalid: mozilla::pkix does not accept certificates with x509v3 extensions in x509v1 or x509v2 certificates

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[pehrlich]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36

Steps to reproduce:

I'm adding a self-signed root cert via the NSS/NSPR C++ API.

This needs one x509v3 extension, the subject alt name.  


```C++
    // Add subjectAltName x509v3 extension containing our localhost IPv4
    // address of 127.0.0.1.  The subjectAltName entry takes precedence over
    // the CommonName (CN) entry, thus we are allowed to have a more
    // descriptive name there. In addition, this is needed by Safari on Mac in
    // order to properly trust the certificate.
    X509V3_CTX ctx;
    X509V3_set_ctx_nodb(&ctx);
    X509V3_set_ctx(&ctx, m_x509, m_x509, nullptr, nullptr, 0);

    // Removing this line causes the cert to be accepted by firefox:
    X509_EXTENSION* ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_subject_alt_name, (char*)"DNS:127.0.0.1,IP:127.0.0.1");
    
    if (ext) {
      X509_add_ext(m_x509, ext , -1);
      X509_EXTENSION_free(ext);
    }
    
    // Sign the certificate
    X509_sign(m_x509, m_key->m_pkey, EVP_sha1());
```




Actual results:

Using this extension, or indeed any x509v3 extension, causes firefox to fail with `Error code: sec_error_extension_value_invalid`.


Expected results:

This appears to be a pkix bug, as setting `use_mozillapkix_verification = false`  in `about:config`, or using ff < 31, causes the cert to be accepted OK.

There is a linked SO issue: http://stackoverflow.com/questions/25028136/adding-x509v3-extension-causes-mozilla-pkix-not-to-trust

Comment 1

[Camilo Viecco (:cviecco)]

One question and one request:
1. Can you please put the misbehaving cert in this bug?
2. Are you sure you are using a v3 certificate? (NSS accepts v1/v2 ceritificates with v3 extensions, mozilla::pkix does not).

Comment 2

[Camilo Viecco (:cviecco)]

Opps. O forgot to make neeed info, can you supply the information in comment 1?

Comment 3

[pehrlich]

@Camilo bingo!  Setting the cert to v3 solved the issue, saving us a ton of time:

    X509_set_version(m_x509, 2L);

Comment 4

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

If this is found to be common issue we might have to reopen this issue and change mozilla::pkix to ignore the version number for the purposes of deciding whether to accept extensions.

Comment 5

[pehrlich]

FWIW - That is the behavior of Chrome/Safari: they accepted the certificate w/o the explicit versioning.

Comment 6

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

Let's just go ahead and reopen the bug until we have a chance to discuss what to do.

Comment 7

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

Here are some more websites getting the same error, reported in bug 987969:
https://saanvi.ru
https://krasdos.ru
https://krasraritet.com

Comment 8

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

[Tracking Requested - why for this release]: Compatibility regression where websites (possibly many) that use self-signed certificates and/or private CAs do not work in Firefox 31 and later.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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

Comment 10

[Mikhail Smirnov]

website getting the same error
https://tw.risbi.ru/itr/

Comment 11

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

Brian - We're almost out of time for Firefox 32. Did you discuss what to do as you suggested in comment 6?

Comment 12

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

As we're at the end of the 32 beta cycle, I'm marking this as won't fix for Firefox 32. I would like to see some progress soon (before uplift on Sep 2 if possible) if this is to be fixed in 33.

Comment 13

[Camilo Viecco (:cviecco)]

Attachment: fix-v1-self-signed

Comment 14

[Camilo Viecco (:cviecco)]

Attachment: tests-v1-allow-self-signed-with-ext

Comment 15

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

This bug is actually very much like bug 1047177. I have learned that some (all?) versions of OpenSSL's API for generating certificates default to generating v1 certificates. The API allows you to add extensions to any cert, regardless of version. It seems there are tools out there that use the OpenSSL API to create certificates with extensions, but then forget to change the version number.

The best fix is to change the version number checks from the extension parsing code in mozilla::pkix so that v1 (and v4) certificates with extensions are accepted.

Comment 16

[Camilo Viecco (:cviecco)]

Attachment: allow-v1-self-signed-with-extensions

Comment 17

[Camilo Viecco (:cviecco)]

Attachment: tests-v1-ok

Comment 18

[Camilo Viecco (:cviecco)]

try push 

https://tbpl.mozilla.org/?tree=Try&rev=16af74dfae18

Comment 19

[Camilo Viecco (:cviecco)]

Attachment: allow-v1-self-signed-with-extensions (v1.1)

Comment 20

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

Comment on attachment 8477493 [details] [diff] [review]
allow-v1-self-signed-with-extensions (v1.1)

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

r- until we decide on what the right thing to do is. See comment.

::: security/pkix/lib/pkixcert.cpp
@@ +151,5 @@
>    // as v3 certificates.
> +  // For compatibity reasons we will process v3 extensions on v1 certificates
> +  // only when self-issued.
> +  if (version == der::Version::v3 || version == der::Version::v4 ||
> +      (version == der::Version::v1 && InputsAreEqual(issuer, subject))) {

My concern here is that this doesn't actually enforce that the certificate is self-signed. How do you plan to deal with this? If it's not a concern, then maybe we should just be ok with v1 certificates having v3 extensions?

Comment 21

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

Comment on attachment 8477491 [details] [diff] [review]
tests-v1-ok

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

Clearing review until we decide on how we want the behavior of v1 certificates to change.

Comment 22

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

I propose we make the following changes:

* Accept v1, v2, v3, v4 and treat all those versions as being 100% equivalent except as noted below. 
* Reject other versions with a hard failure (so that we don't have poor interaction with cert error override ranking).
* Continue to have an exception for v1 trust anchors that do not have basicConstraints extension. AFAICT, this is the only place where we should be using the stored version field.

This logic is clear and easy to understand. I don't think we're compromising security in any way with this logic; AFAICT, it is still stricter than what NSS, OpenSSL, and Windows CryptoAPI are doing.

My understanding is that there is a group working on a new version of X509 (believe it or not), and that version will be v5. I'm guessing they skipped over v4 due to the off-by-one issues in encoding v3 that we've seen elsewhere.

Comment 23

[Richard Barnes [:rbarnes]]

Attachment: bug-1045973.patch

This patch just removes the version guards around parsing of fields that are disallowed in v1 and v2 certificates.  In terms of Brian's plan in comment 22:

* This patch removes the parsing distinctions.  I couldn't find others.
* Other versions are rejected by der::OptionalVersion().
* The exception for v1 TAs is still there.

Comment 24

[Richard Barnes [:rbarnes]]

Comment on attachment 8477491 [details] [diff] [review]
tests-v1-ok

I believe these tests are still valid given the plan in comment 22.

Comment 25

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

Richard, could you actually prepare a diff -w (i.e. no-whitespace changes) version of the fix patch? That'll make it a lot easier to review. Thanks.

Comment 26

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

Comment on attachment 8477491 [details] [diff] [review]
tests-v1-ok

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

Ok - r=me given there's a good explanation as to why those two test-cases still fail (and with other comments addressed).

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +205,5 @@
>    ca_error = SEC_ERROR_EXTENSION_VALUE_INVALID;
> +  ee_error = SEC_ERROR_BAD_DER;
> +  check_ok_ca(cert_from_file('v3_int-v1_ca_bc.der'));
> +  check_ok(cert_from_file('v1_ee-v3_int-v1_ca_bc.der'));
> +  check_cert_err(cert_from_file('v1_bc_ee-v3_int-v1_ca_bc.der'), ee_error);

I don't have a good explanation for why this would still fail - why isn't this case ok?

@@ +207,5 @@
> +  check_ok_ca(cert_from_file('v3_int-v1_ca_bc.der'));
> +  check_ok(cert_from_file('v1_ee-v3_int-v1_ca_bc.der'));
> +  check_cert_err(cert_from_file('v1_bc_ee-v3_int-v1_ca_bc.der'), ee_error);
> +  check_ok(cert_from_file('v2_ee-v3_int-v1_ca_bc.der'));
> +  check_cert_err(cert_from_file('v2_bc_ee-v3_int-v1_ca_bc.der'), ee_error);

Same here.

@@ +210,5 @@
> +  check_ok(cert_from_file('v2_ee-v3_int-v1_ca_bc.der'));
> +  check_cert_err(cert_from_file('v2_bc_ee-v3_int-v1_ca_bc.der'), ee_error);
> +  check_ok(cert_from_file('v3_missing_bc_ee-v3_int-v1_ca_bc.der'));
> +  check_ok(cert_from_file('v3_bc_ee-v3_int-v1_ca_bc.der'));
> +  check_ok(cert_from_file('v4_bc_ee-v3_int-v1_ca_bc.der'));

Ugh - poor naming strikes again. I was reading this as 'a v4 end-entity with basic-constraints signed by a v3 intermediate without basic constraints', but I think in the case of v3, under this naming convention, a v3 cert has basic constraints if the name doesn't mention it. It would be good to either verify and/or fix the naming.

@@ +492,5 @@
>    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('v1_self_signed.der'), SEC_ERROR_UNKNOWN_ISSUER);
> +  check_cert_err(cert_from_file('v1_self_signed_bc.der'), SEC_ERROR_UNKNOWN_ISSUER);

v2 as well?

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +76,5 @@
>  
> +
> +  CertUtils.generate_self_signed_cert(db, srcdir, noise_file, "v1_self_signed",
> +                                      1, False, False)
> +  CertUtils.generate_self_signed_cert(db, srcdir, noise_file, "v1_self_signed_bc",

v2 as well?

Comment 27

[Richard Barnes [:rbarnes]]

Attachment: bug-1045973.wdiff

Here's the wdiff (with a slightly improved comment).  Everything else is just changing indent levels.

Comment 28

[Richard Barnes [:rbarnes]]

Attachment: bug-1045973.wdiff

This one is should be easier to read.

Comment 29

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

Comment on attachment 8491870 [details] [diff] [review]
bug-1045973.wdiff

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

Ok - this looks good with comments addressed. I'll add the r+ to the actual patch so we don't accidentally check this one in.

::: pkixcert.cpp
@@ +124,5 @@
>  
>    static const uint8_t CSC = der::CONTEXT_SPECIFIC | der::CONSTRUCTED;
>  
> +  // According to RFC 5280, all fields below are forbidden for certificates
> +  // indicating a version less than v3.  However, for compatibility reasons,

I would actually leave the comment as is, and then add everything after "However, ..."

@@ +125,5 @@
>    static const uint8_t CSC = der::CONTEXT_SPECIFIC | der::CONSTRUCTED;
>  
> +  // According to RFC 5280, all fields below are forbidden for certificates
> +  // indicating a version less than v3.  However, for compatibility reasons,
> +  // we will parse v1/v2 certificates according to the v3 rules.  The same

nit: I think we commonly have only one space after a period.

@@ +150,1 @@
>    rv = der::OptionalExtensions(tbsCertificate, CSC | 3,

I would leave in a comment here along the lines of "Technically, only v3 certificates may have extensions. However, again for compatibility reasons, we essentially treat v1, v2, v3, and v4 certificates the same in this case."

Comment 30

[Richard Barnes [:rbarnes]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40d6ccba44f1

Comment 31

[Richard Barnes [:rbarnes]]

aaand bounce.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8de2cc24f3

Need to update the xpcshell tests so that they don't expect to fail on v1/v2 certs.

Comment 32

[Richard Barnes [:rbarnes]]

Attachment: bug-1045973.1.patch

Keeler, could you take a quick look at the test changes in this patch?  In particular, make sure that the semantics look correct to you?

All the tests pass with these modifications, and they look superficially sensible to me:
-- Some BAD_DER -> OK
-- Some BAD_DER -> CERT_INVALID

But it would be good if you could confirm.

Comment 33

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

Comment on attachment 8493705 [details] [diff] [review]
bug-1045973.1.patch

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

Yes, this looks correct. Please also update the comments.

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +24,5 @@
>    let hasEVPolicy = {};
>    let verifiedChain = {};
>    let error = certdb.verifyCertNow(cert, usage,
>                                     NO_FLAGS, verifiedChain, hasEVPolicy);
> +  dump("error=" + error + "    expected=" + expected_error + "\n");

nit: we don't need this - do_check_eq prints this information, right?

@@ +88,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v1_int-v1_ca.der'), ee_error);
>  
>    // v1 intermediate with v3 extensions. CA is invalid.

nit: update comment

@@ +110,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v2_int-v1_ca.der'), ee_error);
>  
>    // A v2 intermediate with basic constraints (not allowed in mozilla::pkix)

nit: update comment

@@ +121,5 @@
>  
>    // Section is OK. A x509 v3 CA MUST have bc
>    // http://tools.ietf.org/html/rfc5280#section-4.2.1.9
>    ca_error = SEC_ERROR_CA_CERT_INVALID;
>    ee_error = SEC_ERROR_CA_CERT_INVALID;

nit: we don't use ca_error or ee_error in this block, so we may as well remove these here

@@ +159,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v1_int-v1_ca_bc.der'), ee_error);
>  
>    // Using a v1 intermediate with v3 extenstions (invalid).

nit: update comment

@@ +181,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v2_int-v1_ca_bc.der'), ee_error);
>  
>    // Using a v2 intermediate with basic constraints (invalid)

nit: update comment

@@ +203,5 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v3_int_missing_bc-v1_ca_bc.der'), ee_error);
>  
>    // these should pass assuming we are OK with v1 ca signing v3 intermediates
>    ca_error = SEC_ERROR_EXTENSION_VALUE_INVALID;
> +  ee_error = SEC_ERROR_BAD_DER;

nit: we don't use ca_error or ee_error in this block, so we might as well remove them

@@ +304,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v1_int-v2_ca_bc.der'), ee_error);
>  
>    // v2 ca, v1 intermediate with bc (invalid)

nit: update comment

@@ +326,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v2_int-v2_ca_bc.der'), ee_error);
>  
>    // v2 ca, v2 intermediate with bc (invalid)

nit: update comment

@@ +348,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v3_int_missing_bc-v2_ca_bc.der'), ee_error);
>  
>    // v2 ca, valid v3 intermediate (is OK if we use 'classic' semantics)

nit: update comment

@@ +396,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v2_int-v3_ca.der'), ee_error);
>  
>    // v2 intermediate with bc (invalid)

nit: update comment

@@ +418,4 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v3_int_missing_bc-v3_ca.der'), ee_error);
>  
>    // I dont think that v3 intermediates should be allowed to sign v1 or v2
>    // certs, but other thanthat this  is what we usually get in the wild.

nit: this comment can be removed

@@ +441,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v1_int-v3_ca_missing_bc.der'), ee_error);
>  
>    // Int v1 with BC that is just invalid (classic fail insanity OK)

nit: update comment

@@ +465,3 @@
>    check_cert_err(cert_from_file('v4_bc_ee-v2_int-v3_ca_missing_bc.der'), ee_error);
>  
>    // v2 intermediate (even with basic constraints) is invalid

This comment was always wrong, looks like.

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +77,5 @@
> +
> +  CertUtils.generate_self_signed_cert(db, srcdir, noise_file, "v1_self_signed",
> +                                      1, False, False)
> +  CertUtils.generate_self_signed_cert(db, srcdir, noise_file, "v1_self_signed_bc",
> +                                      1, True, False)

I think it might be worth it to also generate two v2 certs like the v1 certs generated here (and then have the corresponding tests at the bottom of test_cert_version.js).

Comment 34

[Richard Barnes [:rbarnes]]

Attachment: bug-1045973.2.patch

Addressing keeler's test comments, carrying forward r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c75153cd1a0

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c75153cd1a0

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Richard, could you fill the uplift requests to aurora & beta? thanks

Comment 37

[Richard Barnes [:rbarnes]]

Comment on attachment 8494001 [details] [diff] [review]
bug-1045973.2.patch

[Feature/regressing bug #]: The need for uplift to 33/34 is a result of mozilla::pkix becoming mandatory in those releases (Bug 975229)

[User impact if declined]: This bug causes Firefox to present non-overrideable certificate errors when it encounters certain certificates, which are apparently generated by some devices.  This behavior could be disabled with use_mozillapkix_verification=false in Firefox <=32, but that will not be possible in 33.

[Describe test coverage new/current, TBPL]: Updates to xpcshell tests confirmed expected behavior.

[Risks and why]: Low risk.  Just removes a condition that caused different processing for a relatively uncommon class of certificates.

[String/UUID change made/needed]: None

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/314722b429da
https://hg.mozilla.org/releases/mozilla-beta/rev/a4697303afa6

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

This does not meet ESR landing criteria.

Comment 40

[otten]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #39)
> This does not meet ESR landing criteria.

Why doesn't it meet ESR landing criteria? Since this is not really a new feature it could be added to ESR as well, couldn't it?

Comment 41

[hatta0713]

+1

SSL WEB filter service like symantec.cloud and firewalls like fortigate
needs their private CA certificate.

It is really critical for companies.

(In reply to otten from comment #40)
> (In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #39)
> > This does not meet ESR landing criteria.
> 
> Why doesn't it meet ESR landing criteria? Since this is not really a new
> feature it could be added to ESR as well, couldn't it?

Comment 42

[Michael Torrie]

Is there any easy way to take a certificate and run it through libpkix in a way that shows me exactly what it's tripping up over?  Firefox's error is useless, and Thunderbird doesn't seem to give me any error other than "unknown error has occurred" while talking to the server.  My certs are generated with a tool called XCA (uses openssl under the hood).  I have my own Certificate CA, but libpkix doesn't appear to like it, or my signed certs, but I don't know how to identify exactly what isn't liked.  

Presumably I could/should fix my CA and resign all my certs.  But I can't if I can't determine what's wrong with them in the first place.  I checked the version number as reported by an older version of Firefox and it says the cert is v3.  So I dunno.

In my last job we ran a number of internal certs this way, signed by an internal CA.  Glad I'm not working there now, as this latest round of firefox updates would have broken a lot of things.