Add support for checking only against local CRLs. by martin-barta-sie · Pull Request #29 · siemens/libsecutils

martin-barta-sie · 2022-08-15T13:26:31Z

No description provided.

DDvO

Hmm, basically the only actual improvements I see in this PR is the two small unsigned int -> int changes and the re-worked directory traversal in STORE_load_check_dir().

Why did you feel the need to introduce check_revocation_local_only_method()?
Unless a user adds extra cert status sources (via CDPs or OCSP) or enables the use of CDP or AIA entries in certs, it should already do exactly what you need (and nothing more).
Or what does check_revocation_any_method() by default do beyond what you need/want?

DDvO · 2022-08-22T12:22:26Z

include/secutils/certstatus/certstatus.h

+/*
+ * Check revocation status on certs in ctx->chain. As a generalization of
+ * check_revocation() in crypto/x509/x509_vfy.c, considers local CRLs only.
+ * To be used as a callback function to be past to


past -> passed

DDvO · 2022-08-22T12:31:47Z

include/secutils/certstatus/certstatus.h

+ * check_revocation() in crypto/x509/x509_vfy.c, considers local CRLs only.
+ * To be used as a callback function to be past to
+ * X509_STORE_set_check_revocation()
+ */


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()

DDvO · 2022-08-22T12:32:30Z

include/secutils/certstatus/certstatus.h

@@ -132,4 +132,12 @@ bool check_cert_revocation(X509_STORE_CTX* ctx, OPTIONAL OCSP_RESPONSE* resp);
 */


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

DDvO · 2022-08-22T12:43:57Z

src/credentials/store.c

    {
        LOG(FL_ERR, "cannot read directory '%s'", trust_dir);


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

DDvO · 2022-08-22T12:53:47Z

src/credentials/store.c

+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);
+}
+
+


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.

DDvO · 2022-08-22T13:31:57Z

include/secutils/certstatus/certstatus.h

+ * Check revocation status on certs in ctx->chain. As a generalization of
+ * check_revocation() in crypto/x509/x509_vfy.c, considers local CRLs only.


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.

DDvO · 2022-08-22T13:47:33Z

src/certstatus/certstatus.c

+
+
+/*
+ * @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;
+}


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.

DDvO · 2022-08-22T13:49:06Z

include/secutils/credentials/store.h

+/*!*****************************************************************************
+ * @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);
+


I see no point in defining this - see below.

DDvO · 2022-08-26T11:58:34Z

src/credentials/store.c

        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));


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

Add support for checking only against local CRLs.

03eddff

martin-barta-sie mentioned this pull request Aug 15, 2022

Merge our current version of the code with the OSS libsecutils. #27

Closed

DDvO requested changes Aug 22, 2022

View reviewed changes

DDvO reviewed Aug 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for checking only against local CRLs.#29

Add support for checking only against local CRLs.#29
martin-barta-sie wants to merge 1 commit intosiemens:masterfrom
martin-barta-sie:feature/local-crls

martin-barta-sie commented Aug 15, 2022

Uh oh!

DDvO left a comment

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 22, 2022

Uh oh!

DDvO Aug 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -132,4 +132,12 @@ bool check_cert_revocation(X509_STORE_CTX* ctx, OPTIONAL OCSP_RESPONSE* resp);
		*/

		* Check revocation status on certs in ctx->chain. As a generalization of
		* check_revocation() in crypto/x509/x509_vfy.c, considers local CRLs only.

Conversation

martin-barta-sie commented Aug 15, 2022

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants