Closed Bug 921890 Opened 7 years ago Closed 7 years ago

Add NSS-based key extraction and signature verification to insanity::pkix

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

No description provided.
Severity: normal → enhancement
Status: NEW → ASSIGNED
No longer depends on: 916629
Priority: -- → P1
Target Milestone: --- → mozilla29
Attached patch bug-921890.patchSplinter Review
Attachment #8363480 - Flags: review?(dkeeler)
Attachment #8363480 - Flags: review?(cviecco)
Comment on attachment 8363480 [details] [diff] [review]
bug-921890.patch

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

::: security/insanity/lib/pkixkey.cpp
@@ +66,5 @@
> +  // TODO: Ideally, we would do this check before we call
> +  // VFY_VerifyDataWithAlgorithmID. But, VFY_VerifyDataWithAlgorithmID gives us
> +  // the hash algorithm so it is more convenient to do things in this order.
> +  uint32_t policy;
> +  if (NSS_GetAlgorithmPolicy(hashAlg, &policy) != SECSuccess) {

PORT_SetError(SEC_ERROR_INVALID_ARGS); ? (I noticed NSS_GetAlgorithmPolicy can return without setting the error)
Attachment #8363480 - Flags: review?(cviecco) → review+
Comment on attachment 8363480 [details] [diff] [review]
bug-921890.patch

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

LGTM.

::: security/insanity/include/insanity/pkix.h
@@ +18,5 @@
> +#ifndef insanity_pkix__pkix_h
> +#define insanity_pkix__pkix_h
> +
> +#include "seccomon.h"
> +#include "certt.h"

Let's sort these includes.

::: security/insanity/lib/pkixkey.cpp
@@ +20,5 @@
> +#include "insanity/pkix.h"
> +#include "cert.h"
> +#include "prerror.h"
> +#include "secerr.h"
> +#include "cryptohi.h"

Let's separate/sort these includes to match Mozilla style, unless you have a strong opinion against it.

@@ +30,5 @@
> +                 void* pkcs11PinArg)
> +{
> +  if (!sd || !sd->data.data || !sd->signatureAlgorithm.algorithm.data ||
> +      !sd->signature.data) {
> +    PR_NOT_REACHED("invalid args to VerifySignedData");

cert should also not be null, right?
Attachment #8363480 - Flags: review?(dkeeler) → review+
(In reply to Camilo Viecco (:cviecco) from comment #2)
> PORT_SetError(SEC_ERROR_INVALID_ARGS); ? (I noticed NSS_GetAlgorithmPolicy
> can return without setting the error)

If NSS_GetAlgorithmPolicy can return without setting the error code, that is a bug in NSS_GetAlgorithmPolicy that needs to be fixed. Please double check and file the bug if necessary.
Flags: needinfo?(cviecco)
recheked and my initial read was incorrect. There is no fail in NSS_GetAlgorithmPolicy (assuming dynamic OID's) are setup correctly.
Flags: needinfo?(cviecco)
https://hg.mozilla.org/mozilla-central/rev/fbc2aa4d5f78
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8363480 [details] [diff] [review]
bug-921890.patch

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8363480 - Flags: approval-mozilla-aurora?
Comment on attachment 8363480 [details] [diff] [review]
bug-921890.patch

Uplifted granted to the patches relative to the new feature: "Add insanity::pkix as certificate verification option"
Lukas and I discussed with Brian and we think it is important to have this feature for 29 (but disabled by default).
It is early in the aurora process and they have plenty of tests for these feature (and to make sure that the current behaviors are still performing correctly).
Attachment #8363480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.