PAM: Multiple certificates on a Smartcard

[sumit-bose]

With this patch set SSSD will ask the user to select a certificate if multiple
certificates suitable for authentication are found on a Smartcard.

The first three patches are basically refactoring to use a list of structs
instead of individual variables. The third also adds the PAM conversations to
select the certificate.

The fourth patch makes sure that p11_child can address a specific certificate
as well.

The fifth adds the string shown to the user to identify a certificate
in the selection dialog.

The sixth makes sure the user name is available when the authentication request
is send to the backend.

Finally the seventh patch adds error messages explaining the NSS error codes in
p11_child to make debugging easier.

Resolves https://pagure.io/SSSD/sssd/issue/3560

[fidencio]

@sumit-bose, as we discussed in our internal meeting, someone from our QA team would do the functional testing.

So far I've taken a look at the code and came up with the following changes: https://github.com/fidencio/sssd/tree/review/multiple_certs

The most part of the fixup patches are minors and could be ignored but "fixup! pam_sss: refactoring, use struct cert_auth_info" would be worth to have squashed.

I'll run coverity in those patches and wait for QA ack as well. Okay?

[fidencio]

So, coverity found some issues:

• Error: USE_AFTER_FREE (CWE-825): [#def1]

sssd-1.16.1/src/sss_client/pam_sss.c:154: alias: Assigning: "cai" = "cai->next". Now both point to the same storage.
sssd-1.16.1/src/sss_client/pam_sss.c:155: freed_arg: "free_cai" frees "cai".
sssd-1.16.1/src/sss_client/pam_sss.c:145:9: freed_arg: "free" frees parameter "cai".
sssd-1.16.1/src/sss_client/pam_sss.c:154: deref_after_free: Dereferencing freed pointer "cai".
#  152|   
#  153|       if (list != NULL) {
#  154|->         DLIST_FOR_EACH(cai, list) {
#  155|               free_cai(cai);
#  156|           }

Here it seems that a DLIST_FOR_EACH_SAFE() should be used.
Something like:

diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index b2968634e..e6d559b44 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -148,10 +148,11 @@ static void free_cai(struct cert_auth_info *cai)

 static void free_cert_list(struct cert_auth_info *list)
 {
-    struct cert_auth_info *cai;
+    struct cert_auth_info *cai, *cai_next;

     if (list != NULL) {
-        DLIST_FOR_EACH(cai, list) {
+        DLIST_FOR_EACH_SAFE(cai, cai_next, list) {
+            DLIST_REMOVE(list, cai);
             free_cai(cai);
         }
     }

• Error: FORWARD_NULL (CWE-476): [#def2]

sssd-1.16.1/src/responder/pam/pamsrv_p11.c:433: var_compare_op: Comparing "pd->authtok" to null implies that "pd->authtok" might be null.
sssd-1.16.1/src/responder/pam/pamsrv_p11.c:461: var_deref_model: Passing null pointer "pd->authtok" to "sss_authtok_get_type", which dereferences it.
sssd-1.16.1/src/util/authtok.c:30:5: deref_parm: Directly dereferencing parameter "tok".
#   28|   enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token *tok)
#   29|   {
#   30|->     return tok->type;
#   31|   }
#   32|   

This one could either be solved by a simple check on the called to ensure that tok is not NULL or by the suggested patch below:

diff --git a/src/util/authtok.c b/src/util/authtok.c
index c2f78be32..2c5a26ce3 100644
--- a/src/util/authtok.c
+++ b/src/util/authtok.c
@@ -27,6 +27,10 @@ struct sss_auth_token {

 enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token *tok)
 {
+    if (tok == NULL) {
+        return SSS_AUTHTOK_TYPE_EMPTY;
+    }
+
     return tok->type;
 }

The patch above would also avoid a check done as part of the one patches to ensure that pd->authtok != NULL

[fidencio]

I have updated the comments in order to reflect on what we discussed on IRC.

[sumit-bose]

Thank you for the review, I include all your comments in the latest version.

[fidencio]

After a few tries to run coverity in the new code I've finally able to and seems that no new issues have been introduced.

You have an ACK from me. @jhrozek would like to run some downstream tests (please, @jhrozek, let us know about the result as soon as you have those) and after that and Scott ACK this PR will be ready to go.

@sumit-bose, please, let us know about Scott's tests results.

[sumit-bose]

Hi Fabiano,

thank you for the review. Scott's test are not done yet. He found that just showing the subject-DN to select a certificate is not sufficient for some types of Smartcards where all certificates will have the same subject-DN. I prepared a new build for him which he hopefully can test today.

[fidencio]

Okay, let me know when you update the patches and I'll do a third round of review then.
@jhrozek, I'd suggest you to wait with the downstream tests till we have the ACK from Scott.

[sumit-bose]

I just updates the patches to the latest version which got a positive feedback from Scott. The changes are mostly in the two new patches.

[fidencio]

Okay. I'm firing our internal CI and I'll try to go through these patches before lunch.

[jhrozek]

Please let me know if I should re-run some of the downstream tests I ran the other day. Although you should be able to do that as well if you have the job IDs around -- just cloning those after you build the new test repo should do the trick.

[fidencio]

@jhrozek, don't worry, I'll run the downstream tests based on the job IDs you pointed me to.

[fidencio]

So, code-wise I have just a few nitpicks related to the new patches. Feel free to pick up the changes or ignore them if you don't think they're valid:

[ffidenci@pessoa sssd]$ cat 0001-fixup-pam-filter-certificates-in-the-responder-not-i.patch 
From b881194ae06e4e835f84d5bfbdf2924e5b2b7b15 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Wed, 8 Nov 2017 12:26:23 +0100
Subject: [PATCH] fixup! pam: filter certificates in the responder not in the
 child

---
 src/responder/pam/pamsrv_p11.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index 6386954ae..c2eccb1f3 100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -157,7 +157,7 @@ errno_t p11_refresh_certmap_ctx(struct pam_ctx *pctx,
         /* Try to add default matching rule */
         ret = sss_certmap_add_rule(sss_certmap_ctx, SSS_CERTMAP_MIN_PRIO,
                                    CERT_AUTH_DEFAULT_MATCHING_RULE, NULL, NULL);
-        if (ret != 0) {
+        if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "Failed to add default matching rule.\n");
         }
@@ -176,7 +176,7 @@ errno_t p11_refresh_certmap_ctx(struct pam_ctx *pctx,
                                    certmap_list[c]->match_rule,
                                    certmap_list[c]->map_rule,
                                    certmap_list[c]->domains);
-        if (ret != 0) {
+        if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "sss_certmap_add_rule failed for rule [%s] "
                   "with error [%d][%s], skipping. "
@@ -463,7 +463,7 @@ static errno_t parse_p11_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf,
         }
 
         ret = sss_certmap_match_cert(sss_certmap_ctx, der, der_size);
-        if (ret == 0) {
+        if (ret == EOK) {
             DLIST_ADD(cert_list, cert_auth_info);
         } else {
             DEBUG(SSSDBG_TRACE_LIBS,
-- 
2.13.6

Covscan came up without any issues and I'm just waiting for the results of the downstream tests to ACK the patches.

[sumit-bose]

@fidencio, thank you for the review. I used '0' instead of EOK here on purpose. Although in the SSSD source tree the sss_certmap_* calls are coming from an external library and the return code for success is '0' as documented in src/lib/certmap/sss_certmap.h.

EOK is used by SSSD's internal calls and is currently defined as '0'. But in theory it should be possible to set this to any int value and SSSD should keep working. I pretty sure this would currently break heavily but that why I would prefer to not use EOK for calls from external libraries.

[fidencio]

@sumit-bose, makes sense :-) So, no more comments from me and I'm just waiting for the results of downstream tests.

[fidencio]

@sumit-bose, last thing ... running our internal CI has been a little problematic due to the amount of dependencies pulled by gdm-devel.

I'd like to suggest to check for the OS version and add gdm-devel as a requirement only for el > 7.4 and fedora >= 27.

Does that make sense?

[fidencio]

Hmm. It should be enough:

[ffidenci@pessoa x86_64]$ cat 0001-fixup-pam_sss-refactoring-use-struct-cert_auth_info.patch
From 18286c3ffd14053909eca1114419c3a63009e9dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Wed, 8 Nov 2017 14:30:25 +0100
Subject: [PATCH] fixup! pam_sss: refactoring, use struct cert_auth_info

---
 contrib/sssd.spec.in | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
index b3ebd32cd..4a2745baa 100644
--- a/contrib/sssd.spec.in
+++ b/contrib/sssd.spec.in
@@ -121,6 +121,12 @@
     %global with_kcm_option --without-kcm
 %endif
 
+%if (0%{?fedora} >= 27 || (0%{?rhel} >= 7 && 0%{?rhel7_minor} > 4))
+    %global depend_on_gdm_devel 1
+%else
+    %global depend_on_gdm_devel 0
+%endif
+
 Name: @PACKAGE_NAME@
 Version: @PACKAGE_VERSION@
 Release: 0@PRERELEASE_VERSION@%{?dist}
@@ -233,7 +239,9 @@ BuildRequires: libuuid-devel
 BuildRequires: jansson-devel
 BuildRequires: libcurl-devel
 %endif
+%if (0%{?depend_on_gdm_devel} == 1)
 BuildRequires: gdm-devel
+%endif
 
 %description
 Provides a set of daemons to manage access to remote directories and
-- 
2.13.6

Just fired an internal CI build in order to have it tested.

[fidencio]

Okay, Debian testing has been failing on our CI and the following patch solves the issue:

[ffidenci@pessoa sssd]$ cat 0001-fixup-PAM-handled-multiple-certs-in-the-responder.patch 
From 0ce79d843545cb939b891cdf66b1dbc8133972a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Wed, 8 Nov 2017 15:47:01 +0100
Subject: [PATCH] fixup! PAM: handled multiple certs in the responder

---
 Makefile.am | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 5296c4ca0..57e03abfe 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1401,6 +1401,7 @@ sssd_pam_LDADD = \
     $(PAM_LIBS) \
     $(SYSTEMD_DAEMON_LIBS) \
     $(SSSD_INTERNAL_LTLIBS) \
+    libsss_certmap.la \
     $(NULL)
 
 if BUILD_SUDO
@@ -2423,6 +2424,7 @@ pam_srv_tests_LDADD = \
     $(SYSTEMD_DAEMON_LIBS) \
     libsss_test_common.la \
     libsss_idmap.la \
+    libsss_certmap.la \
     $(NULL)
 
 EXTRA_responder_get_domains_tests_DEPENDENCIES = \
-- 
2.13.6

You can find the two patches that I mentioned at: https://github.com/fidencio/sssd/tree/review/multiple_certs

[sumit-bose]

The latest version include both fixes, I just renamed the spec file global.

[fidencio]

Okay, all the downstream tests passed (one of the them only in the second run, though).

ACK!

[fidencio]

Please, whoever pushes this patch set, add Scott Poore as reviewer as well.

[lslebodn]

Just a note. I would like to avoid following change:

diff --git a/src/util/authtok.c b/src/util/authtok.c
index c2f78be32..2c5a26ce3 100644
--- a/src/util/authtok.c
+++ b/src/util/authtok.c
@@ -27,6 +27,10 @@ struct sss_auth_token {

 enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token *tok)
 {
+    if (tok == NULL) {
+        return SSS_AUTHTOK_TYPE_EMPTY;
+    }
+
     return tok->type;
 }

We need to properly initialise authtok in all cases. It must not be NULL
BTW following report from static analysers part is outdated:

    Error: FORWARD_NULL (CWE-476): [#def2]

sssd-1.16.1/src/responder/pam/pamsrv_p11.c:433: var_compare_op: Comparing "pd->authtok" to null implies that "pd->authtok" might be null.
sssd-1.16.1/src/responder/pam/pamsrv_p11.c:461: var_deref_model: Passing null pointer "pd->authtok" to "sss_authtok_get_type", which dereferences it.
sssd-1.16.1/src/util/authtok.c:30:5: deref_parm: Directly dereferencing parameter "tok".
#   28|   enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token *tok)
#   29|   {
#   30|->     return tok->type;
#   31|   }
#   32|   

[fidencio]

So, I've just done some tests here and seems that we can just drop "authtok: check for NULL in sss_authtok_get_type()" patch, as long as we add the following fixup to the "p11_child: use options to select certificate for authentication" patch.

[ffidenci@pessoa x86_64]$ cat 0001-fixup-p11_child-use-options-to-select-certificate-fo.patch 
From 0d2ef9ac950d3539fa1ef0044790799d900f6a21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Fri, 10 Nov 2017 14:04:15 +0100
Subject: [PATCH] fixup! p11_child: use options to select certificate for
 authentication

---
 src/responder/pam/pamsrv_p11.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index 7f7b02b0d..27aeb9833 100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -428,10 +428,9 @@ struct tevent_req *pam_check_cert_send(TALLOC_CTX *mem_ctx,
         extra_args[arg_c++] = "--verify";
     }
 
-    if (pd->authtok != NULL
-            && (sss_authtok_get_type(pd->authtok) == SSS_AUTHTOK_TYPE_SC_PIN
-                || sss_authtok_get_type(pd->authtok) ==
-                                                  SSS_AUTHTOK_TYPE_SC_KEYPAD)) {
+    if (sss_authtok_get_type(pd->authtok) == SSS_AUTHTOK_TYPE_SC_PIN
+            || sss_authtok_get_type(pd->authtok) ==
+                                                  SSS_AUTHTOK_TYPE_SC_KEYPAD) {
         ret = sss_authtok_get_sc(pd->authtok, NULL, NULL, &token_name, NULL,
                                                           &module_name, NULL,
                                                           &key_id, NULL);
-- 
2.13.6

[fidencio]

@lslebodn, @sumit-bose, both of you agree on that? (so we can have it merged sooner than later :-))

[lslebodn]

@lslebodn, @sumit-bose, both of you agree on that? (so we can have it merged sooner than later :-))

Sure I suggested to drop the check.

[sumit-bose]

@fidencio, @lslebodn, the new version has both, the if-check and the sss_authtok patch, remove.

I'll enhance the sss_authtok patch and send it with a different PR.

[fidencio]

Covscan passed. ACK, and I'm adding the "Accepted" label.

[lslebodn]

whitespace unit test failed for me on my laptop

• I would like to re-run downstream tests with current master which contains fixes for few regressions. (therefore temporary removing ACK label)

[lslebodn]

FYI https://bugzilla.redhat.com/show_bug.cgi?id=1512212

[lslebodn]

We should check return value here and fail in case of NULL.
Because it is checked inly in done section which is too late

[lslebodn]

Can it happen that cert_count will be 0 ?

[sumit-bose]

It is checked before if pi->cert_list is NULL, so cert_count is at least one.

[lslebodn]

Clang says that cai can be NULL here

• call pam_sss
• line 2245 -> call get_pam_items
• line 1253 -> "pi->cert_list = NULL"
• return back from call get_pam_items
• line 2263 -> use case SSS_APM_AUTHENTICAte in switch
• line 2287 -> call check_login_token_name
• line 2170 -> "struct cert_auth_info *cai = pi->cert_list;"
• line 2177 -> note: Access to field 'token_name' results in a dereference of a null pointer (loaded from variable 'cai')

But I think it is false positive because we call send_and_receive-> eval_response -> parse_cert_info
What do you think?

[sumit-bose]

I added a check because we cannot be sure that the PAM responder did add the needed data.

[lslebodn]

BTW fix for whitespace on my laptop

echo "" >> src/tests/cmocka/p11_nssdb_2certs/key4.db
echo "" >> src/tests/cmocka/p11_nssdb_2certs/cert9.db

it was simpler than adding it to blacklist and unit test passed

[sumit-bose]

The latest version fixes @lslebodn's comment and modifies a filter in the whitespace test so that the added NSS database files are skipped as well.

[lslebodn]

master:

• 57cefea
• 177ab84
• 08d1f8c
• fd6f404
• 06c2300
• 0a8024a
• 122830e
• 0bdd880
• 39fd336