[RFC] Use of DP_ERR_* codes
by Pavel Březina
Hi!
Since we have pretty nice custom error codes, I would like to get rid of
DP_ERR_* codes. They are practically useless and are adding non-trivial
complexity to the code.
I did only a quick look so it is possible that I missed something, but
let's talk about it. We have the following codes:
> #define DP_ERR_OK 0
DP_ERR_OK is usually used in combination with EOK and in most cases it
is treated as error (or worse not handled at all). There are some
situations when DP_ERR_OK and !EOK is used, it is mostly in combination
with PAM error code and access control, but we can handle this with
custom error codes.
> #define DP_ERR_OFFLINE 1
DP_ERR_OFFLINE means EAGAIN, always. This should be changed to
ERR_OFFLINE or similar custom error code.
> #define DP_ERR_TIMEOUT 2
This is not used at all.
> #define DP_ERR_FATAL 3
This is other than EOK code and should be avoided.
Having only one error code from sdap operations would simplify code flow
and make the code less error prone.
In my opinion this should be part of or even prerequisite to ldap
provider and sdap_id_op refactoring.
8 years, 5 months
Review of umask() in SSSD
by Petr Cech
Hi,
I am reviewing umask() in our code according to
https://fedorahosted.org/sssd/ticket/2424
There are many use like umask(DFL_RSP_UMASK):
src/responder/autofs/autofssrv.c:223
src/responder/ifp/ifpsrv.c:401
src/responder/nss/nsssrv.c:589
src/responder/pac/pacsrv.c:232
src/responder/pam/pamsrv.c:369
src/responder/ssh/sshsrv.c:209
src/responder/sudo/sudosrv.c:215
where DFL_RSP_UMASK is defined as 0177.
There are another three use of umask 0177:
src/confdb/confdb.c:662
src/util/debug.c:365
src/util/server.c:495
And then I see many use of umask 077:
src/p11_child/p11_child_nss.c:485
src/providers/krb5/krb5_child.c:723
src/tests/check_and_open-tests.c:51
src/tests/debug-tests.c:136
src/tests/debug-tests.c:276
src/tests/util-tests.c:596
src/util/domain_info_utils.c:312
src/util/domain_info_utils.c:562
src/tools/tools_util.c:503
I would like to ask you if we would like to use 0077 or 0177 as our very
restrictive mask. I see that our code is not consistent on this
question. I know the difference is small, but it is.
Then we have some unsecure use:
src/providers/ipa/selinux_child.c:154: umask = 0
src/providers/krb5/krb5_ccache.c:188: umask = 0000
src/responder/nss/nsssrv_mmap_cache.c:1121: umask = 0022
but I think there is reason for it.
And the last one is at src/responder/common/responder_common.c:561:
int create_pipe_fd(const char *sock_name, int *_fd, mode_t umaskval)
We use it secure (0177) at:
src/responder/common/responder_common.c:693
src/responder/pam/pamsrv.c:399
And not so secure:
src/responder/common/responder_common.c:670 umask = 0111
src/responder/pam/pamsrv.c:391 umask = 0111
src/tests/cwrap/test_responder_common.c:173 umask = 0111
src/tests/cwrap/test_responder_common.c:179 umask = 0000
So, what could I do? Maybe we could have only one very secure umask and
maybe we could have CONSTANT for every use of umask. Any another ideas?
Regards
Petr
8 years, 5 months
[PATCH] LDAP: Fix leak of file descriptors
by Lukas Slebodnik
ehlo,
Details are in commit message. BTW it would be good to have at least two
reviews.
Reproducer which I used for https://fedorahosted.org/sssd/ticket/2792:
Setup:
* two active directories with sites; so sometimes sssd connect to server A
and sometims to server B.
* block connection to one server
[root@host sssd]# iptables -n -L
Chain INPUT (policy ACCEPT)
target prot opt source destination
Chain FORWARD (policy ACCEPT)
target prot opt source destination
Chain OUTPUT (policy ACCEPT)
target prot opt source destination
DROP tcp -- 0.0.0.0/0 10.12.0.158 tcp dpt:389
* force sssd to go offline (-USR1) and online (-USR2)
You might be able to reproduce ti even with plan LDAP.
It might help if set value of options that
dns_resolver_timeout < ldap_network_timeout
Do you have an idea how to test such fd leak?
(either using cmocka or integration tests)
LS
8 years, 5 months
[PATCH] NSS: fix a use-after-free issue
by Sumit Bose
Hi,
I found this accidentally because I was still running SSSD with
MALLOC_PERTURB_ set which I used some time ago to hunt a glibc issue.
This issue wasn't caught before by the unit-tests because
sss_cmd_done() which frees the context is overwritten in the tests and
so far didn't free the context. Additionally, since the use after free
was in a debug message, the unit tests must be run with debugging enable
to run over it. I think this is also the reason why Coverity or Clang
didn't found this issue before. Does anyone know if it is possible to
tell Converity to assume debug_level is set to 10?
bye,
Sumit
8 years, 5 months
