kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Unwind X509_LU_RETRY and fix a lot of type confusion.

Browse Source 
(This change will be sent upstream. Since the legacy X.509 stack is just
kept around for compatibility, if they decide to fix it in a different
way, we may wish to revert this and apply their fix.)

Dating back to SSLeay, X509_LOOKUP_METHOD had this X509_LU_RETRY
machinery. But it's not documented and it appears to have never worked.

Problems with the existing logic:

- X509_LU_* is not sure whether it is a type enum (to be passed into
  X509_LOOKUP_by_*) or a return enum (to be retained by those same
  functions).

- X509_LOOKUP_by_* is not sure whether it returns 0/1 or an X509_LU_*
  value.  Looking at the functions themselves, one might think it's the
  latter, but for X509_LOOKUP_by_subject returning both 0 and
  X509_LU_FAIL. But looking at the call sites, some expect 0/1 (such as
  X509_STORE_get1_certs) while others expect an X509_LU_* enum (such as
  X509_STORE_CTX_get1_issuer). It is very fortunate that FAIL happens to
  be 0 and X509 happens to be 1.

  These functions primarily call to X509_LOOKUP_METHOD hooks. Looking
  through OpenSSL itself and code checked into Google, I found no
  evidence that any hooks have been implemented except for
  get_by_subject in by_dir.c. We take that one as definitive and observe
  it believes it returns 0/1. Notably, it returns 1 on success even if
  asked for a type other than X509_LU_X509. (X509_LU_X509 = 1. Others are
  different.) I found another piece of third-party software which corroborates
  this worldview.

- X509_STORE_get_by_subject's handling of X509_LU_RETRY (it's the j < 0
  check) is broken. It saves j into vs->current_method where it probably
  meant to save i. (This bug has existed since SSLeay.)

  It also returns j (supposedly X509_LU_RETRY) while all callers of
  X509_STORE_get_by_subject expect it to return 0/1 by checking with !
  instead of <= 0. (Note that all other codepaths return 0 and 1 so this
  function did not actually believe it returned X509_LU_* most of the
  time.)

  This, in turn, gives us a free of uninitialized pointers in
  X509_STORE_get1_certs and other functions which expect that *ret is
  filled in if X509_STORE_get_by_subject returns success. GCC 4.9 with
  optimizations from the Android NDK noticed this, which trigged this
  saga.

  (It's only reachable if any X509_LOOKUP_METHOD returned
  X509_LU_RETRY.)

