[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 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
6 years, 6 months
[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, *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
6 years, 6 months
[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, *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
6 years, 6 months
[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh
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
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 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
6 years, 6 months
[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh
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
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 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
6 years, 6 months
[sssd PR#432][comment] CACHE_REQ: Better debugging for email conflicts
by mzidek-rh
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
6 years, 6 months