trust: Predictable behavior with duplicate certificates in token
authorStef Walter <stefw@gnome.org>
Wed, 20 Mar 2013 14:53:43 +0000 (15:53 +0100)
committerStef Walter <stefw@gnome.org>
Wed, 20 Mar 2013 15:22:35 +0000 (16:22 +0100)
If duplicate certificates are present in a token, we warn about this,
and don't really recommend it. However we have predictable behavior
where blacklist is prefered to anchor is preferred to unknown trust.

https://bugs.freedesktop.org/show_bug.cgi?id=62548

trust/parser.c
trust/tests/test-parser.c
trust/token.c

index 91fd5e8baba4fc53e7f7560d4e9acffcd770f424..7ea879a7f445b0e9e7a026dea6364e316126bae3 100644 (file)
@@ -130,20 +130,112 @@ populate_trust (p11_parser *parser,
        return p11_attrs_build (attrs, &trusted, &distrust, NULL);
 }
 
+static bool
+lookup_cert_duplicate (p11_index *index,
+                       CK_ATTRIBUTE *attrs,
+                       CK_OBJECT_HANDLE *handle,
+                       CK_ATTRIBUTE **dupl)
+{
+       CK_OBJECT_CLASS klass = CKO_CERTIFICATE;
+       CK_ATTRIBUTE *value;
+
+       CK_ATTRIBUTE match[] = {
+               { CKA_VALUE, },
+               { CKA_CLASS, &klass, sizeof (klass) },
+               { CKA_INVALID },
+       };
+
+       /*
+        * TODO: This will need to be adapted when we support reload on
+        * the fly, but for now since we only load once, we can assume
+        * that any certs already present in the index are duplicates.
+        */
+
+       value = p11_attrs_find_valid (attrs, CKA_VALUE);
+       if (value != NULL) {
+               memcpy (match, value, sizeof (CK_ATTRIBUTE));
+               *handle = p11_index_find (index, match, -1);
+               if (*handle != 0) {
+                       *dupl = p11_index_lookup (index, *handle);
+                       return true;
+               }
+       }
+
+       return false;
+}
+
+static char *
+pull_cert_label (CK_ATTRIBUTE *attrs)
+{
+       char *label;
+       size_t len;
+
+       label = p11_attrs_find_value (attrs, CKA_LABEL, &len);
+       if (label)
+               label = strndup (label, len);
+
+       return label;
+}
+
+static int
+calc_cert_priority (CK_ATTRIBUTE *attrs)
+{
+       CK_BBOOL boolv;
+
+       enum {
+               PRI_UNKNOWN,
+               PRI_TRUSTED,
+               PRI_DISTRUST
+       };
+
+       if (p11_attrs_find_bool (attrs, CKA_X_DISTRUSTED, &boolv) && boolv)
+               return PRI_DISTRUST;
+       else if (p11_attrs_find_bool (attrs, CKA_TRUSTED, &boolv) && boolv)
+               return PRI_TRUSTED;
+
+       return PRI_UNKNOWN;
+}
+
 static void
 sink_object (p11_parser *parser,
              CK_ATTRIBUTE *attrs)
 {
+       CK_OBJECT_HANDLE handle;
        CK_OBJECT_CLASS klass;
+       CK_ATTRIBUTE *dupl;
+       char *label;
        CK_RV rv;
 
+       /* By default not replacing anything */
+       handle = 0;
+
        if (p11_attrs_find_ulong (attrs, CKA_CLASS, &klass) &&
            klass == CKO_CERTIFICATE) {
                attrs = populate_trust (parser, attrs);
                return_if_fail (attrs != NULL);
+
+               if (lookup_cert_duplicate (parser->index, attrs, &handle, &dupl)) {
+
+                       /* This is not a good place to be for a well configured system */
+                       label = pull_cert_label (dupl);
+                       p11_message ("duplicate '%s' certificate found in: %s",
+                                    label ? label : "?", parser->basename);
+                       free (label);
+
+                       /*
+                        * Nevertheless we provide predictable behavior about what
+                        * overrides what. If we have a lower or equal priority
+                        * to what's there, then just go away, otherwise replace.
+                        */
+                       if (calc_cert_priority (attrs) <= calc_cert_priority (dupl)) {
+                               p11_attrs_free (attrs);
+                               return;
+                       }
+               }
        }
 
-       rv = p11_index_take (parser->index, attrs, NULL);
+       /* If handle is zero, this just adds */
+       rv = p11_index_replace (parser->index, handle, attrs);
        if (rv != CKR_OK)
                p11_message ("couldn't load file into objects: %s", parser->basename);
 }
