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

Add support for checking only against local CRLs. #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for checking only against local CRLs. #29

Show file tree
Hide file tree
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
8 changes: 8 additions & 0 deletions include/secutils/certstatus/certstatus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,12 @@ bool check_cert_revocation(X509_STORE_CTX* ctx, OPTIONAL OCSP_RESPONSE* resp);
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please also fix a typo of mine in the line before: compatiblity -> compatibility

int check_revocation_any_method(X509_STORE_CTX* ctx);

/*
* Check revocation status on certs in ctx->chain. As a generalization of
* check_revocation() in crypto/x509/x509_vfy.c, considers local CRLs only.
Comment on lines +136 to +137
Copy link
Member

Choose a reason for hiding this comment

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

Note that check_revocation() in crypto/x509/x509_vfy.c does only consider local CRLs.
So as far as this function is needed at all (see further commens below), just say:

* Check revocation status on certs in ctx->chain considering local CRLs only.

* To be used as a callback function to be past to
Copy link
Member

Choose a reason for hiding this comment

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

past -> passed

* X509_STORE_set_check_revocation()
*/
Copy link
Member

Choose a reason for hiding this comment

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

Missing @param and @return lines etc:

 * @param ctx pointer to verification context structure including the cert(s) to check
 * @return true on success, false on rejection or checking error (inconclusive)
 * @note using result type 'int' rather than 'bool' for compatibility with X509_STORE_set_check_revocation()

int check_revocation_local_only_method(X509_STORE_CTX* ctx);

#endif /* SECUTILS_CERTSTATUS_H_ */
17 changes: 17 additions & 0 deletions include/secutils/credentials/store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ bool STORE_load_check_dir(X509_STORE** pstore, const char* trust_dir,

/*!*****************************************************************************
* @brief search for files with CRLs in specified directory and add them to X509_STORE
* @note this function sets check_revocation_any_method() to be used as a callback during CRL validation
*
* @param pstore pointer to trust store to be augmented with CRLs.
* CRL-based status checking will be enabled in it for the full certificate chain.
Expand All @@ -220,6 +221,22 @@ bool STORE_load_check_dir(X509_STORE** pstore, const char* trust_dir,
******************************************************************************/
bool STORE_load_crl_dir(X509_STORE* pstore, const char* crl_dir, OPTIONAL const char* desc, bool recursive, OPTIONAL uta_ctx* ctx);

/*!*****************************************************************************
* @brief search for files with CRLs in specified directory and add them to X509_STORE
* @note this function sets check_revocation_local_only_method() to be used as a callback during CRL validation
*
* @param pstore pointer to trust store to be augmented with CRLs.
* CRL-based status checking will be enabled in it for the full certificate chain.
* @param crl_dir directory where to search for CRLs
* @param desc description of CRLs to use for error reporting, or null
* @param recursive if true, use recursive search in subdirectories
* @param ctx pointer to UTA context for checking file integrity&authenticity using ICV, or null
* @note at least one valid CRL file must be found in each visited directory
* @return true on success, false on error/failure
******************************************************************************/
bool STORE_load_crl_dir_local_only(X509_STORE* pstore, const char* crl_dir, OPTIONAL const char* desc, bool recursive,
OPTIONAL uta_ctx* ctx);

Comment on lines +224 to +239
Copy link
Member

Choose a reason for hiding this comment

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

I see no point in defining this - see below.

/*!*****************************************************************************
* @brief release a trust store
*
Expand Down
160 changes: 158 additions & 2 deletions src/certstatus/certstatus.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

#include <operators.h>

static unsigned int num_CDPs(const X509* cert)
static int num_CDPs(const X509* cert)
{
CRL_DIST_POINTS* cdps = X509_get_ext_d2i(cert, NID_crl_distribution_points, 0, 0);
if(cdps is_eq 0) /* maybe there is still a CDP for delta CRLs */
Expand All @@ -42,7 +42,7 @@ static unsigned int num_CDPs(const X509* cert)
return res;
}

static unsigned int num_AIAs(const X509* cert)
static int num_AIAs(const X509* cert)
{
STACK_OF(OPENSSL_STRING) *aias = X509_get1_ocsp((X509 *)cert);
int res = aias not_eq 0 ? sk_OPENSSL_STRING_num(aias) : 0;
Expand Down Expand Up @@ -416,3 +416,159 @@ int check_revocation_any_method(X509_STORE_CTX* ctx)
}
return true;
}


