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

X509_STORE_CTX accessors. #1044

Closed
wants to merge 3 commits into from
Closed

X509_STORE_CTX accessors. #1044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
815acea
X509_STORE_CTX accessors.
May 7, 2016
58c015a
Check calls X509_OBJECT_new for NULL
May 17, 2016
9ab7513
Allocate sk only just before used
May 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/crl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ int crl_main(int argc, char **argv)
goto end;
}

xobj = X509_STORE_get_X509_by_subject(ctx, X509_LU_X509,
X509_CRL_get_issuer(x));
xobj = X509_STORE_CTX_get_obj_by_subject(ctx, X509_LU_X509,
X509_CRL_get_issuer(x));
if (xobj == NULL) {
BIO_printf(bio_err, "Error getting CRL issuer certificate\n");
goto end;
Expand Down
4 changes: 2 additions & 2 deletions apps/s_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,8 +676,8 @@ static int cert_status_cb(SSL *s, void *arg)
SSL_CTX_get_cert_store(SSL_get_SSL_CTX(s)),
NULL, NULL))
goto err;
obj = X509_STORE_get_X509_by_subject(inctx, X509_LU_X509,
X509_get_issuer_name(x));
obj = X509_STORE_CTX_get_obj_by_subject(inctx, X509_LU_X509,
X509_get_issuer_name(x));
if (obj == NULL) {
BIO_puts(bio_err, "cert_status: Can't retrieve issuer certificate.\n");
goto done;
Expand Down
10 changes: 2 additions & 8 deletions crypto/x509/x509_err.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,11 @@
/*
* Generated by util/mkerr.pl DO NOT EDIT
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
*
* Licensed under the OpenSSL license (the "License"). You may not use
* this file except in compliance with the License. You can obtain a copy
* in the file LICENSE in the source distribution or at
* https://www.openssl.org/source/license.html
*/

/*
* NOTE: this file was auto generated by the mkerr.pl script: any changes
* made to it will be overwritten when the script next updates this file,
* only reason strings will be preserved.
*/

#include <stdio.h>
Expand Down Expand Up @@ -64,6 +59,7 @@ static ERR_STRING_DATA X509_str_functs[] = {
"X509_NAME_ENTRY_set_object"},
{ERR_FUNC(X509_F_X509_NAME_ONELINE), "X509_NAME_oneline"},
{ERR_FUNC(X509_F_X509_NAME_PRINT), "X509_NAME_print"},
{ERR_FUNC(X509_F_X509_OBJECT_NEW), "X509_OBJECT_new"},
{ERR_FUNC(X509_F_X509_PRINT_EX_FP), "X509_print_ex_fp"},
{ERR_FUNC(X509_F_X509_PUBKEY_DECODE), "x509_pubkey_decode"},
{ERR_FUNC(X509_F_X509_PUBKEY_GET0), "X509_PUBKEY_get0"},
Expand All @@ -81,8 +77,6 @@ static ERR_STRING_DATA X509_str_functs[] = {
{ERR_FUNC(X509_F_X509_STORE_CTX_NEW), "X509_STORE_CTX_new"},
{ERR_FUNC(X509_F_X509_STORE_CTX_PURPOSE_INHERIT),
"X509_STORE_CTX_purpose_inherit"},
{ERR_FUNC(X509_F_X509_STORE_GET_X509_BY_SUBJECT),
"X509_STORE_get_X509_by_subject"},
{ERR_FUNC(X509_F_X509_TO_X509_REQ), "X509_to_X509_REQ"},
{ERR_FUNC(X509_F_X509_TRUST_ADD), "X509_TRUST_add"},
{ERR_FUNC(X509_F_X509_TRUST_SET), "X509_TRUST_set"},
Expand Down
127 changes: 68 additions & 59 deletions crypto/x509/x509_lu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,25 +294,22 @@ X509_LOOKUP *X509_STORE_add_lookup(X509_STORE *v, X509_LOOKUP_METHOD *m)
}
}

