944179 - Use QuickDER to decode DER-encoded DSA and ECDSA signatures

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

Attachment: Patch

We should use QuickDER to decode DER-encoded DSA and ECDSA signatures.
The "classic" ASN.1 decoder does not detect extra bytes at the end of
the input buffer. The QuickDER decoder (SEC_QuickDERDecodeItem) detects
that and reports the SEC_ERROR_EXTRA_INPUT error.

The QuickDER decoder requires a non-null |arena| input, so I have to
create a PLArenaPool. I use a chunk size of 256 bytes because r and s
are each at most 66 bytes (NIST curve P-521).

Comment 1

[Wan-Teh Chang]

Comment on attachment 8339614 [details] [diff] [review]
Patch

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

::: lib/cryptohi/dsautil.c
@@ -200,5 @@
>  done:
> -    if (sig.r.data != NULL)
> -	PORT_Free(sig.r.data);
> -    if (sig.s.data != NULL)
> -	PORT_Free(sig.s.data);

sig.r.data and sig.s.data are now allocated from |arena| instead of
the heap, so we just need to free |arena|.

Comment 2

[Wan-Teh Chang]

Comment on attachment 8339614 [details] [diff] [review]
Patch

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

::: lib/cryptohi/dsautil.c
@@ +166,5 @@
>      SECStatus         status;
>      DSA_ASN1Signature sig;
>      SECItem           dst;
>  
>      PORT_Memset(&sig, 0, sizeof(sig));

It is possible that we no longer need to zero |sig| because we don't
need to do null pointer checks on sig.r.data and sig.s.data before
freeing them.

Comment 3

[Ryan Sleevi]

Comment on attachment 8339614 [details] [diff] [review]
Patch

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

r- , but for minor reasons that I think are trivially fixed

::: lib/cryptohi/dsautil.c
@@ +178,5 @@
>      if (result->data == NULL)
>  	goto loser;
>  
> +    /* 256 should enough room for r and s (2 * len). */
> +    arena = PORT_NewArena(256);

If NSS's ECC_MORE_THAN_SUITE_B is defined, the max EC key size is not 521, but 571.

We should instead be taking the max of 2*MAX_ECKEY_LEN (72 bytes) and DSA_MAX_SIGNATURE_LEN (64 bytes, aka DSA_MAX_SUBPRIME_LEN [32] * 2)

This at least matches cryptohi/secvfy.c's decoding calculations.

Alternatively, using a symbolic pre-processor macro here would be a good way of self-documenting the limits. Examining the NSS code-base for calls to PORT_NewArena([0-9]*) shows a problem of not really documenting how the sizes were determined.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Let's do this.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://nss-review.dev.mozaws.net/D142

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/20215ac64691