-
Notifications
You must be signed in to change notification settings - Fork 85
Android's certificate verification JNI layer #2251
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 7 commits
51cf7ca
232f12c
d3ddc19
fda54b8
4f12064
15d9bc7
68f24df
f8a6a46
aa0beee
153320a
55dc250
5c461ed
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 | ||
|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||
|
|
||||
| #include "library/common/jni/jni_support.h" | ||||
| #include "library/common/jni/jni_version.h" | ||||
| #include "library/common/strings/string_conversions.h" | ||||
|
|
||||
| // NOLINT(namespace-envoy) | ||||
|
|
||||
|
|
@@ -255,3 +256,74 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { | |||
| envoy_map native_map = {length / 2, entry_array}; | ||||
| return native_map; | ||||
| } | ||||
|
|
||||
| jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str) { | ||||
| // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it | ||||
|
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. OOC, what is "modified UTF8"?
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. AFAIU it's what is used by the JVM under the hood https://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/types.html#wp16542.
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. Thanks Wowwwww! That is wacky! I didn't realize "Modified UTF-8" was term of art. Wikipedia also has a good summary. Modified UTF-8 (MUTF-8) originated in the Java programming language. In Modified UTF-8, the null character (U+0000) uses the two-byte overlong encoding 11000000 10000000 (hexadecimal C0 80), instead of 00000000 (hexadecimal 00).[79] Modified UTF-8 strings never contain any actual null bytes but can contain all Unicode code points including U+0000,[80] which allows such strings (with a null byte appended) to be processed by traditional null-terminated string functions.
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. To avoid surprises due to the weirdness of Modified UTF-8, the convention we've adopted a this layer is to pass true UTF-8 byte arrays across the JNI and then encode/decode them to java.lang.Strings on the Java side. For simplicity's sake (and perhaps to avoid the risk of extra logic manipulating buffers) it would be nice to maintain that convention, if possible, rather than introduce Modified UTF-8 handling in C++.
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. Oh I wasn't aware of that, my bad. Can you point me to a specific example so I can more closely follow your approach?
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. This is a simple example that shows the JNI side (this supports an implementation of a simple extension to retrieve runtime-defined platform Strings):
And here is the Java side:
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've updated the code to pass true UTF-8 byte arrays across the JNI interface as you suggested. I don't think I need to wrap stuff in an accessor for my use case (the strings are only needed for the duration of the JNI call). Let me know if this looks okay to you! |
||||
| // to NewString. | ||||
| std::u16string utf16 = UTF8ToUTF16(str.data(), str.size()); | ||||
| const jchar* jutf16 = reinterpret_cast<const jchar*>(utf16.data()); | ||||
| return env->NewString(jutf16, utf16.length()); | ||||
| } | ||||
|
|
||||
| jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector<std::string>& v) { | ||||
| jclass jcls_byte_array = env->FindClass("[B"); | ||||
| jobjectArray joa = env->NewObjectArray(v.size(), jcls_byte_array, nullptr); | ||||
|
|
||||
| for (size_t i = 0; i < v.size(); ++i) { | ||||
| jbyteArray byte_array = | ||||
| ToJavaByteArray(env, reinterpret_cast<const uint8_t*>(v[i].data()), v[i].length()); | ||||
| env->SetObjectArrayElement(joa, i, byte_array); | ||||
| } | ||||
| return joa; | ||||
| } | ||||
|
|
||||
| jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len) { | ||||
| jbyteArray byte_array = env->NewByteArray(len); | ||||
| const jbyte* jbytes = reinterpret_cast<const jbyte*>(bytes); | ||||
| env->SetByteArrayRegion(byte_array, /*start=*/0, len, jbytes); | ||||
| return byte_array; | ||||
| } | ||||
|
|
||||
| void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, | ||||
| std::vector<std::string>* out) { | ||||
| size_t len = env->GetArrayLength(array); | ||||
| out->resize(len); | ||||
|
|
||||
| for (size_t i = 0; i < len; ++i) { | ||||
| jbyteArray bytes_array = static_cast<jbyteArray>(env->GetObjectArrayElement(array, i)); | ||||
| jsize bytes_len = env->GetArrayLength(bytes_array); | ||||
| // It doesn't matter if the array returned by GetByteArrayElements is a copy | ||||
| // or not, as the data will be simply be copied into C++ owned memory below. | ||||
| jbyte* bytes = env->GetByteArrayElements(bytes_array, /*isCopy=*/nullptr); | ||||
| (*out)[i].assign(reinterpret_cast<const char*>(bytes), bytes_len); | ||||
| // There is nothing to write back, it is always safe to JNI_ABORT. | ||||
| env->ReleaseByteArrayElements(bytes_array, bytes, JNI_ABORT); | ||||
| // Explicitly delete to keep the local ref count low. | ||||
| env->DeleteLocalRef(bytes_array); | ||||
| } | ||||
| } | ||||
|
|
||||
| void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vector<uint8_t>* out) { | ||||
| const size_t len = env->GetArrayLength(array); | ||||
| out->resize(len); | ||||
|
|
||||
| // It doesn't matter if the array returned by GetByteArrayElements is a copy | ||||
| // or not, as the data will be simply be copied into C++ owned memory below. | ||||
| jbyte* jbytes = env->GetByteArrayElements(array, /*isCopy=*/nullptr); | ||||
| uint8_t* bytes = reinterpret_cast<uint8_t*>(jbytes); | ||||
| std::copy(bytes, bytes + len, out->begin()); | ||||
| // There is nothing to write back, it is always safe to JNI_ABORT. | ||||
| env->ReleaseByteArrayElements(array, jbytes, JNI_ABORT); | ||||
| } | ||||
|
|
||||
| void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result) { | ||||
| // JNI's GetStringUTFChars() returns strings in Java "modified" UTF8, so | ||||
| // instead get the String in UTF16 and manually convert that to UTF8. | ||||
| // We don't care whether the returned string is a copy or not as we're simply | ||||
| // copying it into C++ owned memory. | ||||
| const jchar* utf16 = env->GetStringChars(str, /*isCopy=*/nullptr); | ||||
| size_t len = env->GetStringLength(str); | ||||
| std::string utf8 = UTF16ToUTF8(reinterpret_cast<const char16_t*>(utf16), len); | ||||
| *result = utf8; | ||||
| env->ReleaseStringChars(str, utf16); | ||||
| } | ||||
|
StefanoDuo marked this conversation as resolved.
Outdated
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| load("@rules_cc//cc:defs.bzl", "cc_library") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| cc_library( | ||
| name = "string_conversions_lib", | ||
| srcs = ["string_conversions.cc"], | ||
| hdrs = ["string_conversions.h"], | ||
| visibility = ["//visibility:public"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| #include "library/common/strings/string_conversions.h" | ||
|
|
||
| #include <codecvt> | ||
| #include <locale> | ||
|
|
||
| std::string UTF16ToUTF8(const char16_t* utf16, size_t len) { | ||
| return std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t>{}.to_bytes(utf16, | ||
| utf16 + len); | ||
| } | ||
|
|
||
| std::u16string UTF8ToUTF16(const char* utf8, size_t len) { | ||
| return std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t>{}.from_bytes(utf8, | ||
| utf8 + len); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
|
StefanoDuo marked this conversation as resolved.
Outdated
|
||
| std::string UTF16ToUTF8(const char16_t* utf16, size_t len); | ||
|
StefanoDuo marked this conversation as resolved.
Outdated
|
||
|
|
||
| std::u16string UTF8ToUTF16(const char* utf8, size_t len); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -488,3 +488,27 @@ typedef struct { | |
| // Context passed through to callbacks to provide dispatch and execution state. | ||
| const void* context; | ||
| } envoy_event_tracker; | ||
|
|
||
| /** | ||
| * The list of certificate verification results returned from Java side to the | ||
| * C++ side. | ||
| * A Java counterpart lives in org.chromium.net.CertVerifyStatusAndroid.java | ||
| */ | ||
| typedef enum { | ||
| // Certificate is trusted. | ||
| CERT_VERIFY_STATUS_ANDROID_OK = 0, | ||
| // Certificate verification could not be conducted. | ||
| CERT_VERIFY_STATUS_ANDROID_FAILED = -1, | ||
| // Certificate is not trusted due to non-trusted root of the certificate | ||
| // chain. | ||
| CERT_VERIFY_STATUS_ANDROID_NO_TRUSTED_ROOT = -2, | ||
| // Certificate is not trusted because it has expired. | ||
| CERT_VERIFY_STATUS_ANDROID_EXPIRED = -3, | ||
| // Certificate is not trusted because it is not valid yet. | ||
| CERT_VERIFY_STATUS_ANDROID_NOT_YET_VALID = -4, | ||
| // Certificate is not trusted because it could not be parsed. | ||
| CERT_VERIFY_STATUS_ANDROID_UNABLE_TO_PARSE = -5, | ||
| // Certificate is not trusted because it has an extendedKeyUsage field, but | ||
| // its value is not correct for a web server. | ||
| CERT_VERIFY_STATUS_ANDROID_INCORRECT_KEY_USAGE = -6, | ||
| } envoy_cert_verify_status_android_t; | ||
|
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. Can this enum be made generic such that it could be utilized by other platforms (currently iOS and C++)?
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've removed the "android qualifier". I'm not entirely sure if other platform semantics will/do differ, but I think this should be a reasonable first iteration, thoughts? |
||
Uh oh!
There was an error while loading. Please reload this page.