Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yubihsm-unwrap command #323

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

yubihsm-unwrap command #323

wants to merge 9 commits into from

Conversation

greg-szabo
Copy link

@greg-szabo greg-szabo commented Mar 14, 2023

This PR introduces the yubihsm-unwrap command, the other half of yubihsm-wrap. It allows encrypted key backups ("offline wraps") to be decrypted using the wrap key.

This is useful in cases where the object properties (domain, key ID, etc) have to be changed: yubihsm-unwrap removes the object properties and yubihsm-wrap can add different properties to the key.

It is also useful if someone needs to migrate away from YubiHSM to some other encryption method.

As with every security product, storing the private key on the local file system without encryption is highly discouraged. Keep your keys on the HSM device or if all else fails, keep them wrapped.

Note: I already added the Yubico licensing to the top of the files. I don't want to complicate licensing, so I expect the unwrap command should have the same licensing and ownership as the wrap command. I did it to make life easier for others and make the SDK look more complete. Feel free to send a CLA, if that helps.

yhunwrap/main.c Dismissed Show dismissed Hide dismissed
Copy link
Contributor

@qpernil qpernil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should probably call EVP_DecryptFinal_ex for completeness.
Also, maybe it would make sense to have an option where the output is formatted as a command line for yhwrap (so you can easily import again)

@greg-szabo
Copy link
Author

greg-szabo commented May 29, 2023

Hi @qpernil , thanks for your feedback on the code. At long last, I added EVP_DecryptFinal to the decryption. For AES-CCM it doesn't really have any benefit, as the OpenSSL example AES-CCM decryption code also mentions, finalization should not have any data for AES-CCM decryption.

I like the idea of an output formatted as a command-line input for yhwrap.

The only bottleneck is that I can't find any documentation where the delegated capabilities are stored in the object metadata. Without the delegated responsibilities, the command to reimport the key will not create an identical key to the one exported.

I'm open to advice: either we keep the current output and let the user convert how they see fit or we can create a partial input parameter list for yhwrap with the note that delegated capabilities have to be manually added. Or, if you can point me to something that documents how to get the delegated responsibilities, I'd be interested in implementing that too.

@greg-szabo
Copy link
Author

Hi @qpernil ,

I've run into another issue to fully recreate a yubihsm-wrap-compatible output. The yubihsm-wrap input is a PEM-encoded private key with some OID prefix, which is fine. But the yubihsm-unwrap output (the unwrapped key export) is the SHA512-hashed private key + public key. (64 bytes + 32 bytes)

I'm still looking at RFC8302 to see if I missed something. I'd be grateful for any advice on how to recreate the private key from the HSM export (unwrapped).

@tony-iqlusion
Copy link

@greg-szabo if it's really the "expanded secret key" there's no way to recover the original seed. It would require a preimage attack on SHA-512.

@greg-szabo
Copy link
Author

Thanks, Tony, that's what I derived too. I'll have to find an Ed25519 library that can use this "expanded private key" as input.

@qpernil it seems that with the above knowledge, making yubihsm-unwrap create the input for yubihsm-wrap is not possible because of the difference of the PEM-encoded private key required for yubihsm-wrap and the SHA512-hashed private key returned by yubihsm-unwrap.

Unless there is a way to extract the private key (or seed as Golang defines it) from the HSM device, there's no way to recreate the exact PEM input for yubihsm-wrap.

With that said, I think that the exercise of yubihsm-unwrap is still valuable and unless you have other feedback on what to improve, it is ready for review and merging.

@qpernil
Copy link
Contributor

qpernil commented Jun 8, 2023

The YubiHSM currently only stores the hashed seed, so yubihsm-wrap would have to be modified to allow an alternative input format that contains the hashed seed instead, and then skip the SHA512 hashing it currently performs.

@greg-szabo
Copy link
Author

Hi @qpernil ,

I agree that extending yubihsm-wrap with an option to read hashed private keys for wrapping is a neat idea and it should be done to complement yubihsm-unwrap. Especially, because the current PEM-based import option in yubihsm-wrap is fairly cumbersome.

However, should we maybe do that in a separate PR? It seems to me that it's worth separating that work from this one which focuses on the possibilities with yubihsm-unwrap.

What do you think?

@qpernil
Copy link
Contributor

qpernil commented Jun 28, 2023

Yes that could certainly be done in a separate PR. Please also note #351

@greg-szabo
Copy link
Author

Yes, please, and thank you for #351 !! It took me a bunch of time to troubleshoot why some of my keys don't work and some do. The only hint came from RFC8032 but I didn't put it together for a long time.