index 4c182a0bd20fc84a6887a4d367d2b7091687e1ea..94cfc49c5572aa8dd08343bb90ffb82013fa62a4 100644 (file)
@@ -437,6 +437,131 @@ test_parse_unrecognized (CuTest *cu)
        teardown (cu);
 }
 
+static void
+test_duplicate (CuTest *cu)
+{
+       CK_ATTRIBUTE cacert3[] = {
+               { CKA_CLASS, &certificate, sizeof (certificate) },
+               { CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) },
+               { CKA_VALUE, (void *)test_cacert3_ca_der, sizeof (test_cacert3_ca_der) },
+               { CKA_MODIFIABLE, &falsev, sizeof (falsev) },
+               { CKA_TRUSTED, &falsev, sizeof (falsev) },
+               { CKA_X_DISTRUSTED, &falsev, sizeof (falsev) },
+               { CKA_INVALID },
+       };
+
+       CK_OBJECT_HANDLE *handles;
+       CK_ATTRIBUTE *cert;
+       int ret;
+
+       setup (cu);
+
+       ret = p11_parse_file (test.parser, SRCDIR "/files/cacert3.der", 0);
+       CuAssertIntEquals (cu, P11_PARSE_SUCCESS, ret);
+
+       p11_message_quiet ();
+
+       /* This shouldn't be added, should print a message */
+       ret = p11_parse_file (test.parser, SRCDIR "/files/cacert3.der", 0);
+       CuAssertIntEquals (cu, P11_PARSE_SUCCESS, ret);
+
+       CuAssertTrue (cu, strstr (p11_message_last (), "duplicate") != NULL);
+
+       p11_message_loud ();
+
+       /* Should only be one certificate since the above two are identical */
+       handles = p11_index_find_all (test.index, cacert3, 2);
+       CuAssertPtrNotNull (cu, handles);
+       CuAssertTrue (cu, handles[0] != 0);
+       CuAssertTrue (cu, handles[1] == 0);
+
+       cert = p11_index_lookup (test.index, handles[0]);
+       test_check_attrs (cu, cacert3, cert);
+
+       free (handles);
+       teardown (cu);
+}
+
+static void
+test_duplicate_priority (CuTest *cu)
+{
+       CK_ATTRIBUTE cacert3[] = {
+               { CKA_CLASS, &certificate, sizeof (certificate) },
+               { CKA_VALUE, (void *)test_cacert3_ca_der, sizeof (test_cacert3_ca_der) },
+               { CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) },
+               { CKA_MODIFIABLE, &falsev, sizeof (falsev) },
+               { CKA_INVALID },
+       };
+
+       CK_ATTRIBUTE trusted[] = {
+               { CKA_CLASS, &certificate, sizeof (certificate) },
+               { CKA_VALUE, (void *)test_cacert3_ca_der, sizeof (test_cacert3_ca_der) },
+               { CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) },
+               { CKA_TRUSTED, &truev, sizeof (truev) },
+               { CKA_X_DISTRUSTED, &falsev, sizeof (falsev) },
+               { CKA_INVALID },
+       };
+
+       CK_ATTRIBUTE distrust[] = {
+               { CKA_CLASS, &certificate, sizeof (certificate) },
+               { CKA_VALUE, (void *)test_cacert3_ca_der, sizeof (test_cacert3_ca_der) },
+               { CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) },
+               { CKA_TRUSTED, &falsev, sizeof (falsev) },
+               { CKA_X_DISTRUSTED, &truev, sizeof (truev) },
+               { CKA_INVALID },
+       };
+
+       CK_OBJECT_HANDLE *handles;
+       CK_ATTRIBUTE *cert;
+       int ret;
+
+       setup (cu);
+
+       ret = p11_parse_file (test.parser, SRCDIR "/files/cacert3.der", 0);
+       CuAssertIntEquals (cu, P11_PARSE_SUCCESS, ret);
+
+       p11_message_quiet ();
+
+       /* This shouldn't be added, should print a message */
+       ret = p11_parse_file (test.parser, SRCDIR "/files/cacert3.der",
+                             P11_PARSE_FLAG_ANCHOR);
+       CuAssertIntEquals (cu, P11_PARSE_SUCCESS, ret);
+
+       CuAssertTrue (cu, strstr (p11_message_last (), "duplicate") != NULL);
+
+       p11_message_loud ();
+
+       /* We should now find the trusted certificate */
+       handles = p11_index_find_all (test.index, cacert3, 2);
+       CuAssertPtrNotNull (cu, handles);
+       CuAssertTrue (cu, handles[0] != 0);
+       CuAssertTrue (cu, handles[1] == 0);
+       cert = p11_index_lookup (test.index, handles[0]);
+       test_check_attrs (cu, trusted, cert);
+       free (handles);
+
+       /* Now add a distrutsed one, this should override the trusted */
+
+       p11_message_quiet ();
+
+       ret = p11_parse_file (test.parser, SRCDIR "/files/cacert3.der",
+                             P11_PARSE_FLAG_BLACKLIST);
+       CuAssertIntEquals (cu, P11_PARSE_SUCCESS, ret);
+
+       p11_message_loud ();
+
+       /* We should now find the distrusted certificate */
+       handles = p11_index_find_all (test.index, cacert3, 2);
+       CuAssertPtrNotNull (cu, handles);
+       CuAssertTrue (cu, handles[0] != 0);
+       CuAssertTrue (cu, handles[1] == 0);
+       cert = p11_index_lookup (test.index, handles[0]);
+       test_check_attrs (cu, distrust, cert);
+       free (handles);
+
+       teardown (cu);
+}
+
 int
 main (void)
 {
@@ -457,6 +582,8 @@ main (void)
        SUITE_ADD_TEST (suite, test_parse_thawte);
        SUITE_ADD_TEST (suite, test_parse_invalid_file);
        SUITE_ADD_TEST (suite, test_parse_unrecognized);
+       SUITE_ADD_TEST (suite, test_duplicate);
+       SUITE_ADD_TEST (suite, test_duplicate_priority);
 
        CuSuiteRun (suite);
        CuSuiteSummary (suite, output);
index e0c208950c92b2d3f6e358dd47b8e887e8d3d867..b562788558c4dac07f6813879e79a3aa45e2657b 100644 (file)
@@ -56,8 +56,6 @@
 #include <stdlib.h>
 #include <string.h>
 
-#define ELEMS(x) (sizeof (x) / sizeof (x[0]))
-
 struct _p11_token {
        p11_parser *parser;
        p11_index *index;
@@ -68,18 +66,6 @@ struct _p11_token {
        int loaded;
 };
 
-static void
-on_parser_object (CK_ATTRIBUTE *attrs,
-                  void *user_data)
-{
-       p11_token *token = user_data;
-
-       return_if_fail (attrs != NULL);
-
-       if (p11_index_take (token->index, attrs, NULL) != CKR_OK)
-               return_if_reached ();
-}
-
 static int
 loader_load_file (p11_token *token,
                   const char *filename,
@@ -211,6 +197,7 @@ load_builtin_objects (p11_token *token)
        CK_OBJECT_CLASS builtin = CKO_NSS_BUILTIN_ROOT_LIST;
        CK_BBOOL vtrue = CK_TRUE;
        CK_BBOOL vfalse = CK_FALSE;
+       CK_RV rv;
 
        const char *trust_anchor_roots = "Trust Anchor Roots";
        CK_ATTRIBUTE builtin_root_list[] = {
@@ -219,10 +206,12 @@ load_builtin_objects (p11_token *token)
                { CKA_PRIVATE, &vfalse, sizeof (vfalse) },
                { CKA_MODIFIABLE, &vfalse, sizeof (vfalse) },
                { CKA_LABEL, (void *)trust_anchor_roots, strlen (trust_anchor_roots) },
+               { CKA_INVALID },
        };
 
        p11_index_batch (token->index);
-       on_parser_object (p11_attrs_buildn (NULL, builtin_root_list, ELEMS (builtin_root_list)), token);
+       rv = p11_index_take (token->index, p11_attrs_dup (builtin_root_list), NULL);
+       return_val_if_fail (rv == CKR_OK, 0);
        p11_index_finish (token->index);
        return 1;
 }