Matt Caswell [Fri, 22 Jul 2016 10:55:10 +0000 (11:55 +0100)]
Update the SSL_set_bio()/SSL_set0_rbio()/SSL_set0_wbio() docs
Update the documentation for the newly renamed and modified SSL_set0_rbio()
and SSL_set0_wbio() functions. State that they should be preferred over
SSL_set_bio(). Attempt to document the ownership rules for SSL_set_bio().
Matt Caswell [Thu, 21 Jul 2016 11:17:29 +0000 (12:17 +0100)]
Simplify and rename SSL_set_rbio() and SSL_set_wbio()
SSL_set_rbio() and SSL_set_wbio() are new functions in 1.1.0 and really
should be called SSL_set0_rbio() and SSL_set0_wbio(). The old
implementation was not consistent with what "set0" means though as there
were special cases around what happens if the rbio and wbio are the same.
We were only ever taking one reference on the BIO, and checking everywhere
whether the rbio and wbio are the same so as not to double free.
A better approach is to rename the functions to SSL_set0_rbio() and
SSL_set0_wbio(). If an existing BIO is present it is *always* freed
regardless of whether the rbio and wbio are the same or not. It is
therefore the callers responsibility to ensure that a reference is taken
for *each* usage, i.e. one for the rbio and one for the wbio.
The legacy function SSL_set_bio() takes both the rbio and wbio in one go
and sets them both. We can wrap up the old behaviour in the implementation
of that function, i.e. previously if the rbio and wbio are the same in the
call to this function then the caller only needed to ensure one reference
was passed. This behaviour is retained by internally upping the ref count.
This commit was inspired by BoringSSL commit f715c423224.
Matt Caswell [Thu, 21 Jul 2016 10:02:22 +0000 (11:02 +0100)]
Add some SSL BIO tests
This adds some simple SSL BIO tests that check for pushing and popping of
BIOs into the chain. These tests would have caught the bugs fixed in the
previous three commits, if combined with a crypto-mdebug build.
Matt Caswell [Thu, 21 Jul 2016 09:55:31 +0000 (10:55 +0100)]
Fix BIO_pop for SSL BIOs
The BIO_pop implementation assumes that the rbio still equals the next BIO
in the chain. While this would normally be the case, it is possible that it
could have been changed directly by the application. It also does not
properly cater for the scenario where the buffering BIO is still in place
for the write BIO.
Most of the existing BIO_pop code for SSL BIOs can be replaced by a single
call to SSL_set_bio(). This is equivalent to the existing code but
additionally handles the scenario where the rbio has been changed or the
buffering BIO is still in place.
Matt Caswell [Thu, 21 Jul 2016 09:48:12 +0000 (10:48 +0100)]
Fix BIO_push ref counting for SSL BIO
When pushing a BIO onto an SSL BIO we set the rbio and wbio for the SSL
object to be the BIO that has been pushed. Therefore we need to up the ref
count for that BIO. The existing code was uping the ref count on the wrong
BIO.
Matt Caswell [Fri, 22 Jul 2016 13:58:19 +0000 (14:58 +0100)]
Make the checks for an SSLv2 style record stricter
SSLv2 is no longer supported in 1.1.0, however we *do* still accept an SSLv2
style ClientHello, as long as we then subsequently negotiate a protocol
version >= SSLv3. The record format for SSLv2 style ClientHellos is quite
different to SSLv3+. We only accept this format in the first record of an
initial ClientHello. Previously we checked this by confirming
s->first_packet is set and s->server is true. However, this really only
tells us that we are dealing with an initial ClientHello, not that it is
the first record (s->first_packet is badly named...it really means this is
the first message). To check this is the first record of the initial
ClientHello we should also check that we've not received any data yet
(s->init_num == 0), and that we've not had any empty records.
David Benjamin [Tue, 26 Jul 2016 15:36:23 +0000 (11:36 -0400)]
Use sk_CONF_VALUE_pop_free in do_ext_nconf error path.
8605abf13523579ecab8b1f2a4bcb8354d94af79 fixed the nval leak, but it
used free instead of pop_free. nval owns its contents, so it should be
freed with pop_free. See the pop_free call a few lines down.
This is a no-op as, in this codepath, we must have nval == NULL or
sk_CONF_VALUE_num(nval) == 0. In those cases, free and pop_free are
identical. However, variables should be freed consistently.
Reviewed-by: Kurt Roeckx <kurt@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1351)
Remove current_method: it was intended as a means of retrying
lookups bit it was never used. Now that X509_verify_cert() is
a "one shot" operation it can never work as intended.
This encoding issue also causes the same errors with 2048-bit DHE, if the
public key is encoded in fewer than 256 bytes and includes the TLS stack on
Windows Phone 8.x.
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1320)
Richard Levitte [Mon, 25 Jul 2016 15:02:56 +0000 (17:02 +0200)]
Make it possible for external code to set the certiciate proxy path length
This adds the functions X509_set_proxy_pathlen(), which sets the
internal pc path length cache for a given X509 structure, along with
X509_get_proxy_pathlen(), which retrieves it.
Along with the previously added X509_set_proxy_flag(), this provides
the tools needed to manipulate all the information cached on proxy
certificates, allowing external code to do what's necessary to have
them verified correctly by the libcrypto code.
Richard Levitte [Thu, 7 Jul 2016 21:55:34 +0000 (23:55 +0200)]
Add X509_STORE lock and unlock functions
Since there are a number of function pointers in X509_STORE that might
lead to user code, it makes sense for them to be able to lock the
store while they do their work.
Richard Levitte [Thu, 7 Jul 2016 21:22:45 +0000 (23:22 +0200)]
Add getters / setters for the X509_STORE_CTX and X509_STORE functions
We only add setters for X509_STORE function pointers except for the
verify callback function. The thought is that the function pointers
in X509_STORE_CTX are a cache for the X509_STORE functions.
Therefore, it's preferable if the user makes the changes in X509_STORE
before X509_STORE_CTX_init is called, and otherwise use the verify
callback to override any results from OpenSSL's internal
calculations.
TS_OBJ_print_bio() misuses OBJ_txt2obj: it should print the result
as a null terminated buffer. The length value returned is the total
length the complete text reprsentation would need not the amount of
data written.
Richard Levitte [Thu, 21 Jul 2016 10:33:23 +0000 (12:33 +0200)]
VMS: Rearrange installation targets for shared libraries
The way it was implemented before this change, the shared libraries
were installed twice. On a file system that supports file
generations, that's a waste. Slightly rearranging the install targets
solves the problem.
Kurt Roeckx [Sun, 17 Jul 2016 13:28:09 +0000 (15:28 +0200)]
Cast to an unsigned type before negating
llvm's ubsan reported:
runtime error: negation of -9223372036854775808 cannot be represented in type
'long'; cast to an unsigned type to negate this value to itself
Richard Levitte [Tue, 19 Jul 2016 11:24:57 +0000 (13:24 +0200)]
Install shared libraries in runtime install
On non-Windows platforms, shared libraries are both development and
runtime files. We only installed them as development files, this
makes sure they get installed as runtime files as well.
Matt Caswell [Fri, 17 Jun 2016 12:59:59 +0000 (13:59 +0100)]
Never expose ssl->bbio in the public API.
This is adapted from BoringSSL commit 2f87112b963.
This fixes a number of bugs where the existence of bbio was leaked in the
public API and broke things.
- SSL_get_wbio returned the bbio during the handshake. It must always return
the BIO the consumer configured. In doing so, some internal accesses of
SSL_get_wbio should be switched to ssl->wbio since those want to see bbio.
- The logic in SSL_set_rfd, etc. (which I doubt is quite right since
SSL_set_bio's lifetime is unclear) would get confused once wbio got
wrapped. Those want to compare to SSL_get_wbio.
- If SSL_set_bio was called mid-handshake, bbio would get disconnected and
lose state. It forgets to reattach the bbio afterwards. Unfortunately,
Conscrypt does this a lot. It just never ended up calling it at a point
where the bbio would cause problems.
- Make more explicit the invariant that any bbio's which exist are always
attached. Simplify a few things as part of that.
RT#4572
Reviewed-by: Richard Levitte <levitte@openssl.org>
f0e0fd51fd8307f6eae64862ad9aaea113f1177a added X509_STORE_CTX_set_verify_cb
with a typedef'd argument, making the original one redundant. Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org>
Todd Short [Tue, 5 Jul 2016 13:59:29 +0000 (09:59 -0400)]
OCSP_request_add0_id() inconsistent error return
There are two failure cases for OCSP_request_add_id():
1. OCSP_ONEREQ_new() failure, where |cid| is not freed
2. sk_OCSP_ONEREQ_push() failure, where |cid| is freed
This changes makes the error behavior consistent, such that |cid| is
not freed when sk_OCSP_ONEREQ_push() fails. OpenSSL only takes
ownership of |cid| when the function succeeds.
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1289)
Richard Levitte [Tue, 19 Jul 2016 17:38:57 +0000 (19:38 +0200)]
Define a few internal macros for easy use of run_once functions
Because pthread_once() takes a function taking no argument and
returning nothing, and we want to be able to check if they're
successful, we define a few internal macros to get around the issue.