X509_OBJECT *X509_STORE_get_X509_by_subject(X509_STORE_CTX *vs, int type,
X509_NAME *name)
X509_OBJECT *X509_STORE_CTX_get_obj_by_subject(X509_STORE_CTX *vs, int type,
Copy link
Member

Choose a reason for hiding this comment

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

Please change the prefix from X509_STORE_CTX_ to X509_STORE_. You're not manipulating the ctx, nor are you retrieving values from it, you'r just using it to get information from the associated store, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but the arg is a store_ctx and so it's an operation on the STORE_CTX. the old name exists in the compatibility section.

Copy link
Member

Choose a reason for hiding this comment

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

(never mind)

X509_NAME *name)
{
X509_OBJECT *ret;
X509_OBJECT *ret = X509_OBJECT_new();

ret = OPENSSL_malloc(sizeof (*ret));
if (ret == NULL) {
X509err(X509_F_X509_STORE_GET_X509_BY_SUBJECT, ERR_R_MALLOC_FAILURE);
if (ret == NULL)
return NULL;
}
if (!X509_STORE_get_by_subject(vs, type, name, ret)) {
OPENSSL_free(ret);
if (!X509_STORE_CTX_get_by_subject(vs, type, name, ret)) {
X509_OBJECT_free(ret);
return NULL;
}
return ret;
}

int X509_STORE_get_by_subject(X509_STORE_CTX *vs, X509_LOOKUP_TYPE type,
X509_NAME *name, X509_OBJECT *ret)
int X509_STORE_CTX_get_by_subject(X509_STORE_CTX *vs, X509_LOOKUP_TYPE type,
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here, please change the prefix back from X509_STORE_CTX_ to X509_STORE_.

Copy link
Member

Choose a reason for hiding this comment

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

(never mind)

X509_NAME *name, X509_OBJECT *ret)
{
X509_STORE *ctx = vs->ctx;
X509_LOOKUP *lu;
Expand Down Expand Up @@ -341,9 +338,6 @@ int X509_STORE_get_by_subject(X509_STORE_CTX *vs, X509_LOOKUP_TYPE type,
return 0;
}

/*- if (ret->data.ptr != NULL)
X509_OBJECT_free_contents(ret); */

ret->type = tmp->type;
ret->data.ptr = tmp->data.ptr;

Expand All @@ -359,11 +353,9 @@ int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)

if (x == NULL)
return 0;
obj = OPENSSL_malloc(sizeof(*obj));
if (obj == NULL) {
X509err(X509_F_X509_STORE_ADD_CERT, ERR_R_MALLOC_FAILURE);
obj = X509_OBJECT_new();
if (obj == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

So, sometimes this functions adds an error code on error, and sometimes not? That's not consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The X509_STORE_new sets an error code.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Ok.

return 0;
}
obj->type = X509_LU_X509;
obj->data.x509 = x;

Expand All @@ -372,8 +364,7 @@ int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
X509_OBJECT_up_ref_count(obj);

if (X509_OBJECT_retrieve_match(ctx->objs, obj)) {
X509_OBJECT_free_contents(obj);
OPENSSL_free(obj);
X509_OBJECT_free(obj);
X509err(X509_F_X509_STORE_ADD_CERT,
X509_R_CERT_ALREADY_IN_HASH_TABLE);
ret = 0;
Expand All @@ -392,11 +383,9 @@ int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x)

if (x == NULL)
return 0;
obj = OPENSSL_malloc(sizeof(*obj));
if (obj == NULL) {
X509err(X509_F_X509_STORE_ADD_CRL, ERR_R_MALLOC_FAILURE);
obj = X509_OBJECT_new();
if (obj == NULL)
return 0;
}
obj->type = X509_LU_CRL;
obj->data.crl = x;

Expand All @@ -405,8 +394,7 @@ int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x)
X509_OBJECT_up_ref_count(obj);

if (X509_OBJECT_retrieve_match(ctx->objs, obj)) {
X509_OBJECT_free_contents(obj);
OPENSSL_free(obj);
X509_OBJECT_free(obj);
X509err(X509_F_X509_STORE_ADD_CRL, X509_R_CERT_ALREADY_IN_HASH_TABLE);
ret = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, inconsistent setting of error code

} else
Expand All @@ -432,23 +420,37 @@ int X509_OBJECT_up_ref_count(X509_OBJECT *a)

X509 *X509_OBJECT_get0_X509(X509_OBJECT *a)
{
if (a == NULL || a->type != X509_LU_X509)
return NULL;
return a->data.x509;
}

X509_CRL *X509_OBJECT_get0_X509_CRL(X509_OBJECT *a)
{
if (a == NULL || a->type != X509_LU_CRL)
return NULL;
return a->data.crl;
}

int X509_OBJECT_get_type(X509_OBJECT *a)
{
return a->type;
}

void X509_OBJECT_free(X509_OBJECT *a)
X509_OBJECT *X509_OBJECT_new()
{
if (a == NULL)
return;
X509_OBJECT_free_contents(a);
OPENSSL_free(a);
X509_OBJECT *ret = OPENSSL_zalloc(sizeof(*ret));

if (ret == NULL) {
X509err(X509_F_X509_OBJECT_NEW, ERR_R_MALLOC_FAILURE);
return NULL;
}
ret->type = X509_LU_FAIL;
return ret;
}

void X509_OBJECT_free_contents(X509_OBJECT *a)

void X509_OBJECT_free(X509_OBJECT *a)
{
if (a == NULL)
return;
Expand All @@ -462,6 +464,7 @@ void X509_OBJECT_free_contents(X509_OBJECT *a)
X509_CRL_free(a->data.crl);
break;
}
OPENSSL_free(a);
}

