Closed Bug 1036080 Opened 5 years ago Closed 5 years ago

nsNSSCertificateDB::addCertFromBase64 exits early without changing trust bits of existing permanent certificates.

Categories

(Core :: Security: PSM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: hpathak, Assigned: hpathak)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch test_addCertFromBase64.diff (obsolete) — Splinter Review
The addCertFrombase64 function exits early without changing the trust bits for newly added permanent certificates. The attached xpcshell test demonstrates the behavior. "Test intermediate" gets added when a tls connection is made. The same certificate is added later on as untrusted using addCertFromBase64. But this does not cause subsequent connection test to fail.
Assignee: nobody → hpathak
Attachment #8452639 - Attachment is obsolete: true
Attachment #8452899 - Flags: review?(dkeeler)
Comment on attachment 8452899 [details] [diff] [review]
Bug1036080_fix_addCertFromBase64.diff

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

The fix is good. The test should be re-worked so it will run faster, in parallel with other tests, and on all platforms (see comments).

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1629,5 @@
>      return MapSECStatus(SECFailure);
>    }
>  
>    if (tmpCert->isperm) {
> +    return SetCertTrustFromString(newCert, aTrust);

Maybe add a comment like "There's already a certificate that matches this one in the database. We still want to set its trust to the given value."

::: security/manager/ssl/tests/unit/test_addcertFromBase64.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Maybe this file should be called test_add_preexisting_cert.js.

@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +do_get_profile();
> +var certDB = Cc["@mozilla.org/security/x509certdb;1"]

nit: for new code, we use 'let' instead of 'var' in the majority of cases

@@ +17,5 @@
> +                      clearSessionCache);
> +}
> +
> +function run_test() {
> +  add_tls_server_setup("BadCertServer");

The problem with the tls server tests is that they're slow, can't be run in parallel, and don't run on Android. We should re-work this test to be a bit like the others that don't use the tls server. I'm thinking something like this:

load_cert("some_ca", "CTu,CTu,CTu"); // make sure the root cert is in the database
load_cert("some_intermediate", ",,"); // make sure the intermediate cert is in the database (it inherits its trust from the root)
let ee = certdb.constructX509(read_file("ee-file"));
certdb.verifyCertNow(ee, ...); // this should succeed
let intermediateBase64 = <get base64 encoding of intermediate>;
certdb.addCertFromBase64(intermediateBase64, "p,p,p"); // now the intermediate is actively distrusted
certdb.verifyCertNow(ee, ...); // this should fail

You should be able to use an appropriate (root, intermediate, ee) triple from the certificates in test_intermediate_basic_usage_constraints/

::: security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ +52,5 @@
>    { "bad.include-subdomains.pinning.example.com", "otherIssuerEE" },
>    { "exclude-subdomains.pinning.example.com", "localhostAndExampleCom" },
>    { "sub.exclude-subdomains.pinning.example.com", "otherIssuerEE" },
>    { "test-mode.pinning.example.com", "otherIssuerEE" },
> +  { "test-blocklist-inter.example.com", "ocspEEWithIntermediate"},

We actually shouldn't need to do this - see the comments in test_addcertFromBase64.js.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +93,5 @@
>  skip-if = os == "android"
> +[test_addcertFromBase64.js]
> +run-sequentially = hardcoded ports
> +# Bug 1036080: this test times out on Android
> +skip-if = os == "android"

After implementing the suggestions in test_addcertFromBase64.js, neither run-sequentially nor skip-if should be necessary.
Attachment #8452899 - Flags: review?(dkeeler) → review-
Attachment #8452899 - Attachment is obsolete: true
Attachment #8453388 - Flags: review?(dkeeler)
Comment on attachment 8453388 [details] [diff] [review]
Bug1036080_test_addCertFromBase64.diff

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

Looking good. This is almost ready to go, but I'd like one last look, so r- for now.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1628,5 @@
>      NS_ERROR("Couldn't create cert from DER blob");
>      return MapSECStatus(SECFailure);
>    }
>  
> +  /**

nit: use //-style comments here

::: security/manager/ssl/tests/unit/test_add_preexisting_cert.js
@@ +25,5 @@
> +
> +function add_preexisting_cert(name) {
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let cert_der = readFile(do_get_file("test_intermediate_basic_usage_constraints"+

nit: add a space before the '+'
(also, I would have the '/' on this line, not the next one)

@@ +29,5 @@
> +  let cert_der = readFile(do_get_file("test_intermediate_basic_usage_constraints"+
> +                                      "/ee-int-limited-depth.der"));
> +
> +  let ee = certDB.constructX509(cert_der, cert_der.length);
> +  let error_code = certDB.verifyCertNow(ee,certificateUsageSSLServer,

nit: space after the first ','

@@ +31,5 @@
> +
> +  let ee = certDB.constructX509(cert_der, cert_der.length);
> +  let error_code = certDB.verifyCertNow(ee,certificateUsageSSLServer,
> +                                        NO_FLAGS, verifiedChain, hasEVPolicy);
> +  do_check_eq(error_code, Cr.NS_OK);

it might be better to do:

equal(Cr.NS_OK, certDB.verifyCertNow(...));

(looks like do_check_eq, etc. are deprecated: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests )

@@ +34,5 @@
> +                                        NO_FLAGS, verifiedChain, hasEVPolicy);
> +  do_check_eq(error_code, Cr.NS_OK);
> +
> +  /**
> +   * Change the already existing intermediate certificates trust using

In general, use //-style comments except for the modeline/license boilerplate and in idl files
also: "certificate's"

@@ +38,5 @@
> +   * Change the already existing intermediate certificates trust using
> +   * addCertFromBase64(). We use findCertByNickname first to ensure that the
> +   * certificate already exists.
> +   */
> +  let int_cert = certDB.findCertByNickname(null, name);

