RSAOpenSsl seems to allow inconsistent values to ImportParameters

[bartonjs]

When the Modulus and Exponent are from one key, and the private parameters are from a key of a completely different size, the import shouldn't succeed, but does.

The key is in an obviously inconsistent state at that point, maybe it's deferring the check to first use... but we should lift it up to fail the import (and have parity with the other OSes).

[bartonjs]

Given that this isn't new in 3.0 I'm moving to Future (PRs while 3.0 is still open are still welcome).

[vcsjones]

@bartonjs One way to address this is with RSA_check_key after importing private parameters. I did a little PoC over here master...vcsjones:fix-37595 which fixes the test. Is something like that what you had in mind?

[bartonjs]

Well, sort of. The problem is (last I checked) RSA_check_key directly checks the members of a default-engine software key. If it's possible to change the engine used by RSA_new then RSA_check_key would fail after import.

If that is always hard-coded to be the default engine, no worries. (And if we can determine that the engine isn't default, but we can forcibly use the software engine as a temporary engine during import, that's fine, too)

[vcsjones]

The problem is (last I checked) RSA_check_key directly checks the members of a default-engine software key.

@bartonjs Yes, that is the case. Someone could do ENGINE_set_default_RSA in-process and the current implementation would follow that, (or maybe they are hosting the CLR in a process that already changed the default engine) if that is a valid concern.

I think we have three choices.

1. If private parameters are included, create a new RSA using the "build-in" engine and validate the parameters there.

2. Check the engine and don't do anything if it's not the default engine. We can get the engine with RSA_get0_engine. This is new in OpenSSL 1.1.0, but we can shim it in apibridge.c as rsa->engine for older openssls, though the shim architecture is not something I am familiar with.

3. You have a better idea entirely. 😀

I can do either 1 or 2, two feels like more work to me but perhaps more "correct" since one might reject parameters that the engine is otherwise fine with using.

As an aside, EVP_PKEY_check "fixes" this by moving the check functionality to method table, allowing the engine to specify a routine for validating the key, but it only available in OpenSSL 1.1.1 or greater.

[bartonjs]

(1) seems like the easiest short term general solution (a temporary default-engine key to import and check, then if that passes import into the provided key).

[vcsjones]

@bartonjs I pushed up some changes at master...vcsjones:fix-37595. This is not complete (more tests, etc), but wanted to check a few things before I continue.

1. Am I using RENAMED_FUNCTION correctly here?
2. Does .NET Core support FIPS builds of OpenSSL? (I assume yes). I ask because I'm not confident FIPS is handled correctly yet. How can I point .NET Core to a local build of OpenSSL? Do any of the CI legs have a FIPS build of OpenSSL?
3. Is it more preferable for CryptoNative_SetRsaParameters to call the validation function, or should the validation function itself be an exported CryptoNative_ function and called from managed code before it even gets to CryptoNative_SetRsaParameters?

Sorry for the big wall of questions, thanks for bearing with me.

[bartonjs]

1. Looks like it.
2. Hopefully? 😄. I think RHELs are all FIPS builds, so "yes". LD_PRELOAD should work, at least if you build non-portable (might need both LD_PRELOAD and CLR_OPENSSL_VERSION_OVERRIDE if it's portable, 1.1 is available, but you're trying to force 1.0 FIPS).
3. I like it as a detail of SetRsaParameters; that's the only time we'd really want it.

[vcsjones]

@bartonjs to summarize some things I learned..

• Each platform does everything a bit differently. Adding RSA_check_key does allow us to check the key at import time, but now it is more strict that Windows and macOS. For example, If PQ != N, Windows will happily recompute the right N from PQ. OpenSSL's check key on the other hand checks that. We could tackle this similarly as ECDsa.Create(ECParameters) behavior is not consistent across platforms with a public/private key mismatch #27830. We could have Windows export the key again and compare parameters. If the import process "fixed" something, we fail so the platform behavior is consistent.

• Different platforms have different checks for safety of the key. RSA_check_key and macOS will stop you from using an RSA key with a public exponent that is one or even. CNG appears to be happy to use anything for a public exponent. While macOS 10.13 blocks RSA keys with E=1, it does not perform a primality test on P or Q. Just some examples of interesting checks in behavior.

