1036105 - Delegate digest operations to the TrustDomain in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

+++ This bug was initially created as a clone of Bug #1036103 +++
+++ This bug was initially created as a clone of Bug #1033561 +++

Right now pkixocsp.cpp hard-codes a call to PK11_HashBuf. Instead, we should delegate the digest operations to the TrustDomain like we delegate signature verification operations. This way, the TrustDomain could use a non-NSS-based digest functions and/or it could filter the inputs (e.g. enabling/disabling algorithms based on preferences) and/or it could record telemetry about which hash algorithms are being used.

Comment 1

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

Attachment: delegate-DigestBuf-to-TrustDomain.patch

Comment 2

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

This try run includes these changes:
https://tbpl.mozilla.org/?tree=Try&rev=e438d7c4f77a

This is tested in the OCSP unit tests for mozilla::pkix and also in the xpcshell tesets test_cert_signatures.js and the OCSP xpcshell test that tests signature verification.

Comment 3

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

Comment on attachment 8454284 [details] [diff] [review]
delegate-DigestBuf-to-TrustDomain.patch

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

::: security/pkix/include/pkix/pkix.h
@@ +127,5 @@
>  
> +// Computes the SHA-1 hash of the data in the current item.
> +//
> +// item contains the data to hash.
> +// digestBug must point to a buffer to where the SHA-1 hash will be written.

"digestBuf"

@@ +134,5 @@
> +//
> +// TODO(bug 966856): Add SHA-2 support
> +// TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
> +// other, extensive, memory safety efforts in mozilla::pkix, and we should find
> +// a way to provide a more-obviously-safe interface.

How about taking a reference to an array of the correct size?

@@ +135,5 @@
> +// TODO(bug 966856): Add SHA-2 support
> +// TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
> +// other, extensive, memory safety efforts in mozilla::pkix, and we should find
> +// a way to provide a more-obviously-safe interface.
> +SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

::: security/pkix/include/pkix/pkixtypes.h
@@ +306,5 @@
> +  // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
> +  // other, extensive, memory safety efforts in mozilla::pkix, and we should
> +  // find a way to provide a more-obviously-safe interface.
> +  static const size_t DIGEST_LENGTH = 20; // length of SHA-1 digest
> +  virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

::: security/pkix/lib/pkixkey.cpp
@@ +120,5 @@
>                                digestAlg, nullptr, pkcs11PinArg);
>  }
>  
> +SECStatus
> +DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf, size_t digestBufLen)

nit: uint8_t*

Comment 4

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

Comment on attachment 8454284 [details] [diff] [review]
delegate-DigestBuf-to-TrustDomain.patch

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

::: security/pkix/include/pkix/pkix.h
@@ +127,5 @@
>  
> +// Computes the SHA-1 hash of the data in the current item.
> +//
> +// item contains the data to hash.
> +// digestBug must point to a buffer to where the SHA-1 hash will be written.

Fixed.

@@ +134,5 @@
> +//
> +// TODO(bug 966856): Add SHA-2 support
> +// TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
> +// other, extensive, memory safety efforts in mozilla::pkix, and we should find
> +// a way to provide a more-obviously-safe interface.

I'd love to but the usage of this function in CreateEncodedOCSPResquest doesn't allow it.

@@ +135,5 @@
> +// TODO(bug 966856): Add SHA-2 support
> +// TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
> +// other, extensive, memory safety efforts in mozilla::pkix, and we should find
> +// a way to provide a more-obviously-safe interface.
> +SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

Fixed.

::: security/pkix/include/pkix/pkixtypes.h
@@ +306,5 @@
> +  // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
> +  // other, extensive, memory safety efforts in mozilla::pkix, and we should
> +  // find a way to provide a more-obviously-safe interface.
> +  static const size_t DIGEST_LENGTH = 20; // length of SHA-1 digest
> +  virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

Fixed.

::: security/pkix/lib/pkixkey.cpp
@@ +120,5 @@
>                                digestAlg, nullptr, pkcs11PinArg);
>  }
>  
> +SECStatus
> +DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf, size_t digestBufLen)

Fixed.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea91aa53633

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ea91aa53633