https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
On Tue, Aug 06, 2013 at 09:25:55PM +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Sorry I forgot to make it clear that this commit only applies on the 1.9 branch. The rewritten nested groups processing that is present on 1.10 and 1.11 doesn't suffer from this problem.
On Tue, Aug 06, 2013 at 09:35:22PM +0200, Jakub Hrozek wrote:
On Tue, Aug 06, 2013 at 09:25:55PM +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Sorry I forgot to make it clear that this commit only applies on the 1.9 branch. The rewritten nested groups processing that is present on 1.10 and 1.11 doesn't suffer from this problem.
Oh and one more remark -- Pavel suggested that we might even want to call tevent_req_post before tevent_req_done. Since tevent_req_post pretty much just schedules an immediate event, I think that would work, too, but I'm not 100% sure I'm not missing any detail, so I opted for this safer but uglier way, especially considering that the fix is only targeting a maintenance branch.
On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Hi, great catch! Sometimes I wish tevent_req_done() would schedule immediate event, instead of firing the callback directly.
Nack to the patch but ack to the solution. The same code is also in sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't be triggered, but who knows...
On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Hi, great catch! Sometimes I wish tevent_req_done() would schedule immediate event, instead of firing the callback directly.
Nack to the patch but ack to the solution. The same code is also in sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't be triggered, but who knows...
I think it can be triggered if all users are out of all search bases. New patch is attached.
On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Hi, great catch! Sometimes I wish tevent_req_done() would schedule immediate event, instead of firing the callback directly.
Nack to the patch but ack to the solution. The same code is also in sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't be triggered, but who knows...
I think it can be triggered if all users are out of all search bases. New patch is attached.
Ack.
BTW there was lately several use-after-free issues found by users. We should start testing our patches with TALLOC_FREE_FILL=0 - this will overwrite freed memory with zeros on talloc_free() so if there is anything like that in covered code, we will hit that.
On Wed, Aug 07, 2013 at 11:36:37PM +0200, Pavel Březina wrote:
On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Hi, great catch! Sometimes I wish tevent_req_done() would schedule immediate event, instead of firing the callback directly.
Nack to the patch but ack to the solution. The same code is also in sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't be triggered, but who knows...
I think it can be triggered if all users are out of all search bases. New patch is attached.
Ack.
Pushed to sssd-1-9
BTW there was lately several use-after-free issues found by users. We should start testing our patches with TALLOC_FREE_FILL=0 - this will overwrite freed memory with zeros on talloc_free() so if there is anything like that in covered code, we will hit that.
Yep :-)
We should also be using MALLOC_PERTURB, which does the same thing for mallocated memory, making it easier to detect uninitialized access to heap memory.
Can you add a note about TALLOC_FREE_FILL to https://fedorahosted.org/sssd/wiki/DevelTips ?
On (08/08/13 00:50), Jakub Hrozek wrote:
On Wed, Aug 07, 2013 at 11:36:37PM +0200, Pavel Březina wrote:
On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Hi, great catch! Sometimes I wish tevent_req_done() would schedule immediate event, instead of firing the callback directly.
Nack to the patch but ack to the solution. The same code is also in sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't be triggered, but who knows...
I think it can be triggered if all users are out of all search bases. New patch is attached.
Ack.
Pushed to sssd-1-9
BTW there was lately several use-after-free issues found by users. We should start testing our patches with TALLOC_FREE_FILL=0 - this will overwrite freed memory with zeros on talloc_free() so if there is anything like that in covered code, we will hit that.
Yep :-)
We should also be using MALLOC_PERTURB, which does the same thing for mallocated memory, making it easier to detect uninitialized access to heap memory.
Can you add a note about TALLOC_FREE_FILL to https://fedorahosted.org/sssd/wiki/DevelTips ?
+1 but I would use different value than 0, because it will be impossible to distinguish between zero initialized memory and released memory (use after free)
I would prefer very big value for example 0xFD (FreeD memory)
LS
On Thu, Aug 08, 2013 at 08:33:17AM +0200, Lukas Slebodnik wrote:
On (08/08/13 00:50), Jakub Hrozek wrote:
On Wed, Aug 07, 2013 at 11:36:37PM +0200, Pavel Březina wrote:
On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1932
There is a rather strange workaround in the nested groups processing code that calls tevent_req_post outside _send(). However, it broke in certain situations where the tevent_req_call resulted in req being freed, which freed state by extension and then the subsequent _post call was a use-after-free. This patch saves the two variables used outside state so that it's safe to use them even after the callback.
Hi, great catch! Sometimes I wish tevent_req_done() would schedule immediate event, instead of firing the callback directly.
Nack to the patch but ack to the solution. The same code is also in sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't be triggered, but who knows...
I think it can be triggered if all users are out of all search bases. New patch is attached.
Ack.
Pushed to sssd-1-9
BTW there was lately several use-after-free issues found by users. We should start testing our patches with TALLOC_FREE_FILL=0 - this will overwrite freed memory with zeros on talloc_free() so if there is anything like that in covered code, we will hit that.
Yep :-)
We should also be using MALLOC_PERTURB, which does the same thing for mallocated memory, making it easier to detect uninitialized access to heap memory.
Can you add a note about TALLOC_FREE_FILL to https://fedorahosted.org/sssd/wiki/DevelTips ?
+1 but I would use different value than 0, because it will be impossible to distinguish between zero initialized memory and released memory (use after free)
I would prefer very big value for example 0xFD (FreeD memory)
LS
You can even use random values:
export MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina