Delegate digest operations to the TrustDomain in mozilla::pkix

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

+++ 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.
Status: NEW → ASSIGNED
Summary: [TRACKING] Delegate digest operations to the TrustDomain in mozilla::pkix → Delegate digest operations to the TrustDomain in mozilla::pkix
Target Milestone: --- → mozilla33
No longer blocks: 1036103
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.
Blocks: 916629
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*
Attachment #8454284 - Flags: review?(dkeeler) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/2ea91aa53633
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.