-
Notifications
You must be signed in to change notification settings - Fork 319
Basic TLS Encrypted ClientHello (ECH) support (native layer) #1374
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
Changes from 6 commits
37b0729
c54c80c
0b033db
e88b5c9
32f5387
df0a9e9
82b5bad
171e31e
49d0dc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,16 @@ static SSL_CIPHER* to_SSL_CIPHER(JNIEnv* env, jlong ssl_cipher_address, bool thr | |
| return ssl_cipher; | ||
| } | ||
|
|
||
| static SSL_ECH_KEYS* to_SSL_ECH_KEYS(JNIEnv* env, jlong ssl_ech_keys_address, bool throwIfNull) { | ||
| SSL_ECH_KEYS* ssl_ech_keys = | ||
| reinterpret_cast<SSL_ECH_KEYS*>(static_cast<uintptr_t>(ssl_ech_keys_address)); | ||
| if ((ssl_ech_keys == nullptr) && throwIfNull) { | ||
| JNI_TRACE("ssl_ech_keys == null"); | ||
| conscrypt::jniutil::throwNullPointerException(env, "ssl_ech_keys == null"); | ||
| } | ||
| return ssl_ech_keys; | ||
| } | ||
|
|
||
| template <typename T> | ||
| static T* fromContextObject(JNIEnv* env, jobject contextObject) { | ||
| if (contextObject == nullptr) { | ||
|
|
@@ -1157,19 +1167,30 @@ static jint NativeCrypto_EVP_PKEY_cmp(JNIEnv* env, jclass, jobject pkey1Ref, job | |
| JNI_TRACE("EVP_PKEY_cmp(%p, %p)", pkey1Ref, pkey2Ref); | ||
| EVP_PKEY* pkey1 = fromContextObject<EVP_PKEY>(env, pkey1Ref); | ||
| if (pkey1 == nullptr) { | ||
| conscrypt::jniutil::throwNullPointerException(env, "Null pointer, key 1"); | ||
| ERR_clear_error(); | ||
| JNI_TRACE("EVP_PKEY_cmp => pkey1 == null"); | ||
| return 0; | ||
| } | ||
| EVP_PKEY* pkey2 = fromContextObject<EVP_PKEY>(env, pkey2Ref); | ||
| if (pkey2 == nullptr) { | ||
| conscrypt::jniutil::throwNullPointerException(env, "Null pointer, key 2"); | ||
| ERR_clear_error(); | ||
| JNI_TRACE("EVP_PKEY_cmp => pkey2 == null"); | ||
| return 0; | ||
| } | ||
| JNI_TRACE("EVP_PKEY_cmp(%p, %p) <- ptr", pkey1, pkey2); | ||
|
|
||
| int result = EVP_PKEY_cmp(pkey1, pkey2); | ||
| JNI_TRACE("EVP_PKEY_cmp(%p, %p) => %d", pkey1, pkey2, result); | ||
| return result; | ||
| if (result < 0) { | ||
| conscrypt::jniutil::throwInvalidKeyException(env, "Decoding error"); | ||
| ERR_clear_error(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by comment: If you're going to throw a checked exception, you should reflect that in the Java method signature and add a regression test for it. Same for the parsing exception below.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, but more generally let's not change the non-ECH part of the code in this pull request. If you wanted to, please follow up with a new PR. Could you please revert the changes here that are not related to ECH? Thanks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up making these changes because one of the failing tests was the one that tested this method: Could this just be failing locally for some reason? I suppose I can revert it and see what happens with the CI tests. (UPDATE) Commented this part out for now and pushed it. If there's no CI failures I'll pull out all the extra stuff and we can wrap this up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unfortunately it looks like EVP_PKEY_cmp_BothNullParameters still fails (and test_get_RSA_public_params, which doesn't fail locally). I assume this isn't failing elsewhere, but I'm not sure how my changes could have caused this. Could the failure be caused by errors placed in the queue by a prior test? I notice that the failing test is right after one of the new ECH tests.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I think your hunch is correct. If you look at the error in the CI, the complete error is: Note how the error queue is populated in |
||
| JNI_TRACE("VP_PKEY_cmp(%p, %p) => threw exception", pkey1, pkey2); | ||
| return result; | ||
| } else { | ||
| JNI_TRACE("EVP_PKEY_cmp(%p, %p) => %d", pkey1, pkey2, result); | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| /* | ||
|
|
@@ -7786,7 +7807,12 @@ static int sslSelect(JNIEnv* env, int type, jobject fdObject, AppData* appData, | |
| if (fds[1].revents & POLLIN) { | ||
| char token; | ||
| do { | ||
| (void)read(appData->fdsEmergency[0], &token, 1); | ||
| // TEMP - fixes build error | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you get a compiler warning here otherwise?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one was more specific: First I tried assigning the result to a variable, but then it complained that the variable was unused and that's how I ended up with the current fix. There's about 4 places where this happens but this was the only one in a file I had to commit. |
||
| int foo = 0; | ||
| foo = read(appData->fdsEmergency[0], &token, 1); | ||
| if (foo > 0) { | ||
| CONSCRYPT_LOG_VERBOSE("FOO: %d", foo); | ||
| } | ||
| } while (errno == EINTR); | ||
| } | ||
| } | ||
|
|
@@ -7817,7 +7843,12 @@ static void sslNotify(AppData* appData) { | |
| char token = '*'; | ||
| do { | ||
| errno = 0; | ||
| (void)write(appData->fdsEmergency[1], &token, 1); | ||
| // TEMP - fixes build error | ||
| int foo = 0; | ||
| foo = write(appData->fdsEmergency[1], &token, 1); | ||
| if (foo > 0) { | ||
| CONSCRYPT_LOG_VERBOSE("FOO: %d", foo); | ||
| } | ||
| } while (errno == EINTR); | ||
| errno = errnoBackup; | ||
| #endif | ||
|
|
@@ -8269,6 +8300,7 @@ static jlong NativeCrypto_SSL_CTX_new(JNIEnv* env, jclass) { | |
| bssl::UniquePtr<SSL_CTX> sslCtx(SSL_CTX_new(TLS_with_buffers_method())); | ||
| if (sslCtx.get() == nullptr) { | ||
| conscrypt::jniutil::throwExceptionFromBoringSSLError(env, "SSL_CTX_new"); | ||
| ERR_clear_error(); | ||
| return 0; | ||
| } | ||
| SSL_CTX_set_options( | ||
|
|
@@ -11776,6 +11808,191 @@ static jlong NativeCrypto_SSL_get1_session(JNIEnv* env, jclass, jlong ssl_addres | |
| return reinterpret_cast<uintptr_t>(SSL_get1_session(ssl)); | ||
| } | ||
|
|
||
| static void NativeCrypto_SSL_set_enable_ech_grease(JNIEnv* env, jclass, jlong ssl_address, | ||
| CONSCRYPT_UNUSED jobject ssl_holder, | ||
| jboolean enable) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL* ssl = to_SSL(env, ssl_address, true); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_set_enable_ech_grease(%d)", ssl, enable); | ||
| if (ssl == nullptr) { | ||
| return; | ||
| } | ||
| SSL_set_enable_ech_grease(ssl, enable ? 1 : 0); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_set_enable_ech_grease(%d) => success", ssl, enable); | ||
| } | ||
|
|
||
| static jboolean NativeCrypto_SSL_set1_ech_config_list(JNIEnv* env, jclass, jlong ssl_address, | ||
| CONSCRYPT_UNUSED jobject ssl_holder, | ||
| jbyteArray configJavaBytes) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL* ssl = to_SSL(env, ssl_address, true); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p)", ssl, configJavaBytes); | ||
| if (ssl == nullptr) { | ||
| conscrypt::jniutil::throwNullPointerException(env, "Null pointer, ssl address"); | ||
| ERR_clear_error(); | ||
| return JNI_FALSE; | ||
| } | ||
| ScopedByteArrayRO configBytes(env, configJavaBytes); | ||
| if (configBytes.get() == nullptr) { | ||
| conscrypt::jniutil::throwNullPointerException(env, "Null pointer, ech config"); | ||
| ERR_clear_error(); | ||
| JNI_TRACE("NativeCrypto_SSL_set1_ech_config_list => could not read config bytes"); | ||
| return JNI_FALSE; | ||
| } | ||
| int ret = SSL_set1_ech_config_list(ssl, reinterpret_cast<const uint8_t*>(configBytes.get()), | ||
| configBytes.size()); | ||
| if (!ret) { | ||
| conscrypt::jniutil::throwParsingException(env, "Error parsing ECH config"); | ||
| ERR_clear_error(); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p) => threw exception", ssl, configJavaBytes); | ||
| return JNI_FALSE; | ||
| } else { | ||
|
mnbogner marked this conversation as resolved.
Outdated
|
||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p) => %d", ssl, configJavaBytes, ret); | ||
| return ret; | ||
| } | ||
| } | ||
|
|
||
| static jstring NativeCrypto_SSL_get0_ech_name_override(JNIEnv* env, jclass, jlong ssl_address, | ||
| CONSCRYPT_UNUSED jobject ssl_holder) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL* ssl = to_SSL(env, ssl_address, true); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_name_override()", ssl); | ||
| if (ssl == nullptr) { | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_name_override() => nullptr", ssl); | ||
| return nullptr; | ||
| } | ||
| const char* ech_name_override; | ||
| size_t ech_name_override_len; | ||
| SSL_get0_ech_name_override(ssl, &ech_name_override, &ech_name_override_len); | ||
| if (ech_name_override_len > 0) { | ||
| jstring name = env->NewStringUTF(ech_name_override); | ||
| return name; | ||
| } | ||
| return nullptr; | ||
| } | ||
|
|
||
| static jbyteArray NativeCrypto_SSL_get0_ech_retry_configs(JNIEnv* env, jclass, jlong ssl_address, | ||
| CONSCRYPT_UNUSED jobject ssl_holder) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL* ssl = to_SSL(env, ssl_address, true); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_retry_configs()", ssl); | ||
| if (ssl == nullptr) { | ||
| return nullptr; | ||
| } | ||
| const uint8_t* retry_configs; | ||
| size_t retry_configs_len; | ||
| SSL_get0_ech_retry_configs(ssl, &retry_configs, &retry_configs_len); | ||
| if (retry_configs_len <= 0) { | ||
| return nullptr; | ||
| } | ||
| jbyteArray result = env->NewByteArray(static_cast<jsize>(retry_configs_len)); | ||
|
mnbogner marked this conversation as resolved.
|
||
| if (result == nullptr) { | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_retry_configs() => creating byte array failed", | ||
| ssl); | ||
| return nullptr; | ||
| } | ||
| env->SetByteArrayRegion(result, 0, static_cast<jsize>(retry_configs_len), | ||
| reinterpret_cast<const jbyte*>(retry_configs)); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_retry_configs() => %p", ssl, result); | ||
| return result; | ||
| } | ||
|
|
||
| static jlong NativeCrypto_SSL_ECH_KEYS_new(JNIEnv* env, jclass) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| bssl::UniquePtr<SSL_ECH_KEYS> sslEchKeys(SSL_ECH_KEYS_new()); | ||
| if (sslEchKeys.get() == nullptr) { | ||
| conscrypt::jniutil::throwExceptionFromBoringSSLError(env, "SSL_ECH_KEYS_new"); | ||
| return 0; | ||
| } | ||
| JNI_TRACE("NativeCrypto_SSL_ECH_KEYS_new => %p", sslEchKeys.get()); | ||
| return (jlong)sslEchKeys.release(); | ||
| } | ||
|
|
||
| static void NativeCrypto_SSL_ECH_KEYS_up_ref(JNIEnv* env, jclass, jlong ssl_ech_keys_address) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL_ECH_KEYS* ssl_ech_keys = to_SSL_ECH_KEYS(env, ssl_ech_keys_address, true); | ||
| JNI_TRACE("ssl_ech_keys=%p NativeCrypto_SSL_ECH_KEYS_up_ref", ssl_ech_keys); | ||
| if (ssl_ech_keys == nullptr) { | ||
| return; | ||
| } | ||
| SSL_ECH_KEYS_up_ref(ssl_ech_keys); | ||
| } | ||
|
|
||
| static void NativeCrypto_SSL_ECH_KEYS_free(JNIEnv* env, jclass, jlong ssl_ech_keys_address) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL_ECH_KEYS* ssl_ech_keys = to_SSL_ECH_KEYS(env, ssl_ech_keys_address, true); | ||
| JNI_TRACE("ssl_ech_keys=%p NativeCrypto_SSL_ECH_KEYS_free", ssl_ech_keys); | ||
| if (ssl_ech_keys == nullptr) { | ||
| return; | ||
| } | ||
| SSL_ECH_KEYS_free(ssl_ech_keys); | ||
| } | ||
|
|
||
| static jboolean NativeCrypto_SSL_ech_accepted(JNIEnv* env, jclass, jlong ssl_address, | ||
| CONSCRYPT_UNUSED jobject ssl_holder) { | ||
| JNI_TRACE("NativeCrypto_SSL_ech_accepted"); | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL* ssl = to_SSL(env, ssl_address, true); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted", ssl); | ||
| if (ssl == nullptr) { | ||
| conscrypt::jniutil::throwNullPointerException(env, "Null pointer, ssl address"); | ||
| ERR_clear_error(); | ||
| return JNI_FALSE; | ||
| } | ||
| jboolean accepted = SSL_ech_accepted(ssl); | ||
|
|
||
| if (!accepted) { | ||
| conscrypt::jniutil::throwParsingException(env, "Invalid ECH config list"); | ||
| ERR_clear_error(); | ||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted => threw exception", ssl); | ||
| return JNI_FALSE; | ||
| } else { | ||
|
mnbogner marked this conversation as resolved.
Outdated
|
||
| JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted => %d", ssl, accepted); | ||
| return accepted; | ||
| } | ||
| } | ||
|
|
||
| static jboolean NativeCrypto_SSL_CTX_ech_enable_server(JNIEnv* env, jclass, jlong ssl_ctx_address, | ||
| CONSCRYPT_UNUSED jobject holder, | ||
| jbyteArray keyJavaBytes, | ||
| jbyteArray configJavaBytes) { | ||
| CHECK_ERROR_QUEUE_ON_RETURN; | ||
| SSL_CTX* ssl_ctx = to_SSL_CTX(env, ssl_ctx_address, true); | ||
| JNI_TRACE("NativeCrypto_SSL_CTX_ech_enable_server(keyJavaBytes=%p, configJavaBytes=%p)", | ||
| keyJavaBytes, configJavaBytes); | ||
| ScopedByteArrayRO keyBytes(env, keyJavaBytes); | ||
| if (keyBytes.get() == nullptr) { | ||
| JNI_TRACE( | ||
| "NativeCrypto_SSL_CTX_ech_enable_server => threw exception: " | ||
| "could not read key bytes"); | ||
| return JNI_FALSE; | ||
| } | ||
| ScopedByteArrayRO configBytes(env, configJavaBytes); | ||
| if (configBytes.get() == nullptr) { | ||
| JNI_TRACE( | ||
| "NativeCrypto_SSL_CTX_ech_enable_server => threw exception: " | ||
| "could not read config bytes"); | ||
| return JNI_FALSE; | ||
| } | ||
| const uint8_t* ech_key = reinterpret_cast<const uint8_t*>(keyBytes.get()); | ||
| size_t ech_key_size = keyBytes.size(); | ||
| const uint8_t* ech_config = reinterpret_cast<const uint8_t*>(configBytes.get()); | ||
| size_t ech_config_size = configBytes.size(); | ||
| bssl::UniquePtr<SSL_ECH_KEYS> keys(SSL_ECH_KEYS_new()); | ||
| bssl::ScopedEVP_HPKE_KEY key; | ||
| if (!keys || | ||
| !EVP_HPKE_KEY_init(key.get(), EVP_hpke_x25519_hkdf_sha256(), ech_key, ech_key_size) || | ||
| !SSL_ECH_KEYS_add(keys.get(), /*is_retry_config=*/1, ech_config, ech_config_size, | ||
| key.get()) || | ||
| !SSL_CTX_set1_ech_keys(ssl_ctx, keys.get())) { | ||
| JNI_TRACE( | ||
| "NativeCrypto_SSL_CTX_ech_enable_server: " | ||
| "Error setting server's ECHConfig and private key\n"); | ||
| return JNI_FALSE; | ||
| } | ||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| // TESTING METHODS END | ||
|
|
||
| #define CONSCRYPT_NATIVE_METHOD(functionName, signature) \ | ||
|
|
@@ -12133,6 +12350,17 @@ static JNINativeMethod sNativeCryptoMethods[] = { | |
| CONSCRYPT_NATIVE_METHOD(Scrypt_generate_key, "([B[BIIII)[B"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_CTX_set_spake_credential, "([B[B[B[BZIJ" REF_SSL_CTX ")V"), | ||
|
|
||
| // FOR ECH TESTING | ||
| CONSCRYPT_NATIVE_METHOD(SSL_set_enable_ech_grease, "(J" REF_SSL "Z)V"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_set1_ech_config_list, "(J" REF_SSL "[B)Z"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_get0_ech_name_override, "(J" REF_SSL ")Ljava/lang/String;"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_get0_ech_retry_configs, "(J" REF_SSL ")[B"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_ECH_KEYS_new, "()J"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_ECH_KEYS_up_ref, "(J)V"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_ECH_KEYS_free, "(J)V"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_ech_accepted, "(J" REF_SSL ")Z"), | ||
| CONSCRYPT_NATIVE_METHOD(SSL_CTX_ech_enable_server, "(J" REF_SSL_CTX "[B[B)Z"), | ||
|
|
||
| // Used for testing only. | ||
| CONSCRYPT_NATIVE_METHOD(BIO_read, "(J[B)I"), | ||
| CONSCRYPT_NATIVE_METHOD(BIO_write, "(J[BII)V"), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: do you know why this is necessary for your setup? I don't see any issues locally, but it might be due to some version difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had forgotten about this change. I assume it's an issue with my local configuration but the error wasn't specific. I can pull it out when I'm done.