Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
soter: Fix misuse of OpenSSL API for EC key generation (and more) (#875)
* sign/verify: Introduce EVP_PKEY into soter_sign_ctx_t Turns out, Soter generates EC keys incorrectly, by misusing OpenSSL API. It somehow works out for OpenSSL 1.1, but in OpenSSL 3.0 this fails. I suspect the issue stems from a misconception that EVP_PKEY_CTX is a container for generated keypair as EVP_PKEY. It is not. While it's true that EVP_PKEY_CTX_new() accepts an EVP_PKEY which can then be retrived using EVP_PKEY_CTX_get0_pkey() -- this is not a keypair. For EC keys this EVP_PKEY is "key parameters" (the curve, etc.) Generated keypair is output into a *different* EVP_PKEY, and it should be generated with a different function from that Soter uses. The same is mostly true for RSA as well. While it *happens so* that generated RSA key gets placed into EVP_PKEY_CTX, it should be a separate EVP_PKEY object. It works out only because RSA does not have parameters. Fixing this thing is going to be involved. The first bunch of patches only divorces Soter from the notion that EVP_PKEY_CTX is a container for keypairs and introduces EVP_PKEY fields for storing generated keypairs, independently from EVP_PKEY_CTX used to generate them. This patch in particular takes care of the sign/verify context. First we just add the field, initialize it constructors, and free it in destructors of soter_sign_ctx_t. * verify/ec: Retain imported EVP_PKEY Instead of throwing away the EVP_PKEY we have imported, keep it around in our context. EVP_DigestVerifyInit grabs its own reference. * verify/ec: Use EVP_PKEY directly Instead of retrieving the EVP_PKEY from EVP_PKEY_CTX, use the one we have retained in our context. * verify/ec: Do not retain EVP_PKEY_CTX In fact, don't construct it at all. We still have a field that we need to initialize. (Well, actually we don't since it's already NULL because the entire struct is allocated with calloc(), but hey, consistency.) * verify/ec: Drop unused cleanup label Now that EVP_PKEY_CTX is gone from there, we no longer need this label. * verify/ec: soter_sign_ctx_t initialized only once soter_verify_init_ecdsa_none_pkcs8() simply overwrites previous values of EVP_PKEY and EVP_PKEY_CTX with new ones, without freeing whatever was there. This is because it's expected to be called only one on a newly allocated instance of soter_sign_ctx_t. Make this expecatation explicit. * verify/ec: Check soter_sign_ctx_t for NULL Moar defensive coding here. All other functions check "ctx != NULL" and this one dereferences this pointer in the first line. * verify/ec: Cleaner check EVP_PKEY for NULL * verify/ec: Check other arguments for NULL/zero too As a couresy, mostly. OpenSSL will still fail with such values, but we can return SOTER_INVALID_PARAMETER instead. (Though, I belive that all upstream callers of these functions already check for these. If so, the compiler would be free to optimize those checks during inlining.) * verify/ec: Check EVP_PKEY types as well Unfortunately, soter_sign_ctx_t is reused for both ECDSA and RSA signatures, leading to a possibility of mismatch. I believe that upstream callers already check that keys used are consistent with signature type in Soter containers, but let's be double-sure. * verify/ec: Simplify EVP_DigestVerifyFinal() check Well, EC code path already does this correctly so we only simplify the code here. EVP_DigestVerifyFinal() returns 1 for successful signature check, 0 for failed signature check, or a negative value for "a more serious error". This switch handles it correctly, but we can simply check for 1. * verify/ec: Paranoid OpenSSL return value checks While EVP_DigestVerifyInit() and EVP_DigestVerifyUpdate() are not documented to return values other that 1 or 0, let's check for 1 specifically, consistent with EVP_DigestVerifyFinal() which *is* documented to return negative values for errors. * verify/rsa: Retain imported EVP_PKEY Same thing as in EC code paths, instead of throwing it away, retain the EVP_PKEY we have imported. The following commits too mirror the ones from the EC code path. * verify/rsa: Use EVP_PKEY directly * verify/rsa: Do not retain EVP_PKEY_CTX * verify/rsa: Drop unused cleanup label * verify/rsa: soter_sign_ctx_t initialized only once * verify/rsa: Check soter_sign_ctx_t for NULL * verify/rsa: Cleaner check EVP_PKEY for NULL * verify/rsa: Check other arguments for NULL/zero too * verify/rsa: Check EVP_PKEY types as well * verify/rsa: Correct EVP_DigestVerifyFinal() check Similarly to EC code path, check that EVP_DigestVerifyFinal() is successfuly correcly: by comparing the result to 1. Note that the current check is incorrect and might be a source of security issues, as it treats some unsuccessful results incorrectly as successful. * verify/rsa: Paranoid OpenSSL return value checks * sign/rsa: Retain imported EVP_PKEY Now dealing with the signing code path. This time starting with RSA because it's a bit easier. Retain that EVP_PKEY that we import from caller-provided key material, like we did for verify. But note that this does *not* strictly concern the generated key. (Though right now it's retained as well.) * sign/rsa: Return generated EVP_PKEY via out-parameter Instead of using EVP_PKEY_CTX as a container for EVP_PKEY to be filled in during key generation, accept an out-parameter for EVP_PKEY, following the signature of EVP_PKEY_keygen() which is actually used for generating the key (and gets passed the out-parameter now). Currently the NULL checks we perform on dereferenced EVP_PKEY do not make much sense, but I'm retaining them until a later point where they could be safely removed. * sign/rsa: Use EVP_PKEY directly Now that both key generation and importing place their EVP_PKEY into our context field directly, stop using the reference from EVP_PKEY_CTX. * sign/rsa: Local EVP_PKEY_CTX for keygen EVP_PKEY_CTX is used only for key generation, there is no need to retain it afterwards. Pull it out into a local variable. Note that this means we'd have to free it on both the success and failure code paths now. Previously, the destructor did it for us. * sign/rsa: Introduce "soter_status_t res" Start preparing the key generation function soter_rsa_gen_key() to handle *all* work related to generating RSA keys, instead of accepting a partially configured EVP_PKEY_CTX and EVP_PKEY and then finishing key generation. I'll need this function to handle resource management correctly so start introducing the "goto err" pattern by making sure that returned status code is passed via a variable. * sign/rsa: Introduce "err" label and code path Start building the error handling code path here by introducing an "err" label to which we'll jump on error. "pub_exp" needs a special treatment here since it's passed to the EVP_PKEY_CTX. So if we fail to pass it then we free it, but otherwise we must NOT free it again on future error handling. Thus reset it to NULL (which is safe to pass to BN_free()). * sign/rsa: Use "goto err" consistently Now that "goto err" path is in place, use that consistently instead of ad hoc calls to BN_free() and returns. * sign/rsa: Remove lying comments * sign/rsa: Initialize local variables * sign/rsa: Create EVP_PKEY_CTX only for key generation EVP_PKEY_CTX is used only during key generation, there is no need to retain it in soter_sign_ctx_t. Move EVP_PKEY_CTX allocation to soter_rsa_gen_key() which actually needs it. * sign/rsa: Drop unused cleanup label * sign/rsa: Create EVP_PKEY for importing separately Duplicate construction of EVP_PKEY for key generation and importing. In the following commit I'll move EVP_PKEY construction for generation into soter_rsa_gen_key() itself, since it already returns it via an out-parameter and can allocate a new EVP_PKEY all by itself. * sign/rsa: Move EVP_PKEY allocation to key generation Since we generate a new key, might as well allocate it there too. We immediately assign the allocated key to ppkey and the caller will free that for us if we fail. That's also why we can also return before we allocate EVP_PKEY_CTX and don't need to "goto err". (Though, both these points are moot since these lines are going away soon.) * sign/rsa: Use EVP_PKEY_CTX_new_id() for key generation If you read OpenSSL documentation *very carefully*, you'd see that the correct way to generate keys is to start with EVP_PKEY_CTX allocated by EVP_PKEY_CTX_new_id() with the key type you want to generate. The sequence we do here is technically incorrect and just happens to sorta-kinda work out for RSA because of the implementation quirks. The other form -- EVP_PKEY_CTX_new() -- takes EVP_PKEY with "key parameters", which there are none for RSA. This will be important for EC key generation later, but not here. I'll rant about it when I will be dealing with EC code paths. * sign/rsa: soter_sign_ctx_t initialized only once Just like with signature verification, there is an expectation that soter_sign_init_rsa_pss_pkcs8() is called only once on a newly allocated soter_sign_ctx_t. Make this expectation explicit. * sign/rsa: Use "goto err" consistently * sign/rsa: Replace local variable with direct use * sign/rsa: Move local variable declaration * sign/rsa: Check soter_sign_ctx_t for NULL * sign/rsa: Check other arguments for NULL/zero too Similar to the verify code path, do some courtesy checks of arguments for obviously wrong values. Note that for soter_sign_final_rsa_pss_pkcs8() the "signature" argument is actually optional and passing NULL there provides a size measurement. EC code path has this bit, but RSA did not handle it (no idea why, lol). * sign/rsa: Check EVP_PKEY types as well And again, similar to the verify code path, check the types of EVP_PKEY in our soter_sign_ctx_t which is used by both RSA and ECDSA signatures. We don't want mismatches to happen, the context must have been initialized for RSA signature computation. * sign/rsa: Check EVP_PKEY_size() correctly It returns zero for error and actually never returns negative values. BoringSSL even made it return "size_t" for real, removing the need for a cast later. (But OpenSSL keeps it "int", even in 3.0.) * sign/rsa: Pass EVP_PKEY directly to soter_rsa_export_key() Consistent with soter_rsa_import_key(), if anything, and makes these functions completely independent of soter_sign_ctx_t. In fact, if you look around, there are already functions for exporting, importing, and even generating RSA keys. However, right now I'm more focused on getting sign/verify code paths straight. Deduplication will come later. * sign/rsa: Correct OpenSSL return value checks You remember, right? Almost all OpenSSL functions that return "int" status code return 1 for success, 0 for errors, and negative values for even more errors. Checking return values with "!" can treat these extra errors as false positives. Update all functions that we call here, even if some of them are not documented to return negative values. * sign/ec: Retain imported EVP_PKEY A-a-and the same thing we did to RSA we need to do for EC as well. Start by retaining EVP_PKEY in soter_sign_ctx_t. * sign/ec: Return generated EVP_PKEY via out-parameter * sign/ec: Use EVP_PKEY directly * sign/ec: Local EVP_PKEY_CTX for keygen * sign/ec: Introduce "soter_status_t res" Yep, doing the same refactoring as with RSA's key generation function. It will need to allocate and manage multiple resources, using proper cleanup patterns is essential here. * sign/ec: Use "goto err" consistently This patch is a bit underwhelming, but only because all preceding checks are basically "parameter checks" and they will go away soon. * sign/ec: Initialize local variables * sign/ec: Create EVP_PKEY_CTX only for key generation Similar to RSA, EVP_PKEY_CTX is used only for key generation. Move EVP_PKEY_CTX construction and parameter setting over to soter_ec_gen_key() which should deal with all key generation. * sign/ec: Drop unused cleanup label * sign/ec: Create EVP_PKEY for importing separately Similarly to RSA, I'm going to move EVP_PKEY construction completely into soter_ec_gen_key() when we generate keys. However, importing still needs to have a blank EVP_PKEY prepared for it. * sign/ec: Move EVP_PKEY allocation to key generation * sign/ec: Drop redundant EVP_PKEY type check We just set the type to be EVP_PKEY_EC, no need to be paranoid *here*. * sign/ec: Use EVP_PKEY_keygen() for key generation And now start reading very carefully. TTTTT H H III SSS III SSS V V III TTTTT A L T H H I S I S V V I T A A L T HHHHH I SSS I SSS V V I T AAAAA L T H H I S I S V V I T A A L T H H III SSS III SSS V III T A A LLLLL The following several commits are the reason for this entire giant patchset in the first place. This is how you generate EC keys with OpenSSL. Not how they were generated before, but in this way. Previous way sorta-kinda works with OpenSSL 1.1 but it got broken in OpenSSL 3.0 and OpenSSL developers have confirmed that it should have never worked in the first place, but it did, and people copied and pasted incorrect snippets all around, and that's how this misconception got born. Because of course OpenSSL 1.1 documentation provides zero examples of how to generate EC keys. OpenSSL 3.0 has improved. A bit. First of all, there is a dedicated function for generating the keypair. The context also needs to be initialized for key generation first. (Actually, you need *a different* EVP_PKEY_CTX for this, but that's going to be handled in the following commits.) * sign/ec: Use separate EVP_PKEY_CTX for EVP_PKEY_paramgen() Or rather, make a new one for EVP_PKEY_keygen(). *This* is when you should use EVP_PKEY_CTX_new(), when the EVP_PKEY contains "key parameters". This is the whole source of confusion with EC key generation: you need to first generate key parameters and then the keypair itself. * sign/ec: Key parameters use distinct EVP_PKEY So, EC key generation uses two distinct EVP_PKEY objects. First one is used to hold "parameters" (i.e., the curve), and the second one holds the actual keypair. Don't ask me why the hell EVP_PKEY is used to hold parameters, go complain to OpenSSL maintainers. Rename the EVP_PKEY we keep to "param", *don't* store it it output, and pass it only to EVP_PKEY_paramgen(). * sign/ec: Use EVP_PKEY_CTX_new_id() for parameter generation Also, you should use EVP_PKEY_CTX_new_id() to allocate EVP_PKEY_CTX for parameter generation. *Not* use an empty EVP_PKEY with only the type set. This works in OpenSSL 1.1 by accident but this is not how parameter generation is supposed to be performed. (And OpenSSL 3.0 breaks EVP_PKEY_CTX_new() usage for EVP_PKEY_paramgen() completely.) With this change, sign/verify tests finally pass with OpenSSL 3.0, which is the goal of the patchset. The following commits are mopping up and filling in some blanks. * sign/ec: soter_sign_ctx_t initialized only once * sign/ec: Use "goto err" consistently * sign/ec: Replace local variable with direct use * sign/ec: Return resulting status code directly We don't manage resources in this function, the status can be returned directly (this is what we do in RSA code path as well). Note that the status code is different here, since RSA returns SOTER_FAIL while ECDSA returns SOTER_INVALID_SIGNATURE. Keep returning whatever we have been returning in this case. "Eventually, every observable behavior of your system will be depended upon by someone." * sign/ec: Pull out a local variable Don't call EVP_PKEY_size() multiple times, just cache its value. (And also check it for validity before casting into unsigned size_t, like we do in the RSA code path.) The TODO is now DONE, I think. EVP_PKEY_size() returns *maximum* size of various things that can be placed into output buffers by operations with this EVP_PKEY. In our case, that's the signature size. * sign/ec: Flatten "if-else" level No need to make an extra indentation level if the "if" branch always returns. Make the code consistent with RSA code path. * sign/ec: Check soter_sign_ctx_t for NULL * sign/ec: Check other arguments for NULL/zero too Same as with RSA (and verify code paths), make some courtesy checks. The "signature" argument of soter_sign_final_ecdsa_none_pkcs8() is optional, used to return the expected signature output size. * sign/ec: Check EVP_PKEY types as well Like with RSA, check that the key type is consistent with the signature type we are about to make, preventing soter_sign_ctx_t mismatch. Curiously, EC signining code path already included these checks for some functions. Those actually prompted me to add them everywhere. * sign/ec: Check EVP_PKEY_size() correctly * sign/ec: Pass EVP_PKEY directly to soter_ec_export_key() * sign/ec: Correct OpenSSL return value checks Yes, yes, here too some of those function can return negative values which we currently treat as success. Don't do that, okay? * sign/verify: Drop EVP_PKEY_CTX from soter_sign_ctx_t Now finally this value is never used by any of the sign/verify functions, completely replaced by EVP_PKEY, as it should be. * changelog: Jot down a note about improvements Don't tell anything about OpenSSL 3.0 compatibility yet, it's just some refactoring to improve error handling (and hopefully without any accidental vulnerabilities introduced). * CI: Work around legacy Node.js 8.x issues As described in referenced issue, Node.js 8.x tends to cause crashes because it embeds OpenSSL 1.0.2 when Themis has been built against OpenSSL 1.1.1. It seems before all these changes for OpenSSL 3.0 compatibility we somehow avoided calling any APIs that have actually broken binary compatiblity between 1.0.2 and 1.1.1, but now we do. The documented workaround is to use BoringSSL build so BoringSSL we use. That's going to check this workaround in the first place, lol.
- Loading branch information