/*
* @brief Function to check certificate revocation of a certificate and its
* issuer at depth determined by X509_STORE_CTX error_depth
* @param ctx pointer to X509 store context
* @return X509_V_OK (0) on success
* X509_V_ERR_ (>0) error code on failure
*/
static int check_local_cert_crls(X509_STORE_CTX* ctx)
{
if(0 is_eq ctx)
{
LOG(FL_ERR, "null parameter ctx");
return X509_V_ERR_UNSPECIFIED;
}

X509_STORE_CTX* tmp_ctx = X509_STORE_CTX_new();
X509_STORE_CTX_check_revocation_fn check_revocation;
int cert_idx = X509_STORE_CTX_get_error_depth(ctx);
X509* cert = sk_X509_value(X509_STORE_CTX_get0_chain(ctx), cert_idx);
X509* issuer = sk_X509_value(X509_STORE_CTX_get0_chain(ctx), cert_idx+1);
int ssl_ex_idx = SSL_get_ex_data_X509_STORE_CTX_idx();
SSL* ssl = X509_STORE_CTX_get_ex_data(ctx, ssl_ex_idx);
STACK_OF(X509) *certs;
int ret = X509_V_ERR_UNSPECIFIED;

/*
* Unfortunately, check_revocation() in crypto/x509/x509_vfy.c is static,
* yet we can get hold of it via X509_STORE_CTX_get_check_revocation().
*/
if(tmp_ctx is_eq 0
or not X509_STORE_CTX_init(tmp_ctx, 0, 0, 0)
or ((check_revocation = X509_STORE_CTX_get_check_revocation(tmp_ctx)) is_eq 0))
{
LOG(FL_ERR, "cannot get pointer to check_revocation()");
goto err;
}
X509_STORE_CTX_set0_param(tmp_ctx, 0); /* free tmp_ctx->param */
if(not X509_STORE_CTX_init(tmp_ctx, X509_STORE_CTX_get0_store(ctx),
0, 0) /* inherits flags etc. of store */
or not X509_STORE_CTX_set_ex_data(tmp_ctx, ssl_ex_idx, ssl))
{
LOG(FL_ERR, "cannot set up tmp_ctx");
goto err;
}
if((certs = sk_X509_new_reserve(0, 2)) is_eq 0
or (X509_STORE_CTX_set0_verified_chain(tmp_ctx, certs), 0)
or cert is_eq 0 or not sk_X509_push(certs, X509_dup(cert))
or issuer is_eq 0 or not sk_X509_push(certs, X509_dup(issuer)))
{
LOG(FL_ERR, "cannot set certs in tmp_ctx");
goto err;
}

X509_VERIFY_PARAM* tmp_vpm = X509_STORE_CTX_get0_param(tmp_ctx);
X509_VERIFY_PARAM_clear_flags(tmp_vpm, X509_V_FLAG_CRL_CHECK_ALL);
X509_VERIFY_PARAM_set_flags(tmp_vpm, X509_V_FLAG_NONFINAL_CHECK | X509_V_FLAG_NO_CHECK_TIME);
int res = (*check_revocation)(tmp_ctx); /* checks only depth 0 */

if(res is_eq 0)
{
ret = X509_STORE_CTX_get_error(tmp_ctx);
}
else
{
ret = X509_V_OK;
}

err:
X509_STORE_CTX_free(tmp_ctx);
return ret;
}

static bool crl_time_valid(const X509_CRL* crl, const X509_VERIFY_PARAM* vpm)
{
time_t check_time, *ptime = 0;
const ASN1_TIME* crl_end_time;
const ASN1_TIME* crl_last_update;
unsigned long flags = X509_VERIFY_PARAM_get_flags((X509_VERIFY_PARAM*)vpm);

if((flags bitand X509_V_FLAG_USE_CHECK_TIME) not_eq 0)
{
check_time = X509_VERIFY_PARAM_get_time(vpm);
ptime = &check_time;
}
crl_end_time = X509_CRL_get0_nextUpdate(crl);
crl_last_update = X509_CRL_get0_lastUpdate(crl);

if ((crl_end_time not_eq 0 and X509_cmp_time(crl_end_time, ptime) not_eq 1) or
(crl_last_update not_eq 0 and X509_cmp_time(crl_last_update, ptime) not_eq -1))
{
return false;
}

return true;
}

