Allow RSA signing with raw data (without a DigestInfo)

[jahkosha]

This fixes #3713 and #10226.

Instead of using the script written by @misterzed88 in #10226 to generate modified tests vectors, I did directly implement the same logic in the test infrastructure, so we can reuse directly the NIST (or others...) vectors stored in the tests.

[alex]

I haven't reviewed in depth -- but I don't think we should use None to signify this, I think we should have a dedicated sentinel, Raw<Something> perhaps. None looks like a reasonable default, but this is the opposite of that :-)

@reaperhulk wdyt?

[jahkosha]

@alex yeah that is a fair concern 👍 in some early attempt I did add a NoHash hashes instead of using None, but I was afraid that would cause even more confusion so eventually I went with None.

I'll be happy to apply what you folks think is the best, if possible, please provide pointers where in the code-base a similar pattern is used so I can take inspiration from it.

[misterzed88]

I haven't reviewed in depth -- but I don't think we should use None to signify this, I think we should have a dedicated sentinel, Raw<Something> perhaps. None looks like a reasonable default, but this is the opposite of that :-)

@reaperhulk wdyt?

I would like to highlight that the API already uses None for this purpose (for the RSA signature recover functionality, ref. issue #5495). So whatever you decide, you may want to use the same method in all the API functions to make them symmetric.

[reaperhulk]

Hmm, the inconsistency is a bit unfortunate. I'd be inclined to do a RawNoDigestInfo (or perhaps just NoDigestInfo?) sentinel here and then also add that as an option on the signature recovery method while retaining None for backwards compatibility. So this signature would look like sign(value, PKCS1v15(), RawNoDigestInfo()) or sign(value, PKCS1v15(), NoDigestInfo()).

[jahkosha]

@alex @reaperhulk I did add a sentinel NoDigestInfo and support it also on recover_data_from_signature

Please have an other look and let me know what you think

[reaperhulk]

We also need docs for this new value in RSAPrivateKey.sign

[reaperhulk]

I'd prefer we used SHA256 for the tests here because otherwise we'll increasingly have coverage challenges as more and more things disable SHA1.

[jahkosha]

The problem with that change is that the test vectors we use here (pkcs1v15sign-vectors.txt) do use SHA1.

[reaperhulk]

We don't need to use pkcs1v15sign-vectors.txt here, we can use SigVer15_186-3.rsp and filter to passing non-SHA1 tests.

[jahkosha]

Hey @reaperhulk, I finally had some time to have an other pass on this.

I could apply your feedback, there is just one comment I could not fix (SHA1 vs SHA256) because SHA1 is used by the test vectors themself (#13740 (comment)).

Please have a look at the last two commits and let me know if some more polish is needed.

Many thanks!

[reaperhulk]

Suggested change

	.. versionadded:: 47.0
	.. versionadded:: 47.0.0

[jahkosha]

Fixed and squashed in 405302f

[reaperhulk]

We don't need to use pkcs1v15sign-vectors.txt here, we can use SigVer15_186-3.rsp and filter to passing non-SHA1 tests.

[reaperhulk]

Oh, forgot one thing: we need a changelog entry in CHANGELOG.rst for this. Something like:

Added support for PKCS1v15 signing without DigestInfo using :class:~cryptography.path.to.NoDigestInfo.

[jahkosha]

@reaperhulk I gave a try at using SigVer15_186-3.rsp instead of pkcs1v15sign-vectors.txt, but the fields does not match between load_pkcs1_vectors and load_rsa_nist_vectors.

I could add the code to parse signature (S) and message (Msg), but I don't see in SigVer15_186-3.rsp some of the fields used to create the private key, namely: dmp1, dmq1 and iqmp.

Also I'm not sure how to differentiate private/public exponent... could you please give some guidance?

For reference, here is the field used by the test:

                private_key = rsa.RSAPrivateNumbers(
                    p=private["p"],
                    q=private["q"],
                    d=private["private_exponent"],
                    dmp1=private["dmp1"],
                    dmq1=private["dmq1"],
                    iqmp=private["iqmp"],
                    public_numbers=rsa.RSAPublicNumbers(
                        e=private["public_exponent"], n=private["modulus"]
                    ),

I looked in the rest of the codebase but I could not find any test that use SigVer15_186-3.rsp to build private keys.

[misterzed88]

I could add the code to parse signature (S) and message (Msg), but I don't see in SigVer15_186-3.rsp some of the fields used to create the private key, namely: dmp1, dmq1 and iqmp.

In a straightforward RSA implementation it would have been sufficient with the modulus n and the private exponent d for computations with the private key (as illustrated in the test script rsavec.py). There is also a more efficient computation, called the Chinese Reminder Theorem (CRT), which requires the parameters p, q, dmp1, dmq1 and iqmp.

The cryptography library always requires the CRT parameters to be available. Since they are not present in SigVer15_186-3.rsp they must be computed from n, e and d as follows:

p, q = rsa_recover_prime_factors(n, e, d)
dmp1 = rsa_crt_dmp1(d, p)
dmq1 = rsa_crt_dmq1(d, q)
iqmp = rsa_crt_iqmp(p, q)

It would of course be nice if the lib supported private RSA with only d being available, using it directly or with optional CRT precomputation support (for better efficiency in case the key is used multiple times). But that is a diffent story.

Perhaps the maintainers have more context or a better answer.

[jahkosha]

@misterzed88 thanks for your kind explanation, I was able to write code to recompute those values, and that was an opportunity to understand something new, much appreciated.

@reaperhulk sadly, even with the help from @misterzed88 I am not able to make it work, there is two things that I find particularly odd and concerning:

1. When I use rsa_recover_prime_factors (which fundamentally I don't need, it is there just for debugging) the value that I recover does not match the one defined in the test vectors, please have a look at those lines.

2. When I try to do generate the signature in the normal way (without the NoDigestInfo logic - again just for debbuging), the signature does not match. As you can see here.

This prevents me from writing the required tests, do you have any ideas what I'm doing wrong here? I'm just confused after doing those other tests... at this point, I'm not sure where could be the issue.

(I did fix the other points though)

[jahkosha]

@reaperhulk thanks for your guidance and patience!

I was able to address the tests related feedaback: e47bf81

I did also update the CHANGELOG as requested and add the proper version number in the documentation.

Ready for an other pass when you have time, thank you :)

[reaperhulk]

This was just intended as an example. You need to define the actual path to NoDigestInfo here. I believe it's in cryptography.hazmat.primitives.asymmetric.utils

[jahkosha]

Oops, I was so focused an having the other test working that I did not even read carefully!

Anyway, this is fixed in -> 5ee18d4

This time I did copy/pasted a line for Prehashed so it must be right.

[reaperhulk]

This is the only call site for this so I don't think we need this to be a function.

[jahkosha]

It is not the only call site, it is used for other tests thru: https://github.com/pyca/cryptography/pull/13740/files#diff-adca63e2e988b9f614480a194688af9467b6b54710a658d4aa958e48b2bc18c2R526

[reaperhulk]

This isn't necessary if we're only doing SHA256.

[jahkosha]

Fixed in 1f35994

[reaperhulk]

Almost there!

[jahkosha]

@reaperhulk done applying your last feedback!

The only thing that I did not do is inlining compute_rsa_hash_digest_sha256 because as I said it is used in two different places.

Please have an other look :-)

[reaperhulk]

Tests are failing only on the FIPS builders, almost certainly due to key size restrictions. You can either skip this test on FIPS or try adding a check for whether fips is enabled and skip if the modulus is < 2048-bit and see if that fixes it. You can see examples of both of these ideas in test_rsa.py