- Although the code which expects X509_STORE_get_by_subject return 0/1
  does not date to SSLeay, the X509_STORE_get_by_subject call in
  X509_STORE_CTX_get1_issuer *does* (though, at the time, it was inline
  in X509_verify_cert. That code believes X509_STORE_get_by_subject
  returns an X509_LU_* enum, but it doesn't work either! It believes
  *ret is filled in on X509_LU_RETRY, thus freeing another uninitialized
  pointer (GCC noticed this too).

Since this "retry" code has clearly never worked, from SSLeay onwards,
unwind it completely rather than attempt to fix it. No
X509_LOOKUP_METHOD can possibly have depended on it.

Matching all non-broken codepaths X509_LOOKUP_by_* now returns 0/1 and
X509_STORE_get_by_subject returns 0/1. X509_LU_* is purely a type enum
with X509_LU_{REJECT,FAIL} being legacy constants to keep old code
compiling. (Upstream is recommended to remove those values altogether
for 1.1.0.)

On the off chance any get_by_* X509_LOOKUP_METHOD implementations did
not return 0/1 (I have found no evidence anywhere of this, and I believe
it wouldn't have worked anyway), the X509_LOOKUP_by_* wrapper functions
will coerce the return values back to 0/1 before passing up to the
callers which want 0/1. This both avoids the error-prone -1/0/1 calling
convention and, more importantly, avoids problems with third-party
callers which expect a X509_LU_* return code. 0/1 collide with FAIL/X509
while -1 will collide with RETRY and might confuse things.

Change-Id: I98ecf6fa7342866b9124dc6f0b422cb9ce4a1ae7
Reviewed-on: https://boringssl-review.googlesource.com/8303
Reviewed-by: Adam Langley <agl@google.com>
This commit is contained in:
David Benjamin 2016-06-15 18:41:51 -04:00 committed by Adam Langley
parent 054e597670
commit da7f0c65ef
2 changed files with 15 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
crypto/x509/x509_lu.c
View File

@ -130,18 +130,18 @@ int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
X509_OBJECT *ret) X509_OBJECT *ret)
{ {
if ((ctx->method == NULL) || (ctx->method->get_by_subject == NULL)) if ((ctx->method == NULL) || (ctx->method->get_by_subject == NULL))
return X509_LU_FAIL; return 0;
if (ctx->skip) if (ctx->skip)
return 0; return 0;
return ctx->method->get_by_subject(ctx, type, name, ret); return ctx->method->get_by_subject(ctx, type, name, ret) > 0;
} }
int X509_LOOKUP_by_issuer_serial(X509_LOOKUP *ctx, int type, X509_NAME *name, int X509_LOOKUP_by_issuer_serial(X509_LOOKUP *ctx, int type, X509_NAME *name,
ASN1_INTEGER *serial, X509_OBJECT *ret) ASN1_INTEGER *serial, X509_OBJECT *ret)
{ {
if ((ctx->method == NULL) || (ctx->method->get_by_issuer_serial == NULL)) if ((ctx->method == NULL) || (ctx->method->get_by_issuer_serial == NULL))
return X509_LU_FAIL; return 0;
return ctx->method->get_by_issuer_serial(ctx, type, name, serial, ret); return ctx->method->get_by_issuer_serial(ctx, type, name, serial, ret) > 0;
} }
int X509_LOOKUP_by_fingerprint(X509_LOOKUP *ctx, int type, int X509_LOOKUP_by_fingerprint(X509_LOOKUP *ctx, int type,
@ -149,16 +149,16 @@ int X509_LOOKUP_by_fingerprint(X509_LOOKUP *ctx, int type,
X509_OBJECT *ret) X509_OBJECT *ret)
{ {
if ((ctx->method == NULL) || (ctx->method->get_by_fingerprint == NULL)) if ((ctx->method == NULL) || (ctx->method->get_by_fingerprint == NULL))
return X509_LU_FAIL; return 0;
return ctx->method->get_by_fingerprint(ctx, type, bytes, len, ret); return ctx->method->get_by_fingerprint(ctx, type, bytes, len, ret) > 0;
} }
int X509_LOOKUP_by_alias(X509_LOOKUP *ctx, int type, char *str, int len, int X509_LOOKUP_by_alias(X509_LOOKUP *ctx, int type, char *str, int len,
X509_OBJECT *ret) X509_OBJECT *ret)
{ {
if ((ctx->method == NULL) || (ctx->method->get_by_alias == NULL)) if ((ctx->method == NULL) || (ctx->method->get_by_alias == NULL))
return X509_LU_FAIL; return 0;
return ctx->method->get_by_alias(ctx, type, str, len, ret); return ctx->method->get_by_alias(ctx, type, str, len, ret) > 0;
} }
static int x509_object_cmp(const X509_OBJECT **a, const X509_OBJECT **b) static int x509_object_cmp(const X509_OBJECT **a, const X509_OBJECT **b)
@ -301,26 +301,20 @@ int X509_STORE_get_by_subject(X509_STORE_CTX *vs, int type, X509_NAME *name,
X509_STORE *ctx = vs->ctx; X509_STORE *ctx = vs->ctx;
X509_LOOKUP *lu; X509_LOOKUP *lu;
X509_OBJECT stmp, *tmp; X509_OBJECT stmp, *tmp;
int i, j; int i;
CRYPTO_MUTEX_lock_write(&ctx->objs_lock); CRYPTO_MUTEX_lock_write(&ctx->objs_lock);
tmp = X509_OBJECT_retrieve_by_subject(ctx->objs, type, name); tmp = X509_OBJECT_retrieve_by_subject(ctx->objs, type, name);
CRYPTO_MUTEX_unlock_write(&ctx->objs_lock); CRYPTO_MUTEX_unlock_write(&ctx->objs_lock);
if (tmp == NULL || type == X509_LU_CRL) { if (tmp == NULL || type == X509_LU_CRL) {
for (i = vs->current_method; for (i = 0; i < (int)sk_X509_LOOKUP_num(ctx->get_cert_methods); i++) {
i < (int)sk_X509_LOOKUP_num(ctx->get_cert_methods); i++) {
lu = sk_X509_LOOKUP_value(ctx->get_cert_methods, i); lu = sk_X509_LOOKUP_value(ctx->get_cert_methods, i);
j = X509_LOOKUP_by_subject(lu, type, name, &stmp); if (X509_LOOKUP_by_subject(lu, type, name, &stmp)) {
if (j < 0) {
vs->current_method = j;
return j;
} else if (j) {
tmp = &stmp; tmp = &stmp;
break; break;
} }
} }
vs->current_method = 0;
if (tmp == NULL) if (tmp == NULL)
return 0; return 0;
} }
@ -611,22 +605,11 @@ int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
{ {
X509_NAME *xn; X509_NAME *xn;
X509_OBJECT obj, *pobj; X509_OBJECT obj, *pobj;
int ok, idx, ret; int idx, ret;
size_t i; size_t i;
xn = X509_get_issuer_name(x); xn = X509_get_issuer_name(x);
ok = X509_STORE_get_by_subject(ctx, X509_LU_X509, xn, &obj); if (!X509_STORE_get_by_subject(ctx, X509_LU_X509, xn, &obj))
if (ok != X509_LU_X509) {
if (ok == X509_LU_RETRY) {
X509_OBJECT_free_contents(&obj);
OPENSSL_PUT_ERROR(X509, X509_R_SHOULD_RETRY);
return -1;
} else if (ok != X509_LU_FAIL) {
X509_OBJECT_free_contents(&obj);
/* not good :-(, break anyway */
return -1;
}
return 0; return 0;
}
/* If certificate matches all OK */ /* If certificate matches all OK */
if (ctx->check_issued(ctx, x, obj.data.x509)) { if (ctx->check_issued(ctx, x, obj.data.x509)) {
*issuer = obj.data.x509; *issuer = obj.data.x509;

3
include/openssl/x509_vfy.h
View File

@ -109,8 +109,10 @@ The X509_STORE then calls a function to actually verify the
certificate chain. certificate chain.
*/ */
/* The following are legacy constants that should not be used. */
#define X509_LU_RETRY -1 #define X509_LU_RETRY -1
#define X509_LU_FAIL 0 #define X509_LU_FAIL 0
#define X509_LU_X509 1 #define X509_LU_X509 1
#define X509_LU_CRL 2 #define X509_LU_CRL 2
#define X509_LU_PKEY 3 #define X509_LU_PKEY 3
@ -228,7 +230,6 @@ struct x509_lookup_st
struct x509_store_ctx_st /* X509_STORE_CTX */ struct x509_store_ctx_st /* X509_STORE_CTX */
{ {
X509_STORE *ctx; X509_STORE *ctx;
int current_method; /* used when looking up certs */
/* The following are set by the caller */ /* The following are set by the caller */
X509 *cert; /* The cert to check */ X509 *cert; /* The cert to check */