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.

Sign up for GitHub

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

Jump to bottom

Correctly implement SSL.getSigAlgs(...) for BoringSSL #412

Merged
merged 1 commit into from
Nov 14, 2018
Merged

Correctly implement SSL.getSigAlgs(...) for BoringSSL #412

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions openssl-dynamic/src/main/c/ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2235,10 +2235,35 @@ TCN_IMPLEMENT_CALL(jobjectArray, SSL, getSigAlgs)(TCN_STDARGS, jlong ssl) {
SSL *ssl_ = J2P(ssl, SSL *);
TCN_CHECK_NULL(ssl_, ssl, NULL);

// Not supported by BoringSSL and LibreSSL
// https://boringssl.googlesource.com/boringssl/+/ba16a1e405c617f4179bd780ad15522fb25b0a65%5E%21/
#if defined(OPENSSL_IS_BORINGSSL) || defined(LIBRESSL_VERSION_NUMBER)
// Not supported in LibreSSL
#if defined(LIBRESSL_VERSION_NUMBER)
return NULL;
#elif defined(OPENSSL_IS_BORINGSSL)
// Using a different API in BoringSSL
// https://boringssl.googlesource.com/boringssl/+/ba16a1e405c617f4179bd780ad15522fb25b0a65%5E%21/
int i;
jobjectArray array;
jstring algString;
const uint16_t *peer_sigalgs;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be freed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ejona86 nope... get0 does not transfer ownership.

size_t num_peer_sigalgs = SSL_get0_peer_verify_algorithms(ssl_, &peer_sigalgs);
if (num_peer_sigalgs <= 0) {
return NULL;
}
array = (*e)->NewObjectArray(e, num_peer_sigalgs, tcn_get_string_class(), NULL);

if (array == NULL) {
return NULL;
}

for (i = 0; i < num_peer_sigalgs; i++) {
algString = (*e)->NewStringUTF(e, SSL_get_signature_algorithm_name(peer_sigalgs[i], SSL_version(ssl_) != TLS1_2_VERSION));
if (algString == NULL) {
// something is wrong we should better just return here
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't array need to be freed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point... this problem exists in other places as well (as this code was mostly a copy + modify of existing code). I will do a pr shortly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually no it does not need to be freed as it will be handled by the GC:

https://stackoverflow.com/a/12207973/1074097

return NULL;
}
(*e)->SetObjectArrayElement(e, array, i, algString);
}
return array;
#else

// Use weak linking with GCC as this will alow us to run the same packaged version with multiple
Expand Down