[PATCH] OpenSSL: BoringSSL has SSL_get_client_random, etc.

[David Benjamin]

On Thu, May 5, 2016 at 5:16 PM, Jouni Malinen <j at w1.fi> wrote:
> This is problematic with existing versions of BoringSSL. As an example,
> if I apply this and try to build against my previous BoringSSL build,
> the compilation fails with:
>
>   CC  ../src/eap_peer/eap_tls_common.c
> ../src/crypto/tls_openssl.c: In function ‘tls_connection_get_random’:
> ../src/crypto/tls_openssl.c:3079:2: error: implicit declaration of function ‘SSL_get_client_random’ [-Werror=implicit-function-declaration]
>   keys->client_random_len = SSL_get_client_random(
>   ^
> ../src/crypto/tls_openssl.c:3082:2: error: implicit declaration of function ‘SSL_get_server_random’ [-Werror=implicit-function-declaration]
>   keys->server_random_len = SSL_get_server_random(
>   ^
> ../src/crypto/tls_openssl.c: In function ‘openssl_tls_prf’:
> ../src/crypto/tls_openssl.c:3203:2: error: implicit declaration of function ‘SSL_SESSION_get_master_key’
> [-Werror=implicit-function-declaration]
>   master_key_len = SSL_SESSION_get_master_key(sess, master_key,
>   ^
>
>
> IMHO, it is quite unfortunate that BoringSSL is maintained in a manner
> that prevents clean backwards compatibility with at least the versions
> used in the recent past. Applying this patch would break various Android
> cases where the BoringSSL version in the branch is not sufficiently
> recent to have the macro defined.
>
> Would there be any other way of using the pre-processor to automatically
> determine whether BoringSSL is recent enough to include the new
> commands? The "Add SSL_get_client_random and SSL_get_server_random"
> doesn't seem to add anything to the header files to help the
> pre-processor for this..

So, previously, our approach was not to try to support old versions of
BoringSSL much. It's annoying having our own code (much less yours!)
saddled with #ifdefs for our past mistakes. (You all have enough of a
mess on your hands with multiple versions of OpenSSL. I don't want to
make things worse!) Instead, we only use it in environments with
controlled versions that move forwards, so we can coordinate caller
and callee updates, sometimes even atomically. If atomic changes
aren't possible, we'd add very temporary scaffolding #defines and
#ifdefs locally, get to the end state (caller and callee both assumed
new enough) as fast as possible, and dismantle it.

Though talking with Dmitry some more, it sounds like it's actually
desirable to be able to build newer wpa_supplicant against a release
or two behind of AOSP? Is that right? This isn't how we had previous
done things on the BoringSSL end, but we can certainly accommodate it.

I can start a BORINGSSL_API_VERSION counter and roll that into AOSP
now. This will be a random meaningless number except we promise it
will only increase and we'll probably increment it at points vaguely
corresponding with additions or changes in the API, wherever it ends
up convenient to do so. :-) Then this patch will be updated to be
defined(BORINGSSL_API_VERSION). In future we'd do
BORINGSSL_API_VERSION > whatever. And then you all can figure out how
far back it should go. (For my part, I want to minimize your burden,
so I would encourage you need to retain support for versions older
than you need, but it sounds like your master branch cares about more
Android releases than I thought.)

Does that sound reasonable?

>> BoringSSL added 1.1.0's SSL_get_client_random and friends in working towards
>> opaquifying the SSL struct. But it, for the moment, still looks more like 1.0.2
>> than 1.1.0 and advertises OPENSSL_VERSION_NUMBER as such. This means that there
>> is no need to define those in BoringSSL and defining them causes conflicts. (C
>> does not like having static and non-static functions with the same name.)
>
> I guess the wrapper functions in src/crypto/tls_openssl.c could be
> defined to us an alternative name and then use #define to override the
> functions. Not that this is exactly nice, but at least it seems to build
> with the current boringssl.git snapshot:
>
> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> index ebcc545..0f67290 100644
> --- a/src/crypto/tls_openssl.c
> +++ b/src/crypto/tls_openssl.c
> [snip]

Yeah, that's horrific. :-) I would definitely prefer changing our
strategy over forcing you to do that!

If we start the BORINGSSL_API_VERSION counter, the only versions
subject to the naming conflict would be those between when I added
SSL_get_client_random and when I add BORINGSSL_API_VERSION. I'll
update AOSP as soon that happens, so no released version of AOSP will
check a BoringSSL revision in that window. That should avoid this.

David