add "ok(int_cert);"

@@ +43,5 @@
> +  let base64_cert = btoa(getDERString(int_cert));
> +  certDB.addCertFromBase64(base64_cert, "p,p,p", "ignored_argument");
> +  error_code = certDB.verifyCertNow(ee,certificateUsageSSLServer,
> +                                    NO_FLAGS, verifiedChain, hasEVPolicy);
> +  do_check_eq(error_code, SEC_ERROR_UNTRUSTED_ISSUER);

equal(SEC_ERROR_UNTRUSTED_ISSUER, certDB.verifyCertNow(...));
Attachment #8453388 - Flags: review?(dkeeler) → review-
Attachment #8453388 - Attachment is obsolete: true
Attachment #8453906 - Flags: review?(dkeeler)
Comment on attachment 8453906 [details] [diff] [review]
Bug1036080_test_addCertFromBase64.diff

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

Great - r=me with nits addressed.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1630,5 @@
>    }
>  
> +   // There's already a certificate that matches this one in the database.
> +   // We still want to set its trust to the given value.
> +

nit: unnecessary blank line
And, also, as long as the comment is outside of the if statement, we should make it conditional:
"If there's already a cert..., we still want to..."

::: security/manager/ssl/tests/unit/test_add_preexisting_cert.js
@@ +26,5 @@
> +function add_preexisting_cert(name) {
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let cert_der = readFile(do_get_file("test_intermediate_basic_usage_constraints/"
> +                                      + "ee-int-limited-depth.der"));

nit: '+' on the previous line (if that makes the line too long, you can separate the filename out in its own declaration)

@@ +35,5 @@
> +
> +  // Change the already existing intermediate certificate's trust using
> +  // addCertFromBase64(). We use findCertByNickname first to ensure that the
> +  // certificate already exists.
> +

nit: unnecessary blank line

@@ +49,5 @@
> +
> +function run_test() {
> +  load_cert("ca", "CTu,CTu,CTu");
> +  load_cert("int-limited-depth", "CTu,CTu,CTu");
> +  add_preexisting_cert("int-limited-depth");

I'm not sure it makes much sense to pass in the intermediate's name as a variable when everything else is hard-coded. Why not just put the contents of add_preexisting_cert here?
Attachment #8453906 - Flags: review?(dkeeler) → review+
Attachment #8453906 - Attachment is obsolete: true
Attachment #8453931 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b7e9c0479666
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.