summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStef Walter <stefw@gnome.org>2013-03-29 13:40:44 +0100
committerStef Walter <stefw@gnome.org>2013-04-03 15:39:37 +0200
commit91aa0f9623e232fa253308c4f7464dab8902dfea (patch)
treed0b0bf1b012951bbd5c906aaae7edbf5b6655bd6
parenta63311a0f3f2669138d09ff8f618fd4d12fa0c3d (diff)
downloadp11-kit-91aa0f9623e232fa253308c4f7464dab8902dfea.tar.gz
trust: Fix logic for matching invalid NSS serial numbers
Sometimes NSS queries for trust objects using invalid serial numbers that do not have their DER decoding. We fixed this earlier, but want to make sure there are no corner cases, accidentally not matching serial numbers that happen to start with the same bytes as a DER TLV would.
-rw-r--r--trust/module.c120
-rw-r--r--trust/tests/test-module.c107
2 files changed, 180 insertions, 47 deletions
diff --git a/trust/module.c b/trust/module.c
index e7eff62..7595ba1 100644
--- a/trust/module.c
+++ b/trust/module.c
@@ -1122,49 +1122,6 @@ sys_C_SetAttributeValue (CK_SESSION_HANDLE handle,
return rv;
}
-static CK_ATTRIBUTE *
-work_around_broken_nss_serial_number_lookups (CK_ATTRIBUTE *attrs)
-{
- /*
- * WORKAROUND: NSS calls us asking for CKA_SERIAL_NUMBER items that are
- * not DER encoded. It shouldn't be doing this. We never return any certificate
- * serial numbers that are not DER encoded.
- *
- * So work around the issue here while the NSS guys fix this issue.
- * This code should be removed in future versions.
- */
-
- CK_OBJECT_CLASS klass;
- CK_ATTRIBUTE *serial;
- unsigned char *der;
- size_t der_len;
- int len_len;
-
- if (!p11_attrs_find_ulong (attrs, CKA_CLASS, &klass) ||
- klass != CKO_NSS_TRUST)
- return attrs;
-
- serial = p11_attrs_find_valid (attrs, CKA_SERIAL_NUMBER);
- if (!serial || p11_asn1_tlv_length (serial->pValue, serial->ulValueLen) >= 0)
- return attrs;
-
- p11_debug ("working around serial number lookup that's not DER encoded");
-
- /* Assumption that 32 bytes is more than enough to store a ulong */
- der_len = 1 + 32 + serial->ulValueLen;
- der = malloc (der_len);
- return_val_if_fail (der != NULL, NULL);
-
- der[0] = ASN1_TAG_INTEGER | ASN1_CLASS_UNIVERSAL;
- len_len = der_len - 1;
- asn1_length_der (serial->ulValueLen, der + 1, &len_len);
- assert (len_len < der_len - serial->ulValueLen);
- memcpy (der + 1 + len_len, serial->pValue, serial->ulValueLen);
- der_len = 1 + len_len + serial->ulValueLen;
-
- return p11_attrs_take (attrs, CKA_SERIAL_NUMBER, der, der_len);
-}
-
static CK_RV
sys_C_FindObjectsInit (CK_SESSION_HANDLE handle,
CK_ATTRIBUTE_PTR template,
@@ -1218,9 +1175,6 @@ sys_C_FindObjectsInit (CK_SESSION_HANDLE handle,
find->match = p11_attrs_buildn (NULL, template, count);
warn_if_fail (find->match != NULL);
- find->match = work_around_broken_nss_serial_number_lookups (find->match);
- warn_if_fail (find->match != NULL);
-
/* Build a session snapshot of all objects */
find->iterator = 0;
find->snapshot = p11_index_snapshot (indices[0], indices[1], template, count);
@@ -1240,6 +1194,78 @@ sys_C_FindObjectsInit (CK_SESSION_HANDLE handle,
return rv;
}
+static bool
+match_for_broken_nss_serial_number_lookups (CK_ATTRIBUTE *attr,
+ CK_ATTRIBUTE *match)
+{
+ unsigned char der[32];
+ unsigned char *val_val;
+ size_t der_len;
+ size_t val_len;
+ int len_len;
+
+ if (!match->pValue || !match->ulValueLen ||
+ match->ulValueLen == CKA_INVALID ||
+ attr->ulValueLen == CKA_INVALID)
+ return false;
+
+ der_len = sizeof (der);
+ der[0] = ASN1_TAG_INTEGER | ASN1_CLASS_UNIVERSAL;
+ len_len = der_len - 1;
+ asn1_length_der (match->ulValueLen, der + 1, &len_len);
+ assert (len_len < (der_len - 1));
+ der_len = 1 + len_len;
+
+ val_val = attr->pValue;
+ val_len = attr->ulValueLen;
+
+ if (der_len + match->ulValueLen != val_len)
+ return false;
+
+ if (memcmp (der, val_val, der_len) != 0 ||
+ memcmp (match->pValue, val_val + der_len, match->ulValueLen) != 0)
+ return false;
+
+ p11_debug ("worked around serial number lookup that's not DER encoded");
+ return true;
+}
+
+static bool
+find_objects_match (CK_ATTRIBUTE *attrs,
+ CK_ATTRIBUTE *match)
+{
+ CK_OBJECT_CLASS klass;
+ CK_ATTRIBUTE *attr;
+
+ for (; !p11_attrs_terminator (match); match++) {
+ attr = p11_attrs_find ((CK_ATTRIBUTE *)attrs, match->type);
+ if (!attr)
+ return false;
+ if (p11_attr_equal (attr, match))
+ continue;
+
+ /*
+ * WORKAROUND: NSS calls us asking for CKA_SERIAL_NUMBER items that are
+ * not DER encoded. It shouldn't be doing this. We never return any certificate
+ * serial numbers that are not DER encoded.
+ *
+ * So work around the issue here while the NSS guys fix this issue.
+ * This code should be removed in future versions.
+ */
+
+ if (attr->type == CKA_SERIAL_NUMBER &&
+ p11_attrs_find_ulong (attrs, CKA_CLASS, &klass) &&
+ klass == CKO_NSS_TRUST) {
+ if (match_for_broken_nss_serial_number_lookups (attr, match))
+ continue;
+ }
+
+ return false;
+ }
+
+ return true;
+}
+
static CK_RV
sys_C_FindObjects (CK_SESSION_HANDLE handle,
CK_OBJECT_HANDLE_PTR objects,
@@ -1280,7 +1306,7 @@ sys_C_FindObjects (CK_SESSION_HANDLE handle,
if (attrs == NULL)
continue;
- if (p11_attrs_match (attrs, find->match)) {
+ if (find_objects_match (attrs, find->match)) {
objects[matched] = object;
matched++;
}
diff --git a/trust/tests/test-module.c b/trust/tests/test-module.c
index 4facf3b..16d8037 100644
--- a/trust/tests/test-module.c
+++ b/trust/tests/test-module.c
@@ -639,6 +639,46 @@ test_session_find (CuTest *cu)
}
static void
+test_session_find_no_attr (CuTest *cu)
+{
+ CK_ATTRIBUTE original[] = {
+ { CKA_CLASS, &data, sizeof (data) },
+ { CKA_LABEL, "yay", 3 },
+ { CKA_VALUE, "eight", 5 },
+ { CKA_INVALID }
+ };
+
+ CK_ATTRIBUTE match[] = {
+ { CKA_COLOR, "blah", 4 },
+ { CKA_INVALID }
+ };
+
+ CK_SESSION_HANDLE session;
+ CK_OBJECT_HANDLE handle;
+ CK_OBJECT_HANDLE check;
+ CK_ULONG count;
+ CK_RV rv;
+
+ setup (cu);
+
+ rv = test.module->C_OpenSession (test.slots[0], CKF_SERIAL_SESSION, NULL, NULL, &session);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ rv = test.module->C_CreateObject (session, original, 3, &handle);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ rv = test.module->C_FindObjectsInit (session, match, 1);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ rv = test.module->C_FindObjects (session, &check, 1, &count);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ CuAssertIntEquals (cu, 0, count);
+ rv = test.module->C_FindObjectsFinal (session);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ teardown (cu);
+}
+
+static void
test_lookup_invalid (CuTest *cu)
{
CK_SESSION_HANDLE session;
@@ -873,6 +913,71 @@ test_find_serial_der_decoded (CuTest *cu)
}
static void
+test_find_serial_der_mismatch (CuTest *cu)
+{
+ CK_OBJECT_CLASS nss_trust = CKO_NSS_TRUST;
+
+ CK_ATTRIBUTE object[] = {
+ { CKA_CLASS, &nss_trust, sizeof (nss_trust) },
+ { CKA_SERIAL_NUMBER, "\x02\x03\x01\x02\x03", 5 },
+ { CKA_INVALID }
+ };
+
+ CK_ATTRIBUTE match[] = {
+ { CKA_SERIAL_NUMBER, NULL, 0 },
+ { CKA_CLASS, &nss_trust, sizeof (nss_trust) },
+ { CKA_INVALID }
+ };
+
+ CK_SESSION_HANDLE session;
+ CK_OBJECT_HANDLE handle;
+ CK_OBJECT_HANDLE check;
+ CK_ULONG count;
+ CK_RV rv;
+
+ setup (cu);
+
+ rv = test.module->C_OpenSession (test.slots[0], CKF_SERIAL_SESSION, NULL, NULL, &session);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ rv = test.module->C_CreateObject (session, object, 2, &handle);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ /* Do a find with a null serial number, no match */
+ rv = test.module->C_FindObjectsInit (session, match, 2);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ rv = test.module->C_FindObjects (session, &check, 1, &count);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ CuAssertIntEquals (cu, 0, count);
+ rv = test.module->C_FindObjectsFinal (session);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ /* Do a find with a wrong length, no match */
+ match[0].pValue = "at";
+ match[0].ulValueLen = 2;
+ rv = test.module->C_FindObjectsInit (session, match, 2);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ rv = test.module->C_FindObjects (session, &check, 1, &count);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ CuAssertIntEquals (cu, 0, count);
+ rv = test.module->C_FindObjectsFinal (session);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ /* Do a find with a right length, wrong value, no match */
+ match[0].pValue = "one";
+ match[0].ulValueLen = 3;
+ rv = test.module->C_FindObjectsInit (session, match, 2);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ rv = test.module->C_FindObjects (session, &check, 1, &count);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+ CuAssertIntEquals (cu, 0, count);
+ rv = test.module->C_FindObjectsFinal (session);
+ CuAssertIntEquals (cu, CKR_OK, rv);
+
+ teardown (cu);
+}
+
+static void
test_login_logout (CuTest *cu)
{
CK_SESSION_HANDLE session;
@@ -916,10 +1021,12 @@ main (void)
SUITE_ADD_TEST (suite, test_setattr_token);
SUITE_ADD_TEST (suite, test_session_object);
SUITE_ADD_TEST (suite, test_session_find);
+ SUITE_ADD_TEST (suite, test_session_find_no_attr);
SUITE_ADD_TEST (suite, test_session_copy);
SUITE_ADD_TEST (suite, test_session_remove);
SUITE_ADD_TEST (suite, test_session_setattr);
SUITE_ADD_TEST (suite, test_find_serial_der_decoded);
+ SUITE_ADD_TEST (suite, test_find_serial_der_mismatch);
SUITE_ADD_TEST (suite, test_login_logout);
CuSuiteRun (suite);