static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type,
Expand Down Expand Up @@ -524,35 +527,39 @@ STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(X509_STORE *v)
return v->objs;
}

STACK_OF(X509) *X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here

Copy link
Contributor Author

@richsalz richsalz May 12, 2016 •
edited

Choose a reason for hiding this comment

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

same thing above. it's like having an RSA function take a PKEY parameter :)

Copy link
Member

Choose a reason for hiding this comment

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

(never mind)

{
int i, idx, cnt;
STACK_OF(X509) *sk;
STACK_OF(X509) *sk = NULL;
X509 *x;
X509_OBJECT *obj;
sk = sk_X509_new_null();

CRYPTO_THREAD_write_lock(ctx->ctx->lock);
idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_X509, nm, &cnt);
if (idx < 0) {
/*
* Nothing found in cache: do lookup to possibly add new objects to
* cache
*/
X509_OBJECT xobj;
X509_OBJECT *xobj = X509_OBJECT_new();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be checked for NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, fixed.


CRYPTO_THREAD_unlock(ctx->ctx->lock);
if (!X509_STORE_get_by_subject(ctx, X509_LU_X509, nm, &xobj)) {
sk_X509_free(sk);
if (xobj == NULL)
return NULL;
if (!X509_STORE_CTX_get_by_subject(ctx, X509_LU_X509, nm, xobj)) {
X509_OBJECT_free(xobj);
return NULL;
}
X509_OBJECT_free_contents(&xobj);
X509_OBJECT_free(xobj);
CRYPTO_THREAD_write_lock(ctx->ctx->lock);
idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_X509, nm, &cnt);
if (idx < 0) {
CRYPTO_THREAD_unlock(ctx->ctx->lock);
sk_X509_free(sk);
return NULL;
}
}

sk = sk_X509_new_null();
for (i = 0; i < cnt; i++, idx++) {
obj = sk_X509_OBJECT_value(ctx->ctx->objs, idx);
x = obj->data.x509;
Expand All @@ -566,25 +573,23 @@ STACK_OF(X509) *X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
}
CRYPTO_THREAD_unlock(ctx->ctx->lock);
return sk;

}