The byte changes in #351 are non-reversible so at exporting we can't really do much about it and it's not a problem. If someone takes the key and mangles it again, it will result in the same key.

The other quirk that took me some time to realize is that the YubiHSM device must be using little-endian byte order and that's why the wrap command reverses the first 32 bytes (lines 150-154 in the same file).

Maybe during unwrap, we should reverse the reverse order of these bytes so it's easier for the user to use the output. (If I want to validate the key in a test where I have the seed key, I can use a simple sha512sum command to generate the hash but that's in the regular order on linux.) You have to have this inherent knowledge that the expanded secret key is little-endian ordered or we can simply turn it back.

Shall I add a byte reversal code to unwrap for this? I think this is the last minor point that's worth fixing for this specific PR.

@qpernil
Copy link
Contributor

qpernil commented Jul 3, 2023

The fact that it reverses the bytes is simply how the specification for ed25519 is written, the low 32 bytes of the SHA512 hash are to be interpreted as a little-endian integer (https://www.rfc-editor.org/rfc/rfc8032 section 5.1.5 for example), and the bit clearing & setting is also part of the specification. So the result is in fact a big-endian integer. I would suggest just repeating these steps on any conventional (i.e. non-hashed) key and then testing that the result is the same.

@greg-szabo
Copy link
Author

This took longer than expected, but I implemented byte reversal as the last step. With this, the output is as close to a SHA512 sum value as possible.

I have created a seed key and imported it into an HSM device, then exported the expanded secret key with yubihsm-unwrap. Both the seed key and the exported key provided the same signature using the Ed25519 encryption.

Is there anything else to add to this PR?

@qpernil
Copy link
Contributor

qpernil commented Apr 2, 2024

In general the format of keys depends on the type of key, for example you should only reverse the first 32 bytes if it is an ed25519 key. And even for those it really depends on what the purpose is of the unwrapping. As we have noted before it is not currently possible to get back to the seed for ed25519 keys. If you want to check that the unwrapped key matches a seed you should instead process the seed as yubihsm-wrap does, and then compare to the decrypted key. Likewise, for authentication keys that were derived from a password you can't get back the password.

@qpernil
Copy link
Contributor

qpernil commented Apr 2, 2024

Also, you seem to have some errors in the reversing code (array bounds), see the failed checks below.

@greg-szabo
Copy link
Author

Thanks for pointing out the errors. I think it's because I used array notation (brackets) on a pointer. I do a size check before the error message, so it should have worked but I rewrote it to pointer arithmetic so the warning doesn't trigger.

I was narrowly focusing on ed25119 because of my use case, but now that you mention it, yubihsm-wrap can work on other types than asymmetric key.

So, maybe, it's a better purpose for this code to showcase how to decrypt encrypted keys using the wrapkey but not make any assumptions on what key is decrypted. Do you think that makes more sense?

In that case, I'll have to remove the key reversal to fulfill the purpose.

@greg-szabo
Copy link
Author

After some contemplation, I admit you are right. This example application could decrypt any exported key, but only Ed25519 keys must be byte-reversed. So, I removed the byte reversal from the codebase.

With that said, the purpose of this application is to provide an example of how to decrypt exported keys using AES-CCM. I think having this in the library is useful, at least for completeness' sake.

Golang and Rust have no language-native AES-CCM decryption libraries. (In Golang they considered it, and they have other encryption schemes natively supported.) This C implementation with openssl is closer to a safe solution than the third-party libraries in other languages.

I merged main into this branch which may have broken CI. I'll check it tomorrow.

@keefertaylor
Copy link

👋🏻 Howdy @greg-szabo! Thanks for implementing this, it saved me a ton of time and effort this weekend. I was successfully able to export and decrypt a secp256k1 key from the device!

I noticed a few rough edges as I played with this branch, and I've opened a PR to fix them here: greg-szabo#1. If we plan to merge this, I hope you'll consider including them!

@qpernil
Copy link
Contributor

qpernil commented Apr 15, 2024

I think this is a useful application, but it would perhaps be even more useful if the output was in some standard format other applications can use. yubihsm-wrap is an attempt to do something like that (but in the other direction), by taking PEM formatted keys such as those created by openssl. For your info there is an upcoming release of the YubiHSM that does store the ed25519 seed, and hence also returns it when exporting/wrapping keys. So you could basically mirror what yubihsm-wrap does but in the other direction. Just my .02 cents on what this application could be used for..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants