1542207 - Limit policy check on signature algorithms to known algorithms

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Daiki Ueno [:ueno]]

Attachment: nss-dsa-policy.patch

This is a follow up fix of bug 1493936. In the fix, we added a policy check on every signature algorithm:
https://hg.mozilla.org/projects/nss/rev/4bc22e14a592#l5.23

In general, that shouldn't be a problem, as the majority of the algorithms are enabled by default. However, for the weak algorithms we disable by default, there's no way to re-enable them other than with the envvar:
https://searchfox.org/mozilla-central/source/security/nss/lib/util/secoid.c#2054

I'm attaching a patch that limits the check to DSA, as originally intended.

Comment 1

[Robert Relyea]

Comment on attachment 9056093 [details] [diff] [review]
nss-dsa-policy.patch

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

So neither this patch, nor the original is doing what you want it to do. The problem is you are trying to turn on or off the underlying signature algorithm, but you are triggering on the combined signature/hash algorithm. There's a function in verify that will split these out:
sec_DecodeSigAlg() It returns the hashAlg and the encAlg. 

You want to check policy based on the encAlg. You can do this either by making sec_DecodeSigAlg() locally exportable and calling it to get the encAlg, or by adding the check policy at the bottom of each of the blocks you have and then explicitly checking the corresponding encAlg (see the output of sec_DecodeSigAlg() for the result).

::: lib/certhigh/certvfy.c
@@ +129,5 @@
> +                !(policyFlags & NSS_USE_ALG_IN_CERT_SIGNATURE)) {
> +                PORT_SetError(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED);
> +                return SECFailure;
> +            }
> +            /* fall through */

This will not turn off all DSA sigature algorithms.

Comment 2

[Robert Relyea]

In general, that shouldn't be a problem, as the majority of the algorithms are enabled by default. However, for the weak algorithms
we disable by default, there's no way to re-enable them other than with the envvar:

These look like bugs:
xOids[SEC_OID_PKCS1_MD2_WITH_RSA_ENCRYPTION].notPolicyFlags = ~0;
xOids[SEC_OID_PKCS1_MD4_WITH_RSA_ENCRYPTION].notPolicyFlags = ~0;
xOids[SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION].notPolicyFlags = ~0;
If we turn MD5 on by policy, we should also turn on these, or we should never disable them to begin with.

These may also be issues, but we'd have to review the PKCS5 use of policy to determine if they are an issue:
xOids[SEC_OID_PKCS5_PBE_WITH_MD2_AND_DES_CBC].notPolicyFlags = ~0;
xOids[SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC].notPolicyFlags = ~0;

As we currently have policy set up, we should not be disabling the higher level oids. We should be disabling all these functions at the base level when the underlying hash/encryption is implemented. We should only disable them if we add things like rsa-md5 into the policy file.

Anyway triggering on the encryption algorithm will bypass this whole issue for now.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jcj, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Daiki Ueno [:ueno]]

Attachment: Bug 1542207, fix policy check on signature algorithms

Comment 5

[Daiki Ueno [:ueno]]

(In reply to Robert Relyea from comment #1)

Comment on attachment 9056093 [details] [diff] [review]
nss-dsa-policy.patch

Review of attachment 9056093 [details] [diff] [review]:

So neither this patch, nor the original is doing what you want it to do. The
problem is you are trying to turn on or off the underlying signature
algorithm, but you are triggering on the combined signature/hash algorithm.
There's a function in verify that will split these out:
sec_DecodeSigAlg() It returns the hashAlg and the encAlg.

You want to check policy based on the encAlg. You can do this either by
making sec_DecodeSigAlg() locally exportable and calling it to get the
encAlg, or by adding the check policy at the bottom of each of the blocks
you have and then explicitly checking the corresponding encAlg (see the
output of sec_DecodeSigAlg() for the result).

Yes, that's right - thank you for clearing my confusion :-) I have updated the patch on phabricator along these lines.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:ueno, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Daiki Ueno [:ueno]]

Pushed as:
https://hg.mozilla.org/projects/nss/rev/ed8a41d16c1c