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

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: bug-921890.patch

Comment 2

[Camilo Viecco (: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)

Comment 3

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

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?

Comment 4

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

(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.

Comment 5

[Camilo Viecco (:cviecco)]

recheked and my initial read was incorrect. There is no fail in NSS_GetAlgorithmPolicy (assuming dynamic OID's) are setup correctly.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc2aa4d5f78

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbc2aa4d5f78

Comment 8

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

Comment on attachment 8363480 [details] [diff] [review]
bug-921890.patch

[Approval Request Comment]
See bug 878932 comment 37.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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).

Comment 10

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

Comment on attachment 8363480 [details] [diff] [review]
bug-921890.patch

https://hg.mozilla.org/releases/mozilla-aurora/rev/107e019c0b8a