[SSSD] [PATCH] sudo: remove unused param name in sdap_sudo_get_usn()

2015-11-02 Thread Pavel Reichl 

Hello,

please see simple patch attached.

Thanks!
>From 747ccf2265a7dcf110b1a9b5fa2d0819c2bc5734 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Mon, 2 Nov 2015 14:59:49 +0100
Subject: [PATCH] sudo: remove unused param name in sdap_sudo_get_usn()

---
 src/providers/ldap/sdap_sudo_cache.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/providers/ldap/sdap_sudo_cache.c b/src/providers/ldap/sdap_sudo_cache.c
index 27203c227064bdcd918cda67bb93a5d62b42e4bd..56e84ce8f26338ea5856eb5c76627641eee93df1 100644
--- a/src/providers/ldap/sdap_sudo_cache.c
+++ b/src/providers/ldap/sdap_sudo_cache.c
@@ -28,7 +28,6 @@
 static errno_t sdap_sudo_get_usn(TALLOC_CTX *mem_ctx,
  struct sysdb_attrs *attrs,
  struct sdap_attr_map *map,
- const char *name,
  char **_usn)
 {
 const char *usn;
@@ -86,7 +85,7 @@ sdap_save_native_sudorule(TALLOC_CTX *mem_ctx,
 return ret;
 }
 
-ret = sdap_sudo_get_usn(mem_ctx, attrs, map, rule_name, _usn);
+ret = sdap_sudo_get_usn(mem_ctx, attrs, map, _usn);
 if (ret != EOK) {
 DEBUG(SSSDBG_MINOR_FAILURE, "Could not read USN from %s\n", rule_name);
 *_usn = NULL;
-- 
2.4.3

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] sudo: remove unused param name in sdap_sudo_get_usn()

2015-11-02 Thread Lukas Slebodnik 
On (02/11/15 15:51), Petr Cech wrote:
>On 11/02/2015 03:09 PM, Petr Cech wrote:
>>On 11/02/2015 03:03 PM, Pavel Reichl wrote:
>>>Hello,
>>>
>>>please see simple patch attached.
>>>
>>>Thanks!
>>>
>>LGTM, I am waiting for CI :-)
>>
>>Petr
>
>CI tests passed:
>http://sssd-ci.duckdns.org/logs/job/31/85/summary.html
>
>=> ACK
>

master:
* e307c269fe1dc94a1771b459c5925e449ba7668b

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

2015-11-02 Thread Lukas Slebodnik 
On (02/11/15 14:41), Pavel Reichl wrote:
>On 11/02/2015 01:02 PM, Sumit Bose wrote:
>>On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
>>>On (29/10/15 15:32), Pavel Reichl wrote:
Hello,

while I worked on tests for PAM responder Sumit proposed to use attached 
patch. QA have already kindly run their tests (successfully).

I'll do the review myself (But all opinions are welcomed for sure).

Thanks
>>>
From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
src/providers/dp_pam_data_util.c | 4 
1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c 
b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
*/

+#include 
+
>>>We try to avoid including non-standar pam header files.
>>>Prefix "_" is Linux PAM specific.
>>>
>>
>>Fixed, new version attached.
>>
>>bye,
>>Sumit
>
>Thank you both. Code LGTM, ci passed with exception of rawhide where it seems 
>to be a problem of package dependency which is not relevant to this patch:
>http://sssd-ci.duckdns.org/logs/job/31/81/summary.html
>
>ACK
>
I would say you ack-ed your own patch because Sumit's patch is almost the same.
So it should not count.

I will run som authentication related test (with AD)
to be sure it does not break some corner case.
Even though it should not.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] sudo: remove unused param name in sdap_sudo_get_usn()

2015-11-02 Thread Petr Cech 

On 11/02/2015 03:09 PM, Petr Cech wrote:

On 11/02/2015 03:03 PM, Pavel Reichl wrote:

Hello,

please see simple patch attached.

Thanks!


LGTM, I am waiting for CI :-)

Petr


CI tests passed:
http://sssd-ci.duckdns.org/logs/job/31/85/summary.html

=> ACK

Petr
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

2015-11-02 Thread Pavel Reichl 



On 11/02/2015 01:02 PM, Sumit Bose wrote:

On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:

On (29/10/15 15:32), Pavel Reichl wrote:

Hello,

while I worked on tests for PAM responder Sumit proposed to use attached patch. 
QA have already kindly run their tests (successfully).

I'll do the review myself (But all opinions are welcomed for sure).

Thanks


>From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001

From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
src/providers/dp_pam_data_util.c | 4 
1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
*/

+#include 
+

