Closed Bug 1414931 Opened 2 years ago Closed 6 months ago

NSS accepts PKCS#1 v1.5 signatures made with RSA-PSS keys

Categories

(NSS :: Libraries, defect, P3, major)

3.34

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hkario, Assigned: mt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached file cert-verify-server.py
When NSS receives PKCS#1 v1.5 signature made using RSA-PSS key, it continues the connection. This happens both for Server Key Exchange and Certificate Verify.

Reproducer:
Generate rsa-pss key, sign it by a CA trusted by database in ./nssdb. Export to unencrypted files the key (host.key) and certificate (host.crt).

git clone https://github.com/tomato42/tlslite-ng.git
pushd tlslite-ng
git clone https://github.com/warner/python-ecdsa.git .python-ecdsa
ln -s .python-ecdsa/ecdsa ecdsa
popd

PYTHONPATH=tlslite-ng cert-verify-server.py -k host.key -c host.crt

connect to it using tstclnt:
tstclnt -d sql:nssdb -4 -h localhost -p 4433


Result:
connection established

Expected result:
connection aborted by client after receiving Server Key Exchange message
Hubert: From a design question, it appears you believe these should not be accepted.

I'm curious how/why you've reached that conclusion, and how you're defining "RSA-PSS key" - since, after all, both EMSA-PSS-Encode and EMSA-PKCS1-v1_5 are just transformations done before the RSA operation.

I mention this because there's considerable debate on how to restrict key usages (RFC 3447, RFC 4055, RFC 5756 capture those nuances)
by rsa-pss key I understand a certificate that has a Public Key type of RSA-PSS - i.e. key that is restricted to performing only rsa-pss signatures according to RFC 4055:

   When the RSA private key owner wishes to limit the use of the public
   key exclusively to RSASSA-PSS, then the id-RSASSA-PSS object
   identifier MUST be used in the algorithm field within the subject
   public key information, and, if present, the parameters field MUST
   contain RSASSA-PSS-params.
There's not a particularly compelling reason to limit, and further discussion captured in https://tools.ietf.org/html/rfc5756#section-1 highlights where aspects of this may be unwise.

In terms of other implementations, are you aware of implementations that limit in this way? Neither BoringSSL nor CryptoAPI do.
(In reply to Ryan Sleevi from comment #3)
> There's not a particularly compelling reason to limit, and further
> discussion captured in https://tools.ietf.org/html/rfc5756#section-1
> highlights where aspects of this may be unwise.

Section 1 of RFC 5756 does not talk about RSA-PSS at all...

The generic statement is about negotiation of KDFs. RSA-PSS is a _padding_ format.

Section 2 only talks about when the rsa-pss parameters need to be included or not. Not when the RSA-PSS key _type_ needs to be used in the first place.

To reiterate, I am not talking about a situation where we have rsa-pss key limited to using a single hash and MGF.

> In terms of other implementations, are you aware of implementations that
> limit in this way? Neither BoringSSL nor CryptoAPI do.

CryptoAPI does support RSA-PSS keys?
BoringSSL developers are not infallible - just recently we have discovered that they expect SHA-1 fallback in Certificate Verify when the RFC explicitly forbids that behaviour.
I've tested OpenSSL 1.1.1-dev (master fb9163ba4d) and it rejects such signatures by sending a decode_error and printing 
139899071163200:error:1414D172:SSL routines:tls12_check_peer_sigalg:wrong signature type:ssl/t1_lib.c:913:

BoringSSL (master d5dda9b803f) and it rejects it too with a decode_error and prints:
40160216:error:06000066:public key routines:OPENSSL_internal:DECODE_ERROR:/home/hkario/dev/boringssl/crypto/evp/evp_asn1.c:110:

so it doesn't look to me like they don't limit rsa-pss key usage, exactly as the RFC requires them to do
Priority: -- → P3
Blocks: 158750
Attachment #8960980 - Attachment is obsolete: true
This seems to be fixed as part of bug 1428208 (thank you Martin!). Closing.
Assignee: nobody → martin.thomson
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → 3.39
I have verified client side using 187c81f83011

 - NSS rejects PKSC#1 v1.5 signature in ServerKeyExchange message in TLS 1.2 when the server has a rsa-pss key (including sending the correct alert - illegal_parameter)

but the server side still has incorrect behaviour:

 - it doesn't ask for rsa_pss_pss_* signatures, just rsa_pss_rsae_*
 - when it receives rsa_pss_pss_sha256 signature from rsa-pss certificate it rejects it with decrypt_error instead illegal_parameter
 - same if a rsa-pss certificate is used to make rsa_pss_rsae_sha256 signature, rejected with decrypt_error instead of illegal_parameter

there may be more issues, I haven't updated test-rsa-pss-sigs-on-certificate-verify.py to draft-28 yet (https://github.com/tomato42/tlsfuzzer/issues/449)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OK, so we got the main one right.  There are probably some client certificate tests to add (and code to fix).

> it doesn't ask for rsa_pss_pss_* signatures, just rsa_pss_rsae_*

This is the default configuration.  You need to enable the pss_pss signature schemes with SSL_SignatureSchemePrefSet().  If you did call that function, then we have a serious problem.

> when it receives rsa_pss_pss_sha256 signature from rsa-pss certificate it rejects it with decrypt_error instead illegal_parameter
> [same for the reverse]

Seems simple enough to fix.  There are multiple paths, but I assume that you are talking about a PSS client certificate.  That code is here:

https://searchfox.org/nss/rev/397d9b45e399c0035d510d7af48b1aec38a6ec05/lib/ssl/ssl3con.c#9573

I found a bug with DH server key exchange when checking on this: We don't seem to be sending an alert at all (ECDH sends illegal_parameter):

https://searchfox.org/nss/rev/397d9b45e399c0035d510d7af48b1aec38a6ec05/lib/ssl/ssl3con.c#6966

Daiki, do you think that you could look at fixing these?  I could do this, but it would take a while.
Flags: needinfo?(dueno)
Thank you for the pointers; yes, I will look into them.
Flags: needinfo?(dueno)
> This is the default configuration.  You need to enable the pss_pss signature schemes with SSL_SignatureSchemePrefSet().  If you did call that function, then we have a serious problem.

I'm using tstclnt, strsclnt and selfserv, can I make them do that? I'm testing interoperability, so using gtests is not a good fit for it.
We do not have a switch for that.  Sorry.  Opened bug 1482747.
This fixes the corner cases where incorrect alert is sent (or even no alert is sent):

- when the client's CertificateVerify is signed with rsa_pss_pss_*,  while the certificate is RSA
- when the client's CertificateVerify is signed with rsa_pss_rsae_*, while the certificate is RSA-PSS
- when ServerKeyExchange is signed with an inconsistent signature scheme with the server certificate
Comment on attachment 8999893 [details]
Bug 1414931, send correct alert on inconsistent signature scheme

Martin Thomson [:mt:] has approved the revision.
Attachment #8999893 - Flags: review+
Pushed as:
https://hg.mozilla.org/projects/nss/rev/6349fa699c3b

Let's close this bug once we know bug 1482747 sufficiently cover the first issue in comment 8.

verified as fixed in nss 3.41.0

Status: REOPENED → RESOLVED
Closed: Last year6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.