[sssd PR#433][comment] PAM: Multiple certificates on a Smartcard
by fidencio
URL: https://github.com/SSSD/sssd/pull/433
Title: #433: PAM: Multiple certificates on a Smartcard
fidencio commented:
"""
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,11 +148,13 @@ 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, *iter, *tmp;
if (list != NULL) {
- DLIST_FOR_EACH(cai, list) {
- free_cai(cai);
+ DLIST_FOR_EACH_SAFE(cai, iter, list) {
+ tmp = cai;
+ DLIST_REMOVE(list, cai);
+ free_cai(tmp);
}
}
}
```
- **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`
"""
See the full comment at https://github.com/SSSD/sssd/pull/433#issuecomment-341727235
6 years, 6 months
[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by lslebodn
URL: https://github.com/SSSD/sssd/pull/432
Title: #432: CACHE_REQ: Better debugging for email conflicts
lslebodn commented:
"""
On (03/11/17 14:26), mzidek-rh wrote:
>mzidek-rh commented on this pull request.
>> @@ -218,6 +218,21 @@ int sysdb_getpwnam(TALLOC_CTX *mem_ctx,
> goto done;
> }
>
>+ if (res->count > 1) {
>+ /* We expected either 0 or 1 result for search with
>+ * SYSDB_PWNAM_FILTER, but we got more. This error
>+ * is handled individually depending on what function
>+ * called sysdb_getpwnam, so we just print a message
>+ * here and let the caller decide what error code to
>+ * propagate based on res->count > 1. */
>+ DEBUG(SSSDBG_CRIT_FAILURE,
>+ "Search for [%s] returned multiple results. It can be an email "
>+ "address shared among multiple users or an email address of a "
>+ "user that conflicts with another user's fully qualified name. "
>+ "SSSD will not be able to handle those users properly.\n",
>+ sanitized_name);
>+ }
>+
>
>I do not think it is a misuse of nameAlias, but having emailAlias could have some benefits. That discussion is however out of scope for this PR and refactoring of how we handle the emails in sysdb would also require update of sysdb version.
>
I do not think it is out of scope. The title of ticket is
"SSSD authentication fails when two IPA accounts share an email address"
You just underestimate the scope of ticket and want to solve it in hacky way.
>In your example with user123" with email "test(a)example.com" and "benutzer123" with email "TEST(a)example.com", it is clear conflict of emails. I do not see how is the message not helpful here. It clearly says that there may be users with conflicting emails.
>
yes, it is clear conflict of emails because you know about them.
But you would log only following message.
Search for benutzer123 returned multiple results. It can be an email
address shared among multiple users or an email address of a
user that conflicts with another user's fully qualified name.
SSSD will not be able to handle those users properly.
and if sb will try to search sssd cache by
ldbsearch -H /var/lib/sssd/db/cache_example.com.ldb \
"(mail=TEST(a)example.com)" dn,mail
it will return only 1 result.
You might ask why sb would like to use ldbsearch and answer is that there are
10000 users in cache and every user have email address.
The purpose of message is to simplify users job especially and current state
will not help a lot. Because message about more results would be visible
also on another place in sssd log with higher debug level.
BTW upstream ticket even mention also logging message to syslog.
https://pagure.io/SSSD/sssd/issue/3293#comment-456587
"""
See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341723700
6 years, 6 months