sssd-devel November 2017

sssd-devel@lists.fedorahosted.org

10 participants
210 discussions

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by lslebodn 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts lslebodn commented: """ On (03/11/17 16:19), mzidek-rh wrote: >No, I used good example. Your example is the wrong one and would not trigger the error. > There are more ways how you can store users with email to cache. The most important is that email is lowercased in backed and stored to nameAlias and not in responder. In theory, you can use even getpwuid. (I didn't try) But `sysdb_getpwnam` will find them because it uses filter `SYSDB_PWNAM_FILTER` >What do you think will happen if in my example scenario I search for 'getent passwd email2' ? It will return correct results. Do you see why? It is not about returning correct results. The purpose of ticket is to inform about conflicts (or even prevent such conflicts if possible) >I already answered it in one of my previous comments, but here I tell it again. Because in my example email2 (fqdn version email2(a)ipadomain.test) is just a username. >Same as your benutzer123 and user123. It will never trigger the error because they do not conflict with any email address of another user. > They have the same email. Just case is different (unless I did a typo) Therefore lowercased email in "nameAlias" will be the same and will return two users. >In your example, in order to trigger the error you would have to search for getent passwd lorem.ipsum(a)jmail.com. Then you would see the error message telling you that lorem.ipsum(a)jmail.com is is probably conflicting with another users email or fqdn. > As I mentioned about there are more ways how you can store user with email to sssd cache. The problem is that `sysdb_getpwnam` can return more users (e.g. `benutzer123` and `user123`) at the same time because lowercased email stored in "nameAlias" is the same. The other related problem is that `sysdb_getpwnam` should not indirectly search by email address. Search by email address should be done only in `sysdb_search_user_by_upn_res` which explicitly search either by UPN or by email address. LS """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341759495

1 0