We try to avoid including non-standar pam header files.
Prefix "_" is Linux PAM specific.



Fixed, new version attached.

bye,
Sumit


Thank you both. Code LGTM, ci passed with exception of rawhide where it seems 
to be a problem of package dependency which is not relevant to this patch:
http://sssd-ci.duckdns.org/logs/job/31/81/summary.html

ACK





___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SDAP: Remove unused sdap_id_ctx from sdap_id_conn_cache_create

2015-11-02 Thread Lukas Slebodnik 
On (02/11/15 13:20), Pavel Reichl wrote:
>On 11/02/2015 12:07 PM, Pavel Reichl wrote:
>>On 11/02/2015 11:50 AM, Lukas Slebodnik wrote:
>>>ehlo,
>>>
>>>simple patch is attached.
>>>
>>>LS
>>>
>>
>>LGTM, compiles fine. I'll just postpone acking until CI results arrive...
>
>ACK ci passed: http://sssd-ci.duckdns.org/logs/job/31/80/summary.html
>
master:
* ba17e124aa7003a92680eda5df0a9b5292c8c19c

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] sudo: remove unused param name in sdap_sudo_get_usn()

2015-11-02 Thread Petr Cech 

On 11/02/2015 03:03 PM, Pavel Reichl wrote:

Hello,

please see simple patch attached.

Thanks!


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


LGTM, I am waiting for CI :-)

Petr
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

2015-11-02 Thread Sumit Bose 
On Mon, Nov 02, 2015 at 04:05:33PM +0100, Lukas Slebodnik wrote:
> On (02/11/15 14:41), Pavel Reichl wrote:
> >On 11/02/2015 01:02 PM, Sumit Bose wrote:
> >>On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
> >>>On (29/10/15 15:32), Pavel Reichl wrote:
> Hello,
> 
> while I worked on tests for PAM responder Sumit proposed to use attached 
> patch. QA have already kindly run their tests (successfully).
> 
> I'll do the review myself (But all opinions are welcomed for sure).
> 
> Thanks
> >>>
> From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Mon, 19 Oct 2015 13:10:51 -0400
> Subject: [PATCH] PAM: successful authentication sets explicitly 
> PAM_SUCCESSS
> 
> Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
> set PAM_SUCCESSS explicitly for a successful authentication and will
> really return an error in all other cases.
> ---
> src/providers/dp_pam_data_util.c | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/src/providers/dp_pam_data_util.c 
> b/src/providers/dp_pam_data_util.c
> index 
> 10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
>  100644
> --- a/src/providers/dp_pam_data_util.c
> +++ b/src/providers/dp_pam_data_util.c
> @@ -22,6 +22,8 @@
>  along with this program.  If not, see .
> */
> 
> +#include 
> +
> >>>We try to avoid including non-standar pam header files.
> >>>Prefix "_" is Linux PAM specific.
> >>>
> >>
> >>Fixed, new version attached.
> >>
> >>bye,
> >>Sumit
> >
> >Thank you both. Code LGTM, ci passed with exception of rawhide where it 
> >seems to be a problem of package dependency which is not relevant to this 
> >patch:
> >http://sssd-ci.duckdns.org/logs/job/31/81/summary.html
> >
> >ACK
> >
> I would say you ack-ed your own patch because Sumit's patch is almost the 
> same.
> So it should not count.

no, I was mine all the way. Pavel just send it to the list after running
tests with our QE team. Sorry for the confusion.

bye,
Sumit

> 
> I will run som authentication related test (with AD)
> to be sure it does not break some corner case.
> Even though it should not.
> 
> LS
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] LDAP: Fix leak of file descriptors

2015-11-02 Thread Lukas Slebodnik 
On (22/10/15 13:47), Lukas Slebodnik wrote:
>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/010.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

>From 8cb857694f0a6f48b63eee50fe6b03dd3820c28e Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik 
>Date: Thu, 22 Oct 2015 10:30:12 +0200
>Subject: [PATCH] LDAP: Fix leak of file descriptors
>
bump

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] NSS: fix a use-after-free issue

2015-11-02 Thread Lukas Slebodnik 
On (30/10/15 17:35), Sumit Bose wrote:
>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
>

