From 6425b3d92a96e5891e52d2fffe2efba9f4c4aa45 Mon Sep 17 00:00:00 2001 From: olszomal Date: Wed, 15 Jul 2026 09:11:31 +0200 Subject: [PATCH 1/4] Fix URI attribute buffer termination Reserve space for the terminating NUL byte and validate the decoded length before writing it, preventing a potential out-of-bounds write. Fixes CodeQL alert #7. --- src/util_uri.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/util_uri.c b/src/util_uri.c index 2bfc5198..8c63642c 100644 --- a/src/util_uri.c +++ b/src/util_uri.c @@ -695,20 +695,25 @@ static int parse_uri_attr_len(UTIL_CTX *ctx, return ret; } +/* + * Decode a URI attribute into a newly allocated NUL-terminated string. + * URI decoding cannot increase the input length. + */ static int parse_uri_attr(UTIL_CTX *ctx, - const char *attr, int attrlen, char **field) + const char *attr, size_t attrlen, char **field) { int ret = 1; - size_t outlen = attrlen + 1; - char *out = OPENSSL_malloc(outlen); + size_t outlen = attrlen; + char *out; - if (!out) + out = OPENSSL_malloc(outlen + 1); /* reserve 1 byte for NUL terminator */ + if (out == NULL) return 0; ret = parse_uri_attr_len(ctx, attr, attrlen, out, &outlen); if (ret) { - out[outlen] = 0; + out[outlen] = '\0'; *field = out; } else { OPENSSL_free(out); @@ -717,7 +722,6 @@ static int parse_uri_attr(UTIL_CTX *ctx, return ret; } - static int read_from_file(UTIL_CTX *ctx, const char *path, char *field, size_t *field_len) { @@ -752,7 +756,7 @@ static int read_from_file(UTIL_CTX *ctx, } static int parse_pin_source(UTIL_CTX *ctx, - const char *attr, int attrlen, char *field, + const char *attr, size_t attrlen, char *field, size_t *field_len) { char *val; @@ -803,30 +807,30 @@ static int parse_pkcs11_uri(UTIL_CTX *ctx, if (!strncmp(p, "model=", 6)) { p += 6; - rv = parse_uri_attr(ctx, p, (int)(end - p), &tok->model); + rv = parse_uri_attr(ctx, p, (size_t)(end - p), &tok->model); } else if (!strncmp(p, "manufacturer=", 13)) { p += 13; - rv = parse_uri_attr(ctx, p, (int)(end - p), &tok->manufacturer); + rv = parse_uri_attr(ctx, p, (size_t)(end - p), &tok->manufacturer); } else if (!strncmp(p, "token=", 6)) { p += 6; - rv = parse_uri_attr(ctx, p, (int)(end - p), &tok->label); + rv = parse_uri_attr(ctx, p, (size_t)(end - p), &tok->label); } else if (!strncmp(p, "serial=", 7)) { p += 7; - rv = parse_uri_attr(ctx, p, (int)(end - p), &tok->serialnr); + rv = parse_uri_attr(ctx, p, (size_t)(end - p), &tok->serialnr); } else if (!strncmp(p, "object=", 7)) { p += 7; - rv = parse_uri_attr(ctx, p, (int)(end - p), &newlabel); + rv = parse_uri_attr(ctx, p, (size_t)(end - p), &newlabel); } else if (!strncmp(p, "id=", 3)) { p += 3; - rv = parse_uri_attr_len(ctx, p, (int)(end - p), id, id_len); + rv = parse_uri_attr_len(ctx, p, (size_t)(end - p), id, id_len); id_set = 1; } else if (!strncmp(p, "pin-value=", 10)) { p += 10; - rv = pin_set ? 0 : parse_uri_attr_len(ctx, p, (int)(end - p), pin, pin_len); + rv = pin_set ? 0 : parse_uri_attr_len(ctx, p, (size_t)(end - p), pin, pin_len); pin_set = 1; } else if (!strncmp(p, "pin-source=", 11)) { p += 11; - rv = pin_set ? 0 : parse_pin_source(ctx, p, (int)(end - p), pin, pin_len); + rv = pin_set ? 0 : parse_pin_source(ctx, p, (size_t)(end - p), pin, pin_len); pin_set = 1; } else if (!strncmp(p, "type=", 5) || !strncmp(p, "object-type=", 12)) { p = strchr(p, '=') + 1; From 41b3d7b9f02618d3623d8abad00ed9868f83014e Mon Sep 17 00:00:00 2001 From: olszomal Date: Wed, 15 Jul 2026 09:18:45 +0200 Subject: [PATCH 2/4] Document intentional PKCS#11 module loading --- src/libpkcs11.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libpkcs11.c b/src/libpkcs11.c index 69e0453e..bef4e1b9 100644 --- a/src/libpkcs11.c +++ b/src/libpkcs11.c @@ -66,6 +66,7 @@ C_LoadModule(const char *mspec, CK_FUNCTION_LIST_PTR_PTR funcs) memset(mod, 0, sizeof(sc_pkcs11_module_t)); mod->_magic = MAGIC; + /* The caller intentionally selects the PKCS#11 module to load. */ #ifdef WIN32 mod->handle = LoadLibraryA(mspec); #else From 957181949eb1052cae5be43ac7bd1eb5b11468fc Mon Sep 17 00:00:00 2001 From: olszomal Date: Wed, 15 Jul 2026 10:49:12 +0200 Subject: [PATCH 3/4] Document certificate selection logic and simplify logging Fixes CodeQL alert #18 --- src/util_uri.c | 78 ++++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/src/util_uri.c b/src/util_uri.c index 8c63642c..002e6cdb 100644 --- a/src/util_uri.c +++ b/src/util_uri.c @@ -1254,6 +1254,40 @@ static PKCS11_CERT *cert_cmp(PKCS11_CERT *a, PKCS11_CERT *b) } } +static void log_cert(UTIL_CTX *ctx, unsigned int index, + PKCS11_CERT *cert, const char *which) +{ + char *hexbuf; + char *expiry; + + hexbuf = dump_hex((unsigned char *)cert->id, cert->id_len); + expiry = dump_expiry(cert); + + if (which != NULL) { + UTIL_CTX_log(ctx, LOG_NOTICE, + "Returning %s certificate:%s%s%s%s%s%s\n", which, + hexbuf ? " id=" : "", + hexbuf ? hexbuf : "", + cert->label ? " label=" : "", + cert->label ? cert->label : "", + expiry ? " expiry=" : "", + expiry ? expiry : ""); + } else { + UTIL_CTX_log(ctx, LOG_NOTICE, + " %2u %s%s%s%s%s%s\n", index, + hexbuf ? " id=" : "", + hexbuf ? hexbuf : "", + cert->label ? " label=" : "", + cert->label ? cert->label : "", + expiry ? " expiry=" : "", + expiry ? expiry : ""); + } + + OPENSSL_free(hexbuf); + OPENSSL_free(expiry); +} + +/* Find and select a certificate matching the requested object ID or label. */ static void *match_cert(UTIL_CTX *ctx, PKCS11_TOKEN *tok, const char *obj_id, size_t obj_id_len, const char *obj_label) { @@ -1261,8 +1295,8 @@ static void *match_cert(UTIL_CTX *ctx, PKCS11_TOKEN *tok, PKCS11_CERT cert_template = {0}; unsigned int m, cert_count; const char *which; - char *hexbuf, *expiry; + /* Build a certificate template from the requested label and ID. */ errno = 0; cert_template.label = obj_label ? OPENSSL_strdup(obj_label) : NULL; if (errno != 0) { @@ -1279,6 +1313,7 @@ static void *match_cert(UTIL_CTX *ctx, PKCS11_TOKEN *tok, cert_template.id_len = obj_id_len; } + /* Enumerate certificates matching the template. */ if (PKCS11_enumerate_certs_ext(tok, &cert_template, &certs, &cert_count)) { UTIL_CTX_log(ctx, LOG_ERR, "Unable to enumerate certificates\n"); goto cleanup; @@ -1289,21 +1324,13 @@ static void *match_cert(UTIL_CTX *ctx, PKCS11_TOKEN *tok, } UTIL_CTX_log(ctx, LOG_NOTICE, "Found %u certificate%s:\n", cert_count, cert_count == 1 ? "" : "s"); if (obj_id_len != 0 || obj_label) { + /* Select the matching certificate with the longest validity + * period when the caller supplied an ID or label. */ which = "longest expiry matching"; for (m = 0; m < cert_count; m++) { PKCS11_CERT *k = certs + m; - hexbuf = dump_hex((unsigned char *)k->id, k->id_len); - expiry = dump_expiry(k); - UTIL_CTX_log(ctx, LOG_NOTICE, " %2u %s%s%s%s%s%s\n", m + 1, - hexbuf ? " id=" : "", - hexbuf ? hexbuf : "", - k->label ? " label=" : "", - k->label ? k->label : "", - expiry ? " expiry=" : "", - expiry ? expiry : ""); - OPENSSL_free(hexbuf); - OPENSSL_free(expiry); + log_cert(ctx, m + 1, k, NULL); if (obj_label && obj_id_len != 0) { if (k->label && strcmp(k->label, obj_label) == 0 && @@ -1323,21 +1350,13 @@ static void *match_cert(UTIL_CTX *ctx, PKCS11_TOKEN *tok, } } } else { + /* Without explicit selection criteria, prefer the first certificate + * with a nonempty ID and otherwise use the first certificate. */ which = "first (with id present)"; for (m = 0; m < cert_count; m++) { PKCS11_CERT *k = certs + m; - hexbuf = dump_hex((unsigned char *)k->id, k->id_len); - expiry = dump_expiry(k); - UTIL_CTX_log(ctx, LOG_NOTICE, " %2u %s%s%s%s%s%s\n", m + 1, - hexbuf ? " id=" : "", - hexbuf ? hexbuf : "", - k->label ? " label=" : "", - k->label ? k->label : "", - expiry ? " expiry=" : "", - expiry ? expiry : ""); - OPENSSL_free(hexbuf); - OPENSSL_free(expiry); + log_cert(ctx, m + 1, k, NULL); if (!selected_cert && k->id && *k->id) { selected_cert = k; /* Use the first certificate with nonempty id */ @@ -1349,18 +1368,9 @@ static void *match_cert(UTIL_CTX *ctx, PKCS11_TOKEN *tok, } } + /* Log the certificate selected for return. */ if (selected_cert) { - hexbuf = dump_hex((unsigned char *)selected_cert->id, selected_cert->id_len); - expiry = dump_expiry(selected_cert); - UTIL_CTX_log(ctx, LOG_NOTICE, "Returning %s certificate:%s%s%s%s%s%s\n", which, - hexbuf ? " id=" : "", - hexbuf ? hexbuf : "", - selected_cert->label ? " label=" : "", - selected_cert->label ? selected_cert->label : "", - expiry ? " expiry=" : "", - expiry ? expiry : ""); - OPENSSL_free(hexbuf); - OPENSSL_free(expiry); + log_cert(ctx, 0, selected_cert, which); } else { UTIL_CTX_log(ctx, LOG_ERR, "No matching certificate returned.\n"); } From c60d806b228f1098fd521ce18928fa427e9bee6d Mon Sep 17 00:00:00 2001 From: olszomal Date: Wed, 15 Jul 2026 11:12:01 +0200 Subject: [PATCH 4/4] examples: Cleanse PKCS#11 PIN buffers after use --- examples/auth.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/examples/auth.c b/examples/auth.c index f2c0366f..04a04f93 100644 --- a/examples/auth.c +++ b/examples/auth.c @@ -67,7 +67,7 @@ int main(int argc, char *argv[]) unsigned char *random = NULL, *signature = NULL; - char password[20]; + char password[20] = { 0 }; int rc, fd, logged_in; unsigned int nslots, ncerts, siglen; @@ -168,7 +168,7 @@ int main(int argc, char *argv[]) /* perform pkcs #11 login */ rc = PKCS11_login(slot, 0, password); - memset(password, 0, strlen(password)); + OPENSSL_cleanse(password, sizeof(password)); if (rc != 0) { fprintf(stderr, "PKCS11_login failed\n"); rc = 10; @@ -285,6 +285,7 @@ int main(int argc, char *argv[]) rc = 0; failed: + OPENSSL_cleanse(password, sizeof(password)); if (rc) ERR_print_errors_fp(stderr); if (random != NULL)