• It's also worth being clear that this is technically a breaking change. This isn't just moving the error to import time, this is also blocking RSA keys with bad properties. I think Core should block using such keys, but still nonetheless, a change that will break people using broken RSA keys.

Given all of that..

1. How much do you care about tests for "unsafe" keys? That is, should we be testing keys with known weak properties? If it's a feature we want to bring to all platforms, I would then say we add the tests that and add an active issue for platforms where this is not working. If this is just a "bonus" behavior of openssl then I can either omit the tests or make them platform specific.

2. I would propose we do this as a few smaller PRs for different platforms.

[bartonjs]

• Failing if Windows changed N makes sense (because it's not related to the public key you told everyone it was). The other parameters aren't as inviolate, CNG Windows 7, 8(.1), and 10 TH1-RS..3? all ignore D/DP/DQ/QInv and recompute D-lambda (which is smaller than D-phi) and the smaller CRT parameters. RS5 (and maybe 4) at least exports using D-phi instead of D-lambda, so that the keys looked more like they would have from CAPI (which used D-phi).
• Failing on E=1 makes sense. I'm not sure what CNG would even do with that, and it certainly can't recompute D (there is no value that is the multiplicative inverse of E=1 that isn't 1); seems like maybe a deferred-use problem?
• Failing on non-prime P or Q is less obvious. It's good for the platforms that do check it, but I'm wary of situations where we start doing math with the parameters.

For both this issue and 33278, we might need to dual-import into Windows; once for parameter consistency and once for real... I'm not sure if exporting the key calls (internally) NCryptFinalizeKey, meaning that you can't import and then change properties (like the export policy) that can't be altered after finalize... of course, that only matters for the publicly CNG types (RSACng, ECDsaCng)... the ephemeral key opaque types from RSA.Create() or ECDsa.Create() can do the fast path, since there's no way to get the NCRYPT_KEY_HANDLE back.

[vcsjones]

@bartonjs question:

Does CNG support public exponents larger than UInt32.MaxValue? As far as I can tell, "Microsoft Software Key Storage Provider" will not accept an E larger than that, but I am not sure if that is a limitation of that particular CNG provider and other CNG providers could handle more, or if CNG imposes a real upper bound on it.

[bartonjs]

As far as I can tell, "Microsoft Software Key Storage Provider" will not accept an E larger than that

Yep. https://github.com/dotnet/corefx/blob/d3911035f2ba3eb5c44310342cc1d654e42aa316/src/System.Security.Cryptography.X509Certificates/tests/TestData.cs#L1558 is the key I habitually use for testing that.

https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/RSAPkcs1X509SignatureGeneratorTests.cs#L20-L57

I feel like they broke it at some point in Win10 (brought back the 32-bit limit from CAPI because of a perf improvement), but then made it work again (but the limit now might be a 64-bit value for E).

[vcsjones]

@bartonjs thinking about where to take this issue next, the original issue from the title was addressed by dotnet/corefx#38884.

There are a number of other platform inconsistencies. I'm not sure if you want to open new issues for those, or make this one a bit more generic, "Inconsistent validation of RSA parameters at import time across platforms" or something.

1. macOS still has the original test in question disabled.
2. Windows appears to inconsistently throw errors between import and first-use for some parameters.
3. Windows will re-calculate N from PQ while openssl will throw for mismatch.

[bartonjs]

@vcsjones I guess it depends on if you are wanting to tackle them or we're leaving them as open issues.

If you're wanting to provide fixes, I'm fine using this issue as an uber-issue for making parameter import consistent across platforms (or breaking it up into smaller issues, whichever makes you happier).

If we're leaving it as open, it probably makes more sense to open specific issues. (Ideally with a "this type of problem, for example [some parameters]", but perhaps just a "this type of problem... parameter generation left as an exercise to the reader".)

[vcsjones]

If you're wanting to provide fixes

Yes, just been a little slow organizing things. Leaving it as a single issue appeals to my sense of laziness, so I'll just use this issue.