>From 309f59199c49d3d4dc4fa42229f6b2fa1e82e72a Mon Sep 17 00:00:00 2001
>From: Sumit Bose 
>Date: Fri, 30 Oct 2015 16:28:37 +0100
>Subject: [PATCH] NSS: fix a use-after-free issue
>
>---
> src/responder/nss/nsssrv_cmd.c  | 10 ++
> src/tests/cmocka/test_nss_srv.c |  1 +
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
>index 
>b8bd6425e2c937ce6008fd6663fe0312ad68f01e..58d20fab11eea6f419031f439083ca5f6d8c61bb
> 100644
>--- a/src/responder/nss/nsssrv_cmd.c
>+++ b/src/responder/nss/nsssrv_cmd.c
>@@ -5455,11 +5455,13 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, 
>struct cli_ctx *cctx)
> ret = nss_check_well_known_sid(cmdctx);
> if (ret != ENOENT) {
> if (ret == EOK) {
>-DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n",
>- cmdctx->secid);
>-} else {
>-DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
>+DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n", 
>sid_str);
>+/* message is already send and cmdctx is freed,
>+ * we can just return */
>+return EOK;
> }
>+
>+DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
it would be good to log error code here.
> goto done;

Was "use-after-free" caused only by logging sid or was it used elsewhere?

Could you append valgrind error to commit mesage?

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] NSS: fix a use-after-free issue

2015-11-02 Thread Lukas Slebodnik 
On (02/11/15 10:05), Sumit Bose wrote:
>On Mon, Nov 02, 2015 at 09:42:51AM +0100, Lukas Slebodnik wrote:
>> On (30/10/15 17:35), Sumit Bose wrote:
>> >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
>> >
>> 
>> >From 309f59199c49d3d4dc4fa42229f6b2fa1e82e72a Mon Sep 17 00:00:00 2001
>> >From: Sumit Bose 
>> >Date: Fri, 30 Oct 2015 16:28:37 +0100
>> >Subject: [PATCH] NSS: fix a use-after-free issue
>> >
>> >---
>> > src/responder/nss/nsssrv_cmd.c  | 10 ++
>> > src/tests/cmocka/test_nss_srv.c |  1 +
>> > 2 files changed, 7 insertions(+), 4 deletions(-)
>> >
>> >diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
>> >index 
>> >b8bd6425e2c937ce6008fd6663fe0312ad68f01e..58d20fab11eea6f419031f439083ca5f6d8c61bb
>> > 100644
>> >--- a/src/responder/nss/nsssrv_cmd.c
>> >+++ b/src/responder/nss/nsssrv_cmd.c
>> >@@ -5455,11 +5455,13 @@ static int nss_cmd_getbysid(enum sss_cli_command 
>> >cmd, struct cli_ctx *cctx)
>> > ret = nss_check_well_known_sid(cmdctx);
>> > if (ret != ENOENT) {
>> > if (ret == EOK) {
>> >-DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n",
>> >- cmdctx->secid);
>> >-} else {
>> >-DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
>> >+DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n", 
>> >sid_str);
>> >+/* message is already send and cmdctx is freed,
>> >+ * we can just return */
>> >+return EOK;
>> > }
>> >+
>> >+DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
>> it would be good to log error code here.
>> > goto done;
>> 
>> Was "use-after-free" caused only by logging sid or was it used elsewhere?
>
>ah, sorry. Yes, I made two changes here, replacing the variable in the
>debug message and returning immediately and no go to done. The
>use-after-free was in the debug message. cmdctx is not touched in the
>following code path anymore but since we are all set I think it is
>safer to just return here.
>
>> 
>> Could you append valgrind error to commit mesage?
>
>I found the issue via a core dump. Do you prefer the valgrind message or
>the first steps of a stack trace?
>
We can see when memory was released from valgrind message.
A stack trace just shows when it crashed.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] SDAP: Remove unused sdap_id_ctx from sdap_id_conn_cache_create

2015-11-02 Thread Lukas Slebodnik 
ehlo,

simple patch is attached.

LS
>From 08cea9235d8b51d0d8174969c73f93a17cf992e8 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Sat, 24 Oct 2015 00:02:01 +0200
Subject: [PATCH] SDAP: Remove unused sdap_id_ctx from
 sdap_id_conn_cache_create

---
 src/providers/ldap/ldap_common.c | 2 +-
 src/providers/ldap/sdap_id_op.c  | 1 -
 src/providers/ldap/sdap_id_op.h  | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index 
aa4c6cb851a5735e051ef2c024ca0171a4f61148..35de9c0a7929990fbf7a7a194fac3c8e0c9c31f2
 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -938,7 +938,7 @@ sdap_id_ctx_conn_add(struct sdap_id_ctx *id_ctx,
 conn->id_ctx = id_ctx;
 
 /* Create a connection cache */
-ret = sdap_id_conn_cache_create(conn, id_ctx, conn, >conn_cache);
+ret = sdap_id_conn_cache_create(conn, conn, >conn_cache);
 if (ret != EOK) {
 talloc_free(conn);
 return NULL;
diff --git a/src/providers/ldap/sdap_id_op.c b/src/providers/ldap/sdap_id_op.c
index 
0474a9cb7828ef43ef76cf6bebac077315296875..6ad7e8188dc6211907cac88e90e4feff5080695c
 100644
--- a/src/providers/ldap/sdap_id_op.c
+++ b/src/providers/ldap/sdap_id_op.c
@@ -102,7 +102,6 @@ static void sdap_id_op_connect_done(struct tevent_req 
*subreq);
 
 /* Create a connection cache */
 int sdap_id_conn_cache_create(TALLOC_CTX *memctx,
-  struct sdap_id_ctx *id_ctx,
   struct sdap_id_conn_ctx *id_conn,
   struct sdap_id_conn_cache** conn_cache_out)
 {
diff --git a/src/providers/ldap/sdap_id_op.h b/src/providers/ldap/sdap_id_op.h
index 
b808dd89aebb096b7163c10df39784a54b7e0b03..f7f230a734a4090af07c62d1884beefe5ebba750
 100644
--- a/src/providers/ldap/sdap_id_op.h
+++ b/src/providers/ldap/sdap_id_op.h
@@ -38,7 +38,6 @@ struct sdap_id_op;
 
 /* Create a connection cache */
 int sdap_id_conn_cache_create(TALLOC_CTX *memctx,
-  struct sdap_id_ctx *id_ctx,
   struct sdap_id_conn_ctx *id_conn,
   struct sdap_id_conn_cache** conn_cache_out);
 
-- 
2.5.0

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SDAP: Remove unused sdap_id_ctx from sdap_id_conn_cache_create

2015-11-02 Thread Pavel Reichl 

On 11/02/2015 11:50 AM, Lukas Slebodnik wrote:

ehlo,

simple patch is attached.

LS



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



LGTM, compiles fine. I'll just postpone acking until CI results arrive...
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SDAP: Remove unused sdap_id_ctx from sdap_id_conn_cache_create

2015-11-02 Thread Pavel Reichl 

On 11/02/2015 12:07 PM, Pavel Reichl wrote:

On 11/02/2015 11:50 AM, Lukas Slebodnik wrote:

ehlo,

simple patch is attached.

LS



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



LGTM, compiles fine. I'll just postpone acking until CI results arrive...


ACK ci passed: http://sssd-ci.duckdns.org/logs/job/31/80/summary.html

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] NSS: fix a use-after-free issue

2015-11-02 Thread Sumit Bose 
On Mon, Nov 02, 2015 at 10:30:51AM +0100, Lukas Slebodnik wrote:
> On (02/11/15 10:05), Sumit Bose wrote:
> >On Mon, Nov 02, 2015 at 09:42:51AM +0100, Lukas Slebodnik wrote:
> >> On (30/10/15 17:35), Sumit Bose wrote:
> >> >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
> >> >
> >> 
> >> >From 309f59199c49d3d4dc4fa42229f6b2fa1e82e72a Mon Sep 17 00:00:00 2001
> >> >From: Sumit Bose 
> >> >Date: Fri, 30 Oct 2015 16:28:37 +0100
> >> >Subject: [PATCH] NSS: fix a use-after-free issue
> >> >
> >> >---
> >> > src/responder/nss/nsssrv_cmd.c  | 10 ++
> >> > src/tests/cmocka/test_nss_srv.c |  1 +
> >> > 2 files changed, 7 insertions(+), 4 deletions(-)
> >> >
> >> >diff --git a/src/responder/nss/nsssrv_cmd.c 
> >> >b/src/responder/nss/nsssrv_cmd.c
> >> >index 
> >> >b8bd6425e2c937ce6008fd6663fe0312ad68f01e..58d20fab11eea6f419031f439083ca5f6d8c61bb
> >> > 100644
> >> >--- a/src/responder/nss/nsssrv_cmd.c
> >> >+++ b/src/responder/nss/nsssrv_cmd.c
> >> >@@ -5455,11 +5455,13 @@ static int nss_cmd_getbysid(enum sss_cli_command 
> >> >cmd, struct cli_ctx *cctx)
> >> > ret = nss_check_well_known_sid(cmdctx);
> >> > if (ret != ENOENT) {
> >> > if (ret == EOK) {
> >> >-DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n",
> >> >- cmdctx->secid);
> >> >-} else {
> >> >-DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid 
> >> >failed.\n");
> >> >+DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n", 
> >> >sid_str);
> >> >+/* message is already send and cmdctx is freed,
> >> >+ * we can just return */
> >> >+return EOK;
> >> > }
> >> >+
> >> >+DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
> >> it would be good to log error code here.
> >> > goto done;
> >> 
> >> Was "use-after-free" caused only by logging sid or was it used elsewhere?
> >
> >ah, sorry. Yes, I made two changes here, replacing the variable in the
> >debug message and returning immediately and no go to done. The
> >use-after-free was in the debug message. cmdctx is not touched in the
> >following code path anymore but since we are all set I think it is
> >safer to just return here.
> >
> >> 
> >> Could you append valgrind error to commit mesage?
> >
> >I found the issue via a core dump. Do you prefer the valgrind message or
> >the first steps of a stack trace?
> >
> We can see when memory was released from valgrind message.
> A stack trace just shows when it crashed.

good point, new version attached.

bye,
Sumit
From 9561733a6ce32b5787acc5277e2d45026c5126db Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Fri, 30 Oct 2015 16:28:37 +0100
Subject: [PATCH] NSS: fix a use-after-free issue

While handling well-known SIDs a debug statement tries to access memory that is
already freed. This can be seen with the following output from valgrind.

==17600== Invalid read of size 4
==17600==at 0x805ACC6: nss_cmd_getbysid (nsssrv_cmd.c:5458)
==17600==by 0x805AF41: nss_cmd_getnamebysid (nsssrv_cmd.c:5509)
==17600==by 0x80662F4: sss_cmd_execute (responder_cmd.c:161)
==17600==by 0x8067015: client_cmd_execute (responder_common.c:249)
==17600==by 0x80671F5: client_recv (responder_common.c:283)
==17600==by 0x806741C: client_fd_handler (responder_common.c:335)
==17600==by 0x45F5112: epoll_event_loop (tevent_epoll.c:728)
==17600==by 0x45F5112: epoll_event_loop_once (tevent_epoll.c:926)
==17600==by 0x45F32EE: std_event_loop_once (tevent_standard.c:114)
==17600==by 0x45EF3BF: _tevent_loop_once (tevent.c:530)
==17600==by 0x45EF5AB: tevent_common_loop_wait (tevent.c:634)
==17600==by 0x45F326E: std_event_loop_wait (tevent_standard.c:140)
==17600==by 0x45EF647: _tevent_loop_wait (tevent.c:653)
==17600==  Address 0x4b248a0 is 72 bytes inside a block of size 88 free'd
==17600==at 0x402C26D: free (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==17600==by 0x45FEC9E: _talloc_free_internal (talloc.c:1057)
==17600==by 0x45FEC9E: _talloc_free (talloc.c:1581)
==17600==by 0x8066085: sss_cmd_done (responder_cmd.c:93)
==17600==by 0x805A9B0: nss_check_well_known_sid (nsssrv_cmd.c:5382)
==17600==by 0x805AC86: nss_cmd_getbysid

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

2015-11-02 Thread Sumit Bose 
On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
> On (29/10/15 15:32), Pavel Reichl wrote:
> >Hello,
> >
> >while I worked on tests for PAM responder Sumit proposed to use attached 
> >patch. QA have already kindly run their tests (successfully).
> >
> >I'll do the review myself (But all opinions are welcomed for sure).
> >
> >Thanks
> 
> >From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
> >From: Sumit Bose 
> >Date: Mon, 19 Oct 2015 13:10:51 -0400
> >Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS
> >
> >Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
> >set PAM_SUCCESSS explicitly for a successful authentication and will
> >really return an error in all other cases.
> >---
> > src/providers/dp_pam_data_util.c | 4 
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/src/providers/dp_pam_data_util.c 
> >b/src/providers/dp_pam_data_util.c
> >index 
> >10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
> > 100644
> >--- a/src/providers/dp_pam_data_util.c
> >+++ b/src/providers/dp_pam_data_util.c
> >@@ -22,6 +22,8 @@
> > along with this program.  If not, see .
> > */
> > 
> >+#include 
> >+
> We try to avoid including non-standar pam header files.
> Prefix "_" is Linux PAM specific.
> 

Fixed, new version attached.

bye,
Sumit
From fe1fce0f767f07ce4568de3cb86fc8823e5d66ec Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
 src/providers/dp_pam_data_util.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..bed5db872d18e0c3ab13d7b7fd061749253cc72a
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
 */
 
+#include 
+
 #include "providers/data_provider.h"
 #include "util/sss_cli_cmd.h"
 
@@ -48,6 +50,8 @@ struct pam_data *create_pam_data(TALLOC_CTX *mem_ctx)
 goto failed;
 }
 
+pd->pam_status = PAM_SYSTEM_ERR;
+
 pd->authtok = sss_authtok_new(pd);
 if (pd->authtok == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
-- 
2.1.0

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel