util: fix deserialize buffer overflow

ChangeLog

@@ -4,6 +4,7 @@ Copyright (c) 2005-2022 Alon Bar-Lev <[email protected]>
 ????-??-?? - 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.

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,

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,66 +241,78 @@ _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;
 
 cleanup:
 
 	return rv;
+
+#undef __get_source
+#undef __add_target
 }
 

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)

tests/test-basic/test-serialize.c

@@ -0,0 +1,137 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include "../../config.h"
+#include <pkcs11-helper-1.0/pkcs11h-core.h>
+#include <pkcs11-helper-1.0/pkcs11h-certificate.h>
+
+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;
+}