RSA Cipher Encrypt method may truncate leading byte from created Signature, if that byte is 00, causing server validation failure

[FreekSchoenmakersBuckaroo]

Hi gents,

In collaboration with ING Belgium and Buckaroo we have found a bug in the RSA Cipher Encrypt method, that can cause it to create an incorrect signature.

There's a couple interpretations for where you might consider the problem to be, but in essence what happens is that during the RSA 'Transform' method, the BigInteger is converted back to a ByteArray (and then reversed but that's not relevant). That would be this line here:

SSH.NET/src/Renci.SshNet/Security/Cryptography/Ciphers/RsaCipher.cs

Line 141 in 59840ec

return result.ToByteArray().Reverse();

However, this ByteArray does not always return a byte array of the required length. If the BigInteger is relatively small, then the returned ByteArray may be one byte shorter than what the signature length should be. This is of course a fairly rare occurence, leading to less than 1% failure rates.

This is an issue as the resulting signature violates RFC 8017 Section 8.2.2: https://datatracker.ietf.org/doc/html/rfc8017#:~:text=too%20short%22%0A%0A%20%20%20Steps%3A-,1.%20%20Length%20checking%3A%20If%20the%20length%20of%20the%20signature%20S%20is%20not%20k%0A%20%20%20%20%20%20%20%20%20%20octets%2C%20output%20%22invalid%20signature%22%20and%20stop.,-2.%20%20RSA%20verification

We find that this RFC is not always enforced, and this is likely why it has flown under the radar for quite some time (as this bug seems to exist in many older version as well, from testing at least until the 2020 version). An example of server software that does enforce this requirement is the Golang library (see https://github.com/golang/go/blob/cf058730293ac95ce0df40db8068219fe21cbb8a/src/crypto/rsa/pkcs1v15.go#L350 for the exact check that's performed).

An example of a possible argument for the data parameter that results in an invalid signature is the following byte array:
01-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-00-30-21-30-09-06-05-2B-0E-03-02-1A-05-00-04-14-76-FE-69-81-FF-B7-25-94-47-00-DC-9D-89-B5-9F-B5-F8-BA-70-F4

The DER encoded hash for this example was as follows: 30-21-30-09-06-05-2B-0E-03-02-1A-05-00-04-14-76-FE-69-81-FF-B7-25-94-47-00-DC-9D-89-B5-9F-B5-F8-BA-70-F4 (not sure what data exactly will be helpful in resolving this). This is the hash that's generated here:

SSH.NET/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs

Line 63 in 59840ec

var derEncodedHash = DerEncode(hashData);

These inputs encrypt to a byte array that is one byte smaller than the signature should be, causing the signature to be invalidated on the server. If we were to prepend the signature with a 00 byte, the signature is accepted. If we disable the check on the server that enforces RFC 8017, the signature is also accepted.

I believe the open PR #1373 may resolve this issue. I'm not sure if it's preferable that a temporary fix is added to this method for now, until that PR can be merged. It may be wise to add a simple unit test for this test case, to avoid regressions and to make sure that the linked PR actually resolves the problem.

If more info is required please let me know. If you want us to create a PR as a bandaid fix then I think we can do so as well, but given that there is a PR that should hopefully resolve this issue as well it may not be necessary. We could also provide a unit test instead if that would be useful to you.

[Rob-Hague]

Hi, thanks for the write-up

From my perspective #1373 is good to merge. I think it would be most useful if you could provide a unit test, either to be included in that PR or submitted as a follow-up. There are a few examples in test/Renci.SshNet.Tests/Classes/Security/Cryptography/RsaDigitalSignatureTest.cs (which is also modified slightly by the PR)

[FreekSchoenmakersBuckaroo]

@Rob-Hague Thank you for adding the regression test. I still had that on my ToDo list but it's very busy at our department at the moment so I hadn't gotten to it yet.

We're very grateful for your help with this issue!