int check_revocation_local_only_method(X509_STORE_CTX* ctx)
{
if(0 is_eq ctx)
{
LOG(FL_ERR, "null parameter ctx");
return 0;
}

STACK_OF(X509) *chain = X509_STORE_CTX_get0_chain(ctx);
int i = 0;
const int last = sk_X509_num(chain) - 1;

for(i = 0; i <= last; i++)
{
X509* cert = sk_X509_value(chain, i);
if(i is_eq last and X509_check_issued(cert, cert) is_eq X509_V_OK)
{
LOG_cert(FL_DEBUG, "skipping revocation check for self-issued last", cert);
break;
}

STACK_OF(X509_CRL)* crls = X509_STORE_CTX_get1_crls(ctx, X509_get_issuer_name(cert));
if(not crls or 0 is_eq sk_X509_CRL_num(crls) or 0 is_eq sk_X509_CRL_value(crls, 0))
{
char* issuer = X509_NAME_oneline(X509_get_issuer_name(cert), 0, 0);
if (0 not_eq issuer)
{
LOG(FL_ERR, "Local CRL for %s certificate %s not found",
(i + 1 is_eq last) ? "root" : "issuing", issuer);
OPENSSL_free(issuer);
}
return 0;
}

if(true not_eq crl_time_valid(sk_X509_CRL_value(crls, 0), X509_STORE_CTX_get0_param(ctx)))
{
char* issuer = X509_NAME_oneline(X509_CRL_get_issuer(sk_X509_CRL_value(crls, 0)), 0, 0);
if(issuer not_eq 0)
{
LOG(FL_WARN, "CRL issued by %s is not valid in time", issuer);
OPENSSL_free(issuer);
}
}

sk_X509_CRL_pop_free(crls, X509_CRL_free);

X509_STORE_CTX_set_error_depth(ctx, i);
int res = check_local_cert_crls(ctx);

if(X509_V_OK not_eq res)
{
verify_cb_cert(ctx, cert, res);
return 0;
}
chain = X509_STORE_CTX_get0_chain(ctx); /* for some reason need again */
}
return 1;
}
Comment on lines +419 to +574
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for all the effort you spent here, but I see no point in defining check_revocation_local_only_method() and STORE_load_crl_dir_local_only() because
check_revocation_any_method() should already do all you need if no extra cert status sources (via CDPs or OCSP) are defined or enabled, doesn't it?

And even if not, it would have been much better not to try to copy and strip down check_revocation_any_method(), which just causes much redundancy and very likely introduces errors and omissions, but to adapt check_revocation_any_method() - if this is needed after all.

70 changes: 49 additions & 21 deletions src/credentials/store.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,33 +304,36 @@ bool STORE_load_check_dir(X509_STORE** pstore, const char* trust_dir,
OPTIONAL const char* desc, bool recursive,
OPTIONAL X509_VERIFY_PARAM *vpm, OPTIONAL uta_ctx* ctx)
{
DIR* p_dir = 0;
bool found = false;
int count = -1;
int i;

if(0 is_eq pstore or 0 is_eq trust_dir)
{
LOG_err("null pointer argument");
goto err;
}

p_dir = opendir(trust_dir);
if(0 is_eq p_dir)
struct dirent** namelist = 0;
count = scandir(trust_dir, &namelist, 0, alphasort);
if(-1 is_eq count)
{
LOG(FL_ERR, "cannot read directory '%s'", trust_dir);
Comment on lines 320 to 321
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to make use of errno in the error reporting.

goto err;
}

struct dirent* p_dirent = readdir(p_dir);
while(0 not_eq p_dirent)
for(i = 0; i < count; ++i)
{
const struct dirent* p_dirent = namelist[i];

char full_path[UTIL_max_path_len + 1];
snprintf(full_path, sizeof(full_path), "%s/%s", trust_dir, p_dirent->d_name);

struct stat f_stat;
memset(&f_stat, 0x00, sizeof(struct stat));
if(-1 is_eq stat(full_path, &f_stat))
{
LOG(FL_INFO, "cannot read status of %s - %s", full_path, strerror(errno));
LOG(FL_ERR, "cannot read status of %s - %s", full_path, strerror(errno));
Copy link
Member

Choose a reason for hiding this comment

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

If you consider this an error, is it still good to continue?

}
else
{
Expand All @@ -350,7 +353,6 @@ bool STORE_load_check_dir(X509_STORE** pstore, const char* trust_dir,
}
}
}
p_dirent = readdir(p_dir);
}

if(not found)
Expand All @@ -361,9 +363,13 @@ bool STORE_load_check_dir(X509_STORE** pstore, const char* trust_dir,
desc not_eq 0 ? desc : "trusted certs", ctx not_eq 0 ? "with valid ICV " : "", trust_dir);
}
err:
if(p_dir not_eq 0)
for(i = 0; i < count; ++i)
{
closedir(p_dir);
free(namelist[i]);
}
if (-1 not_eq count)
{
free(namelist);
}

