[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size

2017-11-23 Thread sumit-bose 
  URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size

sumit-bose commented:
"""
In the latest version I included the 'key->len > strs_len' check and modified 
the check if strs_len points out of the data section to avoid an overrun.

I kept the memchr() check mainly for the initgroups cache data. For passwd and 
group the first element in the string/data area is the name and hence 'key->len 
> strs_len' makes sure the strcmp() will not read pass the current object. For 
the initgroups data the name is 'somewhere' in the data area because the data 
starts with a list of GIDs. So to avoid that strcmp() goes pass the end of the 
data area with a long key in the case of a corruption the "length" of t_key has 
to be determined. Using strnlen() would be possible as well but imo memchr() is 
more clear here.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/455#issuecomment-346757406
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#455][synchronized] mmap_cache: make checks independent of input size

2017-11-23 Thread sumit-bose 
   URL: https://github.com/SSSD/sssd/pull/455
Author: sumit-bose
 Title: #455: mmap_cache: make checks independent of input size
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/455/head:pr455
git checkout pr455
From 3344ba00412d5267cc221a60b1ee1ef23ad403ae Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Fri, 17 Nov 2017 10:51:44 +0100
Subject: [PATCH 1/2] mmap_cache: make checks independent of input size

Currently the consistency checks for the mmap_cache payload data on the
client and the responder side include the length of the input string of
the current request. Since there might be hash collisions which other
much longer or much shorter names those checks might fail although there
is no data corruption.

This patch removes the checks using the length of the input and adds a
check if the name found in the payload is zero-terminated inside of the
payload data.

Resolves https://pagure.io/SSSD/sssd/issue/3571
---
 src/responder/nss/nsssrv_mmap_cache.c | 26 +-
 src/sss_client/nss_mc_group.c | 14 --
 src/sss_client/nss_mc_initgr.c| 14 +-
 src/sss_client/nss_mc_passwd.c| 14 --
 4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index a87ad646f..46a1585cd 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -547,18 +547,34 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc,
 return NULL;
 }
 
+if (key->len > strs_len) {
+/* The string cannot be in current record */
+slot = sss_mc_next_slot_with_hash(rec, hash);
+continue;
+}
+
 safealign_memcpy(_ptr, rec->data, sizeof(rel_ptr_t), NULL);
-if (key->len > strs_len
-|| (name_ptr + key->len) > (strs_offset + strs_len)
-|| (uint8_t *)rec->data + strs_offset + strs_len > max_addr) {
+t_key = (char *)rec->data + name_ptr;
+/* name_ptr must point to some data in the strs/gids area of the data
+ * payload. Since it is a pointer relative to rec->data it must larger
+ * equal strs_offset and must be smaller then strs_offset + strs_len.
+ * Additionally the area must not end outside of the data table and
+ * t_key must be a zero-terminates string. */
+if (name_ptr < strs_offset
+|| name_ptr >= strs_offset + strs_len
+|| (uint8_t *)rec->data > max_addr
+|| strs_offset > max_addr - (uint8_t *)rec->data
+|| strs_len > max_addr - (uint8_t *)rec->data - strs_offset
+|| memchr(t_key, '\0',
+  (strs_offset + strs_len) - name_ptr) == NULL ) {
 DEBUG(SSSDBG_FATAL_FAILURE,
-  "Corrupted fastcache. name_ptr value is %u.\n", name_ptr);
+  "Corrupted fastcache entry at slot %u. "
+  "name_ptr value is %u.\n", slot, name_ptr);
 sss_mc_save_corrupted(mcc);
 sss_mmap_cache_reset(mcc);
 return NULL;
 }
 
-t_key = (char *)rec->data + name_ptr;
 if (strcmp(key->str, t_key) == 0) {
 break;
 }
diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index ce88d42fd..4b1601a17 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -148,20 +148,22 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
 }
 
 data = (struct sss_mc_grp_data *)rec->data;
+rec_name = (char *)data + data->name;
 /* Integrity check
- * - name_len cannot be longer than all strings
  * - data->name cannot point outside strings
  * - all strings must be within copy of record
- * - size of record must be lower that data table size */
-if (name_len > data->strs_len
-|| (data->name + name_len) > (strs_offset + data->strs_len)
+ * - record must not end outside data table
+ * - rec_name is a zero-terminated string */
+if (data->name < strs_offset
+|| data->name >= strs_offset + data->strs_len
 || data->strs_len > rec->len
-|| rec->len > data_size) {
+|| (uint8_t *) rec + rec->len > gr_mc_ctx.data_table + data_size
+|| memchr(rec_name, '\0',
+  (strs_offset + data->strs_len) - data->name) == NULL) {
 ret = ENOENT;
 goto done;
 }
 
-rec_name = (char *)data + data->name;
 if (strcmp(name, rec_name) == 0) {
 break;
 }
diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c
index a77088d84..d8c01f52e 100644
--- a/src/sss_client/nss_mc_initgr.c
+++

[SSSD] [sssd PR#450][comment] sysdb: do not use objectClass for users and groups

2017-11-23 Thread jhrozek 
  URL: https://github.com/SSSD/sssd/pull/450
Title: #450: sysdb: do not use objectClass for users and groups

jhrozek commented:
"""
Some more test results:
 - PASS: 2156182 - Tests-for-LDAP-ID-and-KRB5-AUTH
 - "PASS": 2156173 - ipa-sudo
   - there is an AVC message in the server recipe, but the client passed
- PASS: 2156174 - ipa-selinuxusermap-func
- PASS: 2156175 - ipa-hbac-func
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/450#issuecomment-346683229
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog

2017-11-23 Thread jhrozek 
  URL: https://github.com/SSSD/sssd/pull/453
Title: #453: Speed up by-ID lookups with the help of the Global Catalog

jhrozek commented:
"""
I'll try to work on that during the next couple of days.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/453#issuecomment-346682291
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#405][comment] WATCHDOG: Restart providers with SIGUSR2 after time drift

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/405
Title: #405:  WATCHDOG: Restart providers with SIGUSR2 after time drift

lslebodn commented:
"""
master:
* ad286a79ce2cf0be8283ccdf24c5189d22eab0c8

sssd-1-14:
* e8fc2e93d400088cb0a0e4dac9500a250eb235c3
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/405#issuecomment-346618308
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#405][+Pushed] WATCHDOG: Restart providers with SIGUSR2 after time drift

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/405
Title: #405:  WATCHDOG: Restart providers with SIGUSR2 after time drift

Label: +Pushed
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#405][closed] WATCHDOG: Restart providers with SIGUSR2 after time drift

2017-11-23 Thread lslebodn 
   URL: https://github.com/SSSD/sssd/pull/405
Author: vtapia
 Title: #405:  WATCHDOG: Restart providers with SIGUSR2 after time drift
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/405/head:pr405
git checkout pr405
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

2017-11-23 Thread bachradsusi 
  URL: https://github.com/SSSD/sssd/pull/457
Title: #457: ipa: Removal of umask(0) in selinux_child

bachradsusi commented:
"""
I guess it's related to the change of SElinux module store location in 
/var/lib/selinux which happened in Release 2015-02-02 aka libsemanage-2.4. 
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/457#issuecomment-346609381
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size

2017-11-23 Thread mzidek-rh 
  URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size

mzidek-rh commented:
"""
On 11/23/2017 12:50 PM, lslebodn wrote:
> On (23/11/17 11:31), mzidek-rh wrote:
>  >mzidek-rh commented on this pull request.
>  >I do not think such optimisation is needed
> 
> And I do not think that these checks for corrupted memory cache are needed
> since 1.11.90. But I'm fine with having them.
> 
>  >because it will almost always be evaluated as false
> 
> That statement is true also for other checks for "corrupted" mmap cache.

Please avoid cherry picking my sentences in a way that changes the original 
meaning and also do not put them in inappropriate context. The statement I said 
is NOT true for the checks for corrupted mmap cache, because the whole 
statement was:

"I do not think such optimisation is needed because it will almost always be 
evaluated as false and in case it is evaluated as true it will only save us 
from doing few more checks. "

So my point was that we do some computation in order to avoid just a slightly 
bigger computation, but we will almost never avoid it anyway. And as we know, 
the checks for corrupted mmap cache were not an optimisation, so the statement 
is simply not applicable to that case unless you remove part of the statement 
and put it to a new context. But by doing that you are creating your own 
statement that has nothing to do with what I said.

But as I said, I do not dislike the check becuase in my (very subjective) 
opinion it makes the code easier to read.

Michal

> 
>  >and in case it is evaluated as true it will only save us from doing 
> few more checks. But maybe having this check makes this part of code 
> more readable (at least it looks that way to me)? So I am OK with it.
>  >
> 
> Thank you for confirmation that you are fine with such check.
> 
> LS
> 
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/455#issuecomment-346605877
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size

lslebodn commented:
"""
On (23/11/17 11:31), mzidek-rh wrote:
>mzidek-rh commented on this pull request.
>I do not think such optimisation is needed

And I do not think that these checks for corrupted memory cache are needed
since 1.11.90. But I'm fine with having them.

>because it will almost always be evaluated as false

That statement is true also for other checks for "corrupted" mmap cache.

>and in case it is evaluated as true it will only save us from doing few more 
>checks. But maybe having this check makes this part of code more readable (at 
>least it looks that way to me)? So I am OK with it.
>

Thank you for confirmation that you are fine with such check.

LS

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/455#issuecomment-346598250
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/457
Title: #457: ipa: Removal of umask(0) in selinux_child

lslebodn commented:
"""
On (22/11/17 12:23), fidencio wrote:
>We're fine about RHEL-6 as long as we don't explicitly backport it there.
>
>I'm a little bit worried about openSUSE/SLES as they may use SELinux. Debian, 
>AFAIR, doesn't use SELinux (but I'd like to have Timo's confirmation here).
>

Actually situation is different.

*SUSE use apparmor and SSSD is compiled without SELinux and libsemanage.
But I do not thik many debian users use SELinux + libsemanage.
BTW debian stable has libsemanage 2.6 
https://packages.debian.org/stretch/libsemanage1

@bachradsusi , do you know which upstream version of libsemanage does not
requires workaround with `umask(0)`?

LS

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/457#issuecomment-346592086
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

2017-11-23 Thread amitkumar50 
  URL: https://github.com/SSSD/sssd/pull/457
Title: #457: ipa: Removal of umask(0) in selinux_child

amitkumar50 commented:
"""
I believe this change would go into rhel-7.1 and above since mentioned bugzilla 
is fixed there.
But yes this code should not go to rhel-6/Debian etc where bugzilla is not 
fixed.

But what we do for these workaround-codes when ever We have an 
working-permanent fix?
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/457#issuecomment-346591909
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#452][+Pushed] SPEC: Reduce build time dependencies

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/452
Title: #452: SPEC: Reduce build time dependencies

Label: +Pushed
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#452][comment] SPEC: Reduce build time dependencies

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/452
Title: #452: SPEC: Reduce build time dependencies

lslebodn commented:
"""
master:
* 700fced0621545845ad7665fe03b94150798f11a
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/452#issuecomment-346577257
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#452][closed] SPEC: Reduce build time dependencies

2017-11-23 Thread lslebodn 
   URL: https://github.com/SSSD/sssd/pull/452
Author: lslebodn
 Title: #452: SPEC: Reduce build time dependencies
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/452/head:pr452
git checkout pr452
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#452][comment] SPEC: Reduce build time dependencies

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/452
Title: #452: SPEC: Reduce build time dependencies

lslebodn commented:
"""
On (16/11/17 12:05), fidencio wrote:
>@lslebodn, just one question (trying to learn a little bit here) why did you 
>choose your approach over BuildRequires: pkgconfig(gdm-pam-extensions) ?
>

Because I prefer to have minimal dependencies in buildroot.
And "pkgconfig(gdm-pam-extensions)" could in theory pull gdm-devel with
disabled updates and updates-testing on f27

LS

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/452#issuecomment-346576805
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#452][comment] SPEC: Reduce build time dependencies

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/452
Title: #452: SPEC: Reduce build time dependencies

lslebodn commented:
"""
On (16/11/17 11:51), fidencio wrote:
>I'd like to ask a mention in the commit message the commit you filed that 
>ended up with the gdm-pam-extensions-devel creation.
>
>Anyways, obvious ACK!
>

http://vm-031.$ABC/logs/job/81/78/summary.html

LS

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/452#issuecomment-346576451
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#405][comment] WATCHDOG: Restart providers with SIGUSR2 after time drift

2017-11-23 Thread fidencio 
  URL: https://github.com/SSSD/sssd/pull/405
Title: #405:  WATCHDOG: Restart providers with SIGUSR2 after time drift

fidencio commented:
"""
debug_prg_name will never be "sssd[be[". 

Please, use strncmp() there and the patch is okay.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/405#issuecomment-346555906
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#405][comment] WATCHDOG: Restart providers with SIGUSR2 after time drift

2017-11-23 Thread vtapia 
  URL: https://github.com/SSSD/sssd/pull/405
Title: #405:  WATCHDOG: Restart providers with SIGUSR2 after time drift

vtapia commented:
"""
Please, go ahead, and thanks!
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/405#issuecomment-346555285
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#405][comment] WATCHDOG: Restart providers with SIGUSR2 after time drift

2017-11-23 Thread lslebodn 
  URL: https://github.com/SSSD/sssd/pull/405
Title: #405:  WATCHDOG: Restart providers with SIGUSR2 after time drift

lslebodn commented:
"""
I did some testing and cannot see any new random failures.
So change seems to be fine.

@vtapia I would like to squash following change before pushing patches 
`s/strstr/strcmp/`

```
diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
index 024e97fbc..f37748ae8 100644
--- a/src/util/util_watchdog.c
+++ b/src/util/util_watchdog.c
@@ -160,7 +160,7 @@ static void watchdog_fd_read_handler(struct tevent_context 
*ev,
   "[%d]: %s\n", ret, sss_strerror(ret));
 orderly_shutdown(1);
 }
-if (strstr(debug_prg_name, "sssd[be[") != NULL) {
+if (strcmp(debug_prg_name, "sssd[be[") == 0) {
 kill(getpid(), SIGUSR2);
 DEBUG(SSSDBG_IMPORTANT_INFO, "SIGUSR2 sent to %s\n", debug_prg_name);
 }
```

Are you fine with such change? And sorry for long review.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/405#issuecomment-346554958
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org