Add support for HPKE cryptographic operations #1118
Conversation
|
|
||
| static jlong NativeCrypto_EVP_HPKE_CTX_new(JNIEnv* env, jclass) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| const EVP_HPKE_CTX* ctx = EVP_HPKE_CTX_new(); |
There was a problem hiding this comment.
Strictly speaking, this can fail on malloc error. (Is Android's malloc fallible? Not actually sure.)
| /* context_len= */ exporterCtxLen)) { | ||
| if (exporterCtxArrayElement != nullptr) { | ||
| env->ReleaseByteArrayElements(exporterCtxArray, exporterCtxArrayElement, JNI_ABORT); | ||
| } |
There was a problem hiding this comment.
Optional: we can dedup a little code by doing something like:
bool ok = EVP_PKEY_CTX_export(...);
if (exporterCtxArrayElement != nullptr) {
env->ReleaseByteArrayElements(exporterCtxArray, exporterCtxArrayElement, JNI_ABORT);
}
if (!ok) {
....
}
Ditto for the other functions. (I don't suppose you all have C++ scopers for these? Might be handy.)
| size_t seed_len; | ||
| }; | ||
|
|
||
| static jbyteArray NativeCrypto_EVP_HPKE_CTX_setup_sender_with_seed(JNIEnv* env, jclass, |
There was a problem hiding this comment.
This is a testing-only interface and needs to retain the for_testing suffix to capture this.
Edit: I see you're using this outside testing. This is not supported and needs to be redesigned to not need this. This PR cannot land until this is fixed.
| // function "EVP_HPKE_CTX_setup_sender_with_seed_for_testing" using "kem->seed_len" | ||
| struct evp_hpke_kem_st { | ||
| size_t seed_len; | ||
| }; |
There was a problem hiding this comment.
Please remove this. Reaching into BoringSSL's internal structs like this is not supported. We make structs private for a reason. We can and will change the definition of these struct and won't make any accommodations for projects that do unsupported things like this. In fact, it's already the case that seed_len isn't first in the struct, so I'm not sure how this is working at all.
If you need an accessor, tell us and we can add the APIs. But I don't think it's necessary here. Seeds are not part of the official HPKE interface and this is only for testing anyway, it doesn't make sense to need it. Whatever you're getting the test vectors from can be assumed to have a priori knowledge of the seed length the particular KEM wants.
Indeed, since you've got a seedArray, it seems you should just be using env->GetArrayLength(seedArray), not kem->seed_len, which is actually an out-of-bounds read when seedArray didn't happen to be the right size.
| if (ctx == nullptr) { | ||
| conscrypt::jniutil::throwNullPointerException(env, "ctx == null"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Optional: All BoringSSL's free functions already handle NULL (by silently doing nothing, like C free), so you can safely remove this. But if you specifically want to throw NPE if someone passes null from Java, checking works.
| * | ||
| * @return IV creates a new array each time this method is called. | ||
| */ | ||
| public byte[] getIv() { |
There was a problem hiding this comment.
What is this about? HPKE does not take an IV for input, even for testing. The AEAD's IV is computed internally as part of the algorithm.
| if (!NativeCrypto.asn1_read_is_empty(seqRef) | ||
| || !NativeCrypto.asn1_read_is_empty(readRef)) { | ||
| throw new IOException("Error reading ASN.1 encoding"); | ||
| } |
There was a problem hiding this comment.
Where does this format come from? HPKE does not define any kind of ASN.1 encoding, and comping up with an ad-hoc one seems like it'd cause you problems in the future when this non-standard ad-hoc one turns out to be the wrong one.
| * | ||
| * @return API configured, either Secret Export or Encryption/Decryption. | ||
| */ | ||
| public boolean isExporting() { |
There was a problem hiding this comment.
What's this for? Exporting and encrypt/decrypt are not mutually-exclusive in HPKE. A single HPKE context is usable for encrypt/decrypt (depending on side) and export.
If you end up having to define multiple contexts, you'll have two problems:
- On the receiver side, you'll end up doing an extra decap operation (the expensive part) for each operation.
- On the sender side, the setup process is non-deterministic, so you won't even get the same context each time anyway! (And, obviously, BoringSSL will not support an API marked for testing being used in production. If we see such abuse, we'll unexport the testing API and keep it private to the library. It is only exported as a convenience for unit tests.)
| * @param pskId an identifier for the PSK | ||
| * @return builder | ||
| */ | ||
| public Builder modePskDecryption(byte[] enc, byte[] psk, byte[] pskId) { |
There was a problem hiding this comment.
We don't support the PSK mode. Having these APIs seems misleading.
| protected byte[] engineUpdate(byte[] input, int inputOffset, int inputLen) { | ||
| checkInitialization(); | ||
| if (exporting) { | ||
| final byte[] export = NativeCrypto.EVP_HPKE_CTX_export(evpCtx, input, exportLength); |
There was a problem hiding this comment.
Exporting is not a kind of encryption. This is definitely not the right API for HPKE.
|
The comments have been address and code has been refactored in its own APIs instead of jamming into Java CipherSpi. Closing this PR in favor of #1146 |
Adding support for HPKE cryptographic operations as per RFC-9180.html.
A new implementation of
CipherSpihas been added calledOpenSSLCipherHpkethat can perform the Single-Shot APIs for Encryption and Decryption as well as Secret Exports.The implementation delegates all the cryptographic functions to BoringSSL therefore only the algorithm identifiers and variants that are implemented on BoringSSL are supported. There is only one variant supported which is BASE_MODE (0x00). The algorithm identifiers supported are:
KEM (key encapsulation mechanism)
KDF (key derivation function)
AEAD (Authenticated Encryption with Associated Data)
The tests that verify algorithm correctness using the test vectors mentioned in the reference section can be found at
CipherBasicsTests