diff options
author | Stef Walter <stefw@gnome.org> | 2013-03-20 15:53:43 +0100 |
---|---|---|
committer | Stef Walter <stefw@gnome.org> | 2013-03-20 16:22:35 +0100 |
commit | 0ecabc858dd6c1c2055f53202a01251e2ad7d2c2 (patch) | |
tree | 69184001695208399944f3fc5c9cd9b4fd692e98 | |
parent | e075585ef1cffc988894b4efbf3d14d5e55dcdcc (diff) | |
download | p11-kit-0ecabc858dd6c1c2055f53202a01251e2ad7d2c2.tar.gz |
trust: Predictable behavior with duplicate certificates in token
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
-rw-r--r-- | trust/parser.c | 94 | ||||
-rw-r--r-- | trust/tests/test-parser.c | 127 | ||||
-rw-r--r-- | trust/token.c | 19 |
3 files changed, 224 insertions, 16 deletions
diff --git a/trust/parser.c b/trust/parser.c index 91fd5e8..7ea879a 100644 --- a/trust/parser.c +++ b/trust/parser.c @@ -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); } diff --git a/trust/tests/test-parser.c b/trust/tests/test-parser.c index 4c182a0..94cfc49 100644 --- a/trust/tests/test-parser.c +++ b/trust/tests/test-parser.c @@ -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); diff --git a/trust/token.c b/trust/token.c index e0c2089..b562788 100644 --- a/trust/token.c +++ b/trust/token.c @@ -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; } |