int cert_num = STORE_certs_num(*pstore);
Expand All @@ -375,35 +381,40 @@ bool STORE_load_check_dir(X509_STORE** pstore, const char* trust_dir,
return found;
}

bool STORE_load_crl_dir(X509_STORE* pstore, const char* crl_dir, OPTIONAL const char* desc, bool recursive, OPTIONAL uta_ctx* ctx)

static bool STORE_load_crl_dir_impl(X509_STORE* pstore, const char* crl_dir, OPTIONAL const char* desc, bool recursive,
OPTIONAL uta_ctx* ctx, int (*callback)(X509_STORE_CTX*))
{
DIR* p_dir = 0;
bool found = false;
int count = -1;
int i;

if(0 is_eq pstore or 0 is_eq crl_dir)
{
LOG(FL_ERR, "null pointer argument");
goto err;
}

p_dir = opendir(crl_dir);
if(0 is_eq p_dir)
struct dirent** namelist = 0;
count = scandir(crl_dir, &namelist, 0, alphasort);
if(-1 is_eq count)
{
LOG(FL_ERR, "cannot access directory '%s'", crl_dir);
goto err;
}

struct dirent* p_dirent = readdir(p_dir);
while(0 not_eq p_dirent)
for(i = 0; i < count; ++i)
{
const struct dirent* p_dirent = namelist[i];

char full_path[UTIL_max_path_len + 1];
snprintf(full_path, sizeof(full_path), "%s/%s", crl_dir, p_dirent->d_name);

struct stat f_stat;
memset(&f_stat, 0x00, sizeof(struct stat));
if(-1 is_eq stat(full_path, &f_stat))
{
LOG(FL_INFO, "cannot read status of %s - %s", full_path, strerror(errno));
LOG(FL_ERR, "cannot read status of %s - %s", full_path, strerror(errno));
}
else
{
Expand All @@ -430,32 +441,49 @@ bool STORE_load_crl_dir(X509_STORE* pstore, const char* crl_dir, OPTIONAL const
}
else if(recursive and (f_stat.st_mode bitand S_IFDIR) and (0 not_eq strncmp(p_dirent->d_name, ".", 1)))
{
if(not STORE_load_crl_dir(pstore, full_path, desc, recursive, ctx))
if(not STORE_load_crl_dir_impl(pstore, full_path, desc, recursive, ctx, callback))
{
found = false;
goto err;
}
}
}
p_dirent = readdir(p_dir);
}

// set up cert CRL check callback for checking full chain
if(found)
{
X509_VERIFY_PARAM_set_flags(X509_STORE_get0_param(pstore), X509_V_FLAG_STATUS_CHECK_ALL);
X509_STORE_set_check_revocation(pstore, &check_revocation_any_method);
X509_STORE_set_check_revocation(pstore, callback);
}

err:
if(p_dir not_eq 0)
for(i = 0; i < count; ++i)
{
free(namelist[i]);
}
if (-1 not_eq count)
{
closedir(p_dir);
free(namelist);
}

return found;
}


bool STORE_load_crl_dir(X509_STORE* pstore, const char* crl_dir, OPTIONAL const char* desc, bool recursive, OPTIONAL uta_ctx* ctx)
{
return STORE_load_crl_dir_impl(pstore, crl_dir, desc, recursive, ctx, &check_revocation_any_method);
}


bool STORE_load_crl_dir_local_only(X509_STORE* pstore, const char* crl_dir, OPTIONAL const char* desc, bool recursive,
OPTIONAL uta_ctx* ctx)
{
return STORE_load_crl_dir_impl(pstore, crl_dir, desc, recursive, ctx, &check_revocation_local_only_method);
}


Comment on lines +474 to +486
Copy link
Member

Choose a reason for hiding this comment

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

You could have implemented these simply as macros.

Or even keep the original name for the default case check_revocation_any_method and define
STORE_load_crl_dir_local_only() by calling STORE_load_crl_dir() and thereafter
X509_STORE_set_check_revocation(pstore, check_revocation_local_only_method).

Yet as commented above, I see no point in defining STORE_load_crl_dir_local_only() since this is anyway the behavior in case STORE_load_crl_dir() is used and no extra cert status sources (via CDPs or AIAs) are defined or enabled.

bool STORE_set1_host_ip(X509_STORE* ts, const char* name, const char* ip)
{
if(ts is_eq 0)
Expand Down