diff --git a/ChangeLog b/ChangeLog index c97f083d..2d2489a2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -4,6 +4,7 @@ Copyright (c) 2005-2022 Alon Bar-Lev ????-??-?? - Version 1.31.0 * threading: fix mutex handling for cond_wait, thanks to Gleb Popov. * mbed: initialize certificate early using mbedtls_x509_crt_init. +* util: fix deserialize buffer overflow. thanks to Aarnav Bos. 2023-12-01 - Version 1.30.0 * core: add dynamic loader provider attribute, thanks to Marc Becker. diff --git a/lib/pkcs11h-serialization.c b/lib/pkcs11h-serialization.c index 74b4ca71..1fc1d538 100644 --- a/lib/pkcs11h-serialization.c +++ b/lib/pkcs11h-serialization.c @@ -415,6 +415,11 @@ pkcs11h_certificate_deserializeCertificateId ( certificate_id->attrCKA_ID_size = strlen (p)/2; + if (certificate_id->attrCKA_ID_size == 0) { + rv = CKR_ATTRIBUTE_VALUE_INVALID; + goto cleanup; + } + if ( (rv = _pkcs11h_mem_malloc ( (void *)&certificate_id->attrCKA_ID, diff --git a/lib/pkcs11h-util.c b/lib/pkcs11h-util.c index 7325db44..1fcadfc1 100644 --- a/lib/pkcs11h-util.c +++ b/lib/pkcs11h-util.c @@ -51,6 +51,20 @@ #include "common.h" #include "_pkcs11h-util.h" +static +int +_ctoi(const char c) { + if ('0' <= c && c <= '9') { + return c - '0'; + } else if ('A' <= c && c <= 'F') { + return c - 'A' + 10; + } else if ('a' <= c && c <= 'f') { + return c - 'a' + 10; + } else { + return -1; + } +} + void _pkcs11h_util_fixupFixedString ( OUT char * const target, /* MUST BE >= length+1 */ @@ -78,6 +92,7 @@ _pkcs11h_util_hexToBinary ( IN const char * const source, IN OUT size_t * const p_target_size ) { + CK_RV ret = CKR_ATTRIBUTE_VALUE_INVALID; size_t target_max_size; const char *p; char buf[3] = {'\0', '\0', '\0'}; @@ -92,29 +107,31 @@ _pkcs11h_util_hexToBinary ( *p_target_size = 0; while (*p != '\x0' && *p_target_size < target_max_size) { - if (isxdigit ((unsigned char)*p)) { - buf[i%2] = *p; + int b1, b2; - if ((i%2) == 1) { - unsigned v; - if (sscanf (buf, "%x", &v) != 1) { - v = 0; - } - target[*p_target_size] = (char)(v & 0xff); - (*p_target_size)++; - } + if ((b1 = _ctoi(*p)) == -1) { + goto cleanup; + } + p++; - i++; + if ((b2 = _ctoi(*p)) == -1) { + goto cleanup; } p++; + + target[*p_target_size] = (char)((b1 << 4) | b2); + (*p_target_size)++; } if (*p != '\x0') { - return CKR_ATTRIBUTE_VALUE_INVALID; - } - else { - return CKR_OK; + goto cleanup; } + + ret = CKR_OK; + +cleanup: + + return ret; } CK_RV @@ -224,60 +241,69 @@ _pkcs11h_util_unescapeString ( CK_RV rv = CKR_FUNCTION_FAILED; const char *s = source; char *t = target; + size_t m = *max; size_t n = 0; /*_PKCS11H_ASSERT (target!=NULL); Not required*/ _PKCS11H_ASSERT (source!=NULL); _PKCS11H_ASSERT (max!=NULL); +#define __get_source(b) \ + do { \ + if (*s == '\0') { \ + rv = CKR_ATTRIBUTE_VALUE_INVALID; \ + goto cleanup; \ + } \ + b = *s; \ + s++; \ + } while(0) + +#define __add_target(c) \ + do { \ + if (t != NULL) { \ + if (n >= m) { \ + rv = CKR_ATTRIBUTE_VALUE_INVALID; \ + goto cleanup; \ + } \ + *t = (c); \ + t++; \ + } \ + n++; \ + } while(0) + while (*s != '\x0') { if (*s == '\\') { - if (t != NULL) { - if (n+1 > *max) { - rv = CKR_ATTRIBUTE_VALUE_INVALID; - goto cleanup; - } - else { - char b[3]; - unsigned u; - b[0] = s[2]; - b[1] = s[3]; - b[2] = '\x0'; - sscanf (b, "%08x", &u); - *t = (char)(u & 0xff); - t++; - } - } - s+=4; - } - else { - if (t != NULL) { - if (n+1 > *max) { - rv = CKR_ATTRIBUTE_VALUE_INVALID; - goto cleanup; - } - else { - *t = *s; - t++; - } + int bin; + int b1, b2; + + __get_source(bin); + + __get_source(bin); + if (bin != 'x') { + rv = CKR_ATTRIBUTE_VALUE_INVALID; + goto cleanup; } - s++; - } - n+=1; - } + __get_source(bin); + if ((b1 = _ctoi(bin)) == -1) { + rv = CKR_ATTRIBUTE_VALUE_INVALID; + goto cleanup; + } - if (t != NULL) { - if (n+1 > *max) { - rv = CKR_ATTRIBUTE_VALUE_INVALID; - goto cleanup; - } - else { - *t = '\x0'; - t++; + __get_source(bin); + if ((b2 = _ctoi(bin)) == -1) { + rv = CKR_ATTRIBUTE_VALUE_INVALID; + goto cleanup; + } + __add_target((b1 << 4) | b2); + } else { + int bin; + __get_source(bin); + __add_target(bin); } } - n++; + + __add_target('\0'); *max = n; rv = CKR_OK; @@ -285,5 +311,8 @@ _pkcs11h_util_unescapeString ( cleanup: return rv; + +#undef __get_source +#undef __add_target } diff --git a/tests/test-basic/Makefile.am b/tests/test-basic/Makefile.am index 29494e1f..1a7ef0bd 100644 --- a/tests/test-basic/Makefile.am +++ b/tests/test-basic/Makefile.am @@ -53,6 +53,7 @@ MAINTAINERCLEANFILES=$(srcdir)/Makefile.in MY_TESTS = \ test-basic \ test-basic2 \ + test-serialize \ $(NULL) TESTS=$(MY_TESTS) diff --git a/tests/test-basic/test-serialize.c b/tests/test-basic/test-serialize.c new file mode 100644 index 00000000..5bfd2b27 --- /dev/null +++ b/tests/test-basic/test-serialize.c @@ -0,0 +1,137 @@ +#include +#include +#include +#include "../../config.h" +#include +#include + +static +void +fatal (const char * const m, CK_RV rv) { + fprintf (stderr, "%s - %08lu - %s\n", m, rv, pkcs11h_getMessage (rv)); + exit (1); +} + +static +void +_pkcs11h_hooks_log ( + IN void * const global_data, + IN unsigned flags, + IN const char * const format, + IN va_list args +) { + vfprintf (stdout, format, args); + fprintf (stdout, "\n"); + fflush (stdout); +} + +static +CK_RV +_deserialize ( + const char * const str, + const PKCS11H_BOOL should_success +) { + pkcs11h_certificate_id_t id = NULL; + char *p = NULL; + size_t max; + CK_RV ret; + CK_RV rv; + + ret = pkcs11h_certificate_deserializeCertificateId ( + &id, + str + ); + + if (should_success && ret != CKR_OK) { + fprintf(stderr, "String: %s\n", str); + fatal("Should succeed", rv); + } + if (!should_success && ret == CKR_OK) { + fprintf(stderr, "String: %s\n", str); + fatal("Should fail", rv); + } + + if (id != NULL) { + if ((rv = pkcs11h_certificate_serializeCertificateId ( + NULL, + &max, + id + )) != CKR_OK) { + fatal("pkcs11h_certificate_serializeCertificateId", rv); + } + + if ((p = malloc(max)) == NULL) { + fatal("malloc", CKR_HOST_MEMORY); + } + + if ((rv = pkcs11h_certificate_serializeCertificateId ( + p, + &max, + id + )) != CKR_OK) { + fatal("pkcs11h_certificate_serializeCertificateId(2)", rv); + } + + if (strcmp(p, str)) { + fprintf(stderr, "in: '%s' construct: '%s'\n", str, p); + fatal("Fail serialize compare", CKR_FUNCTION_FAILED); + } + } + + if (p != NULL) { + free(p); + } + + if (id != NULL) { + pkcs11h_certificate_freeCertificateId(id); + } + + return ret; +} + +int main () { + CK_RV rv; + int i; + + printf ("Initializing pkcs11-helper\n"); + + if ((rv = pkcs11h_initialize ()) != CKR_OK) { + fatal ("pkcs11h_initialize failed", rv); + } + + printf ("Registering pkcs11-helper hooks\n"); + + if ((rv = pkcs11h_setLogHook (_pkcs11h_hooks_log, NULL)) != CKR_OK) { + fatal ("pkcs11h_setLogHook failed", rv); + } + + pkcs11h_setLogLevel (TEST_LOG_LEVEL); + + printf ("Sanity\n"); + + _deserialize("manufacturer/model/serial/label/02", TRUE); + + _deserialize("manufacturer/model/serial/label/0z", FALSE); + + _deserialize("manufa\\xggcturer/model/serial/label/02", FALSE); + + printf ("Components\n"); + + const char *failure_string = "manu\\xff/model/serial/label\xee/0"; + char buffer[1024]; + + for (i = 0; i < strlen(failure_string); i++) { + strcpy(buffer, failure_string); + buffer[i] = '\0'; + _deserialize(buffer, FALSE); + } + + printf ("Terminating pkcs11-helper\n"); + + if ((rv = pkcs11h_terminate ()) != CKR_OK) { + fatal ("pkcs11h_terminate failed", rv); + } + + exit (0); + return 0; +}