[sssd PR#433][comment] PAM: Multiple certificates on a Smartcard
by fidencio 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/433 Title: #433: PAM: Multiple certificates on a Smartcard fidencio commented: """ I have updated the comments in order to reflect on what we discussed on IRC. """ See the full comment at https://github.com/SSSD/sssd/pull/433#issuecomment-341759325

1 0

[sssd PR#433][comment] PAM: Multiple certificates on a Smartcard
by fidencio 03 Nov '17

03 Nov '17

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, *cai_next; if (list != NULL) { - DLIST_FOR_EACH(cai, list) { - free_cai(cai); + 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` """ See the full comment at https://github.com/SSSD/sssd/pull/433#issuecomment-341727235

1 0

[sssd PR#433][comment] PAM: Multiple certificates on a Smartcard
by fidencio 03 Nov '17

03 Nov '17

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, *cai_next; if (list != NULL) { - DLIST_FOR_EACH(cai, list) { - free_cai(cai); + DLIST_FOR_EACH_SAFE(cai, cai_next, list) { + DLIST_REMOVE(list, 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` """ See the full comment at https://github.com/SSSD/sssd/pull/433#issuecomment-341727235

1 0

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts mzidek-rh commented: """ Sorry, but this: ``` (Fri Nov 3 15:52:42 2017) [sssd[nss]] [sysdb_getpwnam] (0x0020): Search for [email1(a)ipadomain.test] 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. ``` Does not look to me like this: ``` (Fri Nov 3 15:52:42 2017) [sssd[nss]] [sysdb_getpwnam] (0x0020): Please contact sssd-users(a)lists.fedorahosted.org for help with finding problematic entries in sssd cache." ``` But, as I said multiple times in this PR, I am all for changing the message if we have better wording. But your responses are not giving me any hints as to what I should change. Your response about syslog is also vague, but I understand it as you would prefer to log to syslog as well if the wording is useful. Is that correct? """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341758054

1 0

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by lslebodn 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts lslebodn commented: """ On (03/11/17 15:21), mzidek-rh wrote: >About logging message to syslog. I am not against it. I wanted to solve it by MAN + DEBUG. MAN + DEBUG + SYSLOG, looked like overkill to me, especially given that we often do not syslog similar messages and this did not looked that special to me, but I may be wrong. > We already log some messages in responder: ``` src/responder/common/responder_common.c: sss_log(SSS_LOG_WARNING, "List item [%s] is neither a valid " src/responder/common/responder_common.c- "UID nor a user name which could be " src/responder/common/responder_common.c- "resolved by getpwnam().\n", list[c]); src/responder/common/responder_common.c- goto done; ``` We can log message to syslog but ATM it would not be very helpful. It would be almost the as message "Please contact sssd-users(a)lists.fedorahosted.org for help with finding problematic entries in sssd cache." """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341754139

1 0

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts mzidek-rh commented: """ No, I used good example. Your example is the wrong one and would not trigger the error. What do you think will happen if in my example scenario I search for 'getent passwd email2' ? It will return correct results. Do you see why? I already answered it in one of my previous comments, but here I tell it again. Because in my example email2 (fqdn version email2(a)ipadomain.test) is just a username. Same as your benutzer123 and user123. It will never trigger the error because they do not conflict with any email address of another user. In your example, in order to trigger the error you would have to search for getent passwd lorem.ipsum(a)jmail.com. Then you would see the error message telling you that lorem.ipsum(a)jmail.com is is probably conflicting with another users email or fqdn. > The messages need to contain sufficient information to find above problematic entries among 10000 users in sssd cache. The above is telling me absolutely nothing. I told you to suggest better wording, you did not do that. I do not know what to change. You also did not answer my questiion about syslog. Should I add it or not? """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341752846

1 0

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by lslebodn 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts lslebodn commented: """ On (03/11/17 15:06), mzidek-rh wrote: >It will not generate the message you wrote. >It will never show shortname in the output. Yes, I forgot to use fqname there. But it does not mean that current state will help to find problematic entries. >Here is an example where it can generate the error: >- We have two users called email1 and email2 ... they both share the same email address email1(a)ipadomain.test >``` >[root@clietn ~]# getent passwd email1 >email1:*:667000008:667000008:e mail:/home/email1:/bin/sh >[root@clietn ~]# getent passwd email2 >email2:*:667000009:667000009:e mail2:/home/email2:/bin/sh >[root@clietn ~]# getent passwd email1 >[root@clietn ~]# # NOTHING IS RETURNED HERE, let's check the logs >``` > you are using wrong example here. I should be "user123" and "benutzer123" >``` >(Fri Nov 3 15:52:42 2017) [sssd[nss]] [sysdb_getpwnam] (0x0020): Search for [email1(a)ipadomain.test] 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. >``` The problem here is that "email1(a)ipadomain.test" is not an email but internal fqname. So this message will not be useful if domain and real email is totally different e.g. User1 ---------- login: user123 internal fq name: user123(a)ipadomain.test email: lorem.impusun(a)jmail.com User2: ---------- login: benutzer123 internal fq name: benutzer123(a)ipadomain.test email: LOREM.IPSUM(a)jmail.com >So.. what do you suggest to change in the debug message again? > The messages need to contain sufficient information to find above problematic entries among 10000 users in sssd cache. """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341745085

1 0

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts mzidek-rh commented: """ About logging message to syslog. I am not against it. I wanted to solve it by MAN + DEBUG. MAN + DEBUG + SYSLOG, looked like overkill to me, especially given that we often do not syslog similar messages and this did not looked that special to me, but I may be wrong. So, would you prefer to add a syslog message? If so, then to both the getpwnam or also the the 'by_upn' case? Is it OK to have the same message in the syslog as we have in the DEBUG or do you prefer a different message? """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341734499

1 0

[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh 03 Nov '17

03 Nov '17

URL: https://github.com/SSSD/sssd/pull/432 Title: #432: CACHE_REQ: Better debugging for email conflicts mzidek-rh commented: """ You are wrong. It will not generate the message you wrote. It will never show shortname in the output. It will not generate the error if you search by username that is not an email. Here is an example where it can generate the error: - We have two users called email1 and email2 ... they both share the same email address email1(a)ipadomain.test ``` [root@clietn ~]# getent passwd email1 email1:*:667000008:667000008:e mail:/home/email1:/bin/sh [root@clietn ~]# getent passwd email2 email2:*:667000009:667000009:e mail2:/home/email2:/bin/sh [root@clietn ~]# getent passwd email1 [root@clietn ~]# # NOTHING IS RETURNED HERE, let's check the logs ``` ``` (Fri Nov 3 15:52:42 2017) [sssd[nss]] [sysdb_getpwnam] (0x0020): Search for [email1(a)ipadomain.test] 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. ``` So.. what do you suggest to change in the debug message again? """ See the full comment at https://github.com/SSSD/sssd/pull/432#issuecomment-341730086

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

sssd-devel November 2017