STACK_OF(X509_CRL) *X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm)
STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm)
{
int i, idx, cnt;
STACK_OF(X509_CRL) *sk;
STACK_OF(X509_CRL) *sk = sk_X509_CRL_new_null();
X509_CRL *x;
X509_OBJECT *obj, xobj;
sk = sk_X509_CRL_new_null();
X509_OBJECT *obj, *xobj = X509_OBJECT_new();

Copy link
Member

Choose a reason for hiding this comment

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

xobj should be checked for NULL, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

/*
* Always do lookup to possibly add new CRLs to cache
*/
if (!X509_STORE_get_by_subject(ctx, X509_LU_CRL, nm, &xobj)) {
/* Always do lookup to possibly add new CRLs to cache */
if (sk == NULL || xobj == NULL ||
!X509_STORE_CTX_get_by_subject(ctx, X509_LU_CRL, nm, xobj)) {
X509_OBJECT_free(xobj);
sk_X509_CRL_free(sk);
return NULL;
}
X509_OBJECT_free_contents(&xobj);
X509_OBJECT_free(xobj);
CRYPTO_THREAD_write_lock(ctx->ctx->lock);
idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_CRL, nm, &cnt);
if (idx < 0) {
Expand Down Expand Up @@ -650,32 +655,36 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
Copy link
Member

Choose a reason for hiding this comment

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

Ok, now I'm not so sure any more...

Copy link
Member

Choose a reason for hiding this comment

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

Having looked around, I'm convinced this one is misnamed and should be called X509_STORE_get1_issuer. If noone minds waiting a little bit, I'll ask DrH.

Copy link
Member

Choose a reason for hiding this comment

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

(never mind)

{
X509_NAME *xn;
X509_OBJECT obj, *pobj;
X509_OBJECT *obj = X509_OBJECT_new(), *pobj = NULL;
int i, ok, idx, ret;

if (obj == NULL)
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget an error code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set by x509_object_new

*issuer = NULL;
xn = X509_get_issuer_name(x);
ok = X509_STORE_get_by_subject(ctx, X509_LU_X509, xn, &obj);
ok = X509_STORE_CTX_get_by_subject(ctx, X509_LU_X509, xn, obj);
if (ok != X509_LU_X509) {
X509_OBJECT_free(obj);
if (ok == X509_LU_RETRY) {
X509_OBJECT_free_contents(&obj);
X509err(X509_F_X509_STORE_CTX_GET1_ISSUER, X509_R_SHOULD_RETRY);
return -1;
} else if (ok != X509_LU_FAIL) {
X509_OBJECT_free_contents(&obj);
}
if (ok != X509_LU_FAIL) {
/* not good :-(, break anyway */
return -1;
}
return 0;
}
/* If certificate matches all OK */
if (ctx->check_issued(ctx, x, obj.data.x509)) {
if (x509_check_cert_time(ctx, obj.data.x509, -1)) {
*issuer = obj.data.x509;
if (ctx->check_issued(ctx, x, obj->data.x509)) {
if (x509_check_cert_time(ctx, obj->data.x509, -1)) {
*issuer = obj->data.x509;
X509_up_ref(*issuer);
X509_OBJECT_free(obj);
return 1;
}
}
X509_OBJECT_free_contents(&obj);
X509_OBJECT_free(obj);

/* Else find index of first cert accepted by 'check_issued' */
ret = 0;
Expand Down
4 changes: 2 additions & 2 deletions crypto/x509/x509_vfy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2220,12 +2220,12 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
if (store && store->lookup_certs)
ctx->lookup_certs = store->lookup_certs;
else
ctx->lookup_certs = X509_STORE_get1_certs;
ctx->lookup_certs = X509_STORE_CTX_get1_certs;

if (store && store->lookup_crls)
ctx->lookup_crls = store->lookup_crls;
else
ctx->lookup_crls = X509_STORE_get1_crls;
ctx->lookup_crls = X509_STORE_CTX_get1_crls;

ctx->check_policy = check_policy;

Expand Down
7 changes: 4 additions & 3 deletions include/openssl/x509.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,12 +1042,12 @@ char *X509_TRUST_get0_name(X509_TRUST *xp);
int X509_TRUST_get_trust(X509_TRUST *xp);

/* BEGIN ERROR CODES */

/*
* The following lines are auto generated by the script mkerr.pl. Any changes
* made after this point may be overwritten when the script is next run.
* Content after this point is generated by util/mkerr.pl
* DO NOT EDIT!
*/
void ERR_load_X509_strings(void);

/* Error codes for the X509 functions. */

/* Function codes. */
Expand Down Expand Up @@ -1082,6 +1082,7 @@ void ERR_load_X509_strings(void);
# define X509_F_X509_NAME_ENTRY_SET_OBJECT 115
# define X509_F_X509_NAME_ONELINE 116
# define X509_F_X509_NAME_PRINT 117
# define X509_F_X509_OBJECT_NEW 150
# define X509_F_X509_PRINT_EX_FP 118
# define X509_F_X509_PUBKEY_DECODE 148
# define X509_F_X509_PUBKEY_GET0 119
Expand Down