Closed Bug 1045973 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Security: PSM, defect)

31 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox31 - wontfix
firefox32 + wontfix
firefox33 + fixed
firefox34 --- fixed
firefox35 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: pehrlich, Assigned: rbarnes)

Details

(Keywords: site-compat)

Attachments

(1 file, 9 obsolete files)

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
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).
Opps. O forgot to make neeed info, can you supply the information in comment 1?
Flags: needinfo?(pehrlich)
@Camilo bingo!  Setting the cert to v3 solved the issue, saving us a ton of time:

    X509_set_version(m_x509, 2L);
Flags: needinfo?(pehrlich)
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Summary: [Fx31] Adding x509v3 extension causes Mozilla pkix not to trust → [Fx31] Adding x509v3 extension on non-v3 causes Mozilla pkix not to trust
Resolution: FIXED → INVALID
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.
Keywords: compat
Summary: [Fx31] Adding x509v3 extension on non-v3 causes Mozilla pkix not to trust → mozilla::pkix does not accept certificates with x509v3 extensions in x509v1 or x509v2 certificates
FWIW - That is the behavior of Chrome/Safari: they accepted the certificate w/o the explicit versioning.
Let's just go ahead and reopen the bug until we have a chance to discuss what to do.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Here are some more websites getting the same error, reported in bug 987969:
https://saanvi.ru
https://krasdos.ru
https://krasraritet.com
Summary: mozilla::pkix does not accept certificates with x509v3 extensions in x509v1 or x509v2 certificates → sec_error_extension_value_invalid: mozilla::pkix does not accept certificates with x509v3 extensions in x509v1 or x509v2 certificates
[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.
We won't release a new 31 for this but we might take a patch for ESR 31.
website getting the same error
https://tw.risbi.ru/itr/
Brian - We're almost out of time for Firefox 32. Did you discuss what to do as you suggested in comment 6?
Flags: needinfo?(brian)
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.
Assignee: nobody → cviecco
Attached patch fix-v1-self-signed (obsolete) — Splinter Review
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.
Flags: needinfo?(brian)
Attachment #8476336 - Attachment is obsolete: true
Attached patch tests-v1-ok (obsolete) — Splinter Review
Attachment #8476337 - Attachment is obsolete: true
Attachment #8477490 - Attachment is obsolete: true
Attachment #8477493 - Flags: review?(dkeeler)
Attachment #8477491 - Flags: review?(dkeeler)
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?
Attachment #8477493 - Flags: review?(dkeeler) → review-
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.
Attachment #8477491 - Flags: review?(dkeeler)
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.
Assignee: cviecco → rlb
Attached patch bug-1045973.patch (obsolete) — Splinter Review
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.
Attachment #8477493 - Attachment is obsolete: true
Attachment #8489656 - Flags: review?(dkeeler)
Comment on attachment 8477491 [details] [diff] [review]
tests-v1-ok

I believe these tests are still valid given the plan in comment 22.
Attachment #8477491 - Flags: review?(dkeeler)
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.
Flags: needinfo?(rlb)
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?
Attachment #8477491 - Flags: review?(dkeeler) → review+
Attached file bug-1045973.wdiff (obsolete) —
Here's the wdiff (with a slightly improved comment).  Everything else is just changing indent levels.
Attachment #8491853 - Flags: review?(dkeeler)
Flags: needinfo?(rlb)
Attached patch bug-1045973.wdiff (obsolete) — Splinter Review
This one is should be easier to read.
Attachment #8491853 - Attachment is obsolete: true
Attachment #8491853 - Flags: review?(dkeeler)
Attachment #8491870 - Flags: review?(dkeeler)
Attachment #8491870 - Attachment is patch: true
Attachment #8491870 - Attachment mime type: text/x-patch → text/plain
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."
Attachment #8491870 - Flags: review?(dkeeler)
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.
Attached patch bug-1045973.1.patch (obsolete) — Splinter Review
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.
Attachment #8477491 - Attachment is obsolete: true
Attachment #8489656 - Attachment is obsolete: true
Attachment #8491870 - Attachment is obsolete: true
Attachment #8493705 - Flags: review?(dkeeler)
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).
Attachment #8493705 - Flags: review?(dkeeler) → review+
Addressing keeler's test comments, carrying forward r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c75153cd1a0
Attachment #8494001 - Flags: review+
Attachment #8493705 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6c75153cd1a0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Richard, could you fill the uplift requests to aurora & beta? thanks
Flags: needinfo?(rlb)
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
Attachment #8494001 - Flags: approval-mozilla-beta?
Attachment #8494001 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rlb)
Attachment #8494001 - Flags: approval-mozilla-beta?
Attachment #8494001 - Flags: approval-mozilla-beta+
Attachment #8494001 - Flags: approval-mozilla-aurora?
Attachment #8494001 - Flags: approval-mozilla-aurora+
This does not meet ESR landing criteria.
(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?
+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?
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.
You need to log in before you can comment on or make changes to this bug.