[SSSD] Patch to fix LDAP ID backend GSSAPI credential expired messages

Eugene Indenbom eindenbom at gmail.com
Tue Apr 13 15:38:27 UTC 2010


Hi Simo,

On 04/13/2010 06:44 PM, Simo Sorce wrote:
> On Tue, 13 Apr 2010 09:28:28 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>
>    
>> Hi Stephen and Simo,
>>
>> On 04/13/2010 12:23 AM, Stephen Gallagher wrote:
>>      
>>> On 04/07/2010 10:10 AM, Eugene Indenbom wrote:
>>>
>>>        
>>>> --- 0003-Fixed-recursive-sdap_handle-disconnect-sequence-from.patch
>>>>
>>>> This patch fixes a critical bug of accessing freed memory in
>>>> sdap_handle_release. The problem can be reproduced as follows:
>>>> 1. Break LDAP connection when LDAP query is in progress (e.g. by
>>>> restarting directory server)
>>>> 2. sdap_process_result detects communication error and calls
>>>> sdap_handle_release
>>>> 3. sdap_handle_release calls active sdap_op callback
>>>> 4. callback deletes sdap_handle
>>>> 5. sdap_release_handle segfaults or asserts on access to
>>>> deallocated memory
>>>>
>>>>
>>>>          
>>> I am unable to find any code path such that step 4 (callback deletes
>>> sdap_handle) is possible. Every callback that we ever add with
>>> sdap_op_add() behaves the same way when called from
>>> sdap_handle_release().
>>>
>>> Since sdap_handle_release() invokes the callbacks with the error
>>> code of EIO, they all immediately set tevent_req_error() and
>>> return. There is no path in which they delete any memory.
>>>
>>> Furthermore, all sdap_op_add() calls use a memory heirarchy that is
>>> either below or completely unrelated to the sdap_handle hierarchy.
>>> So calling talloc_free() on the sdap_op object cannot free the
>>> sdap_handle.
>>>
>>> Eugene, if you are sure that this exists in the current master, can
>>> you point out the relevant problem area?
>>>
>>>        
>> Yes, I am sure. The callback deleting the sdap_handle structure is
>> not direct callback to sdap_op, but chain callback on associated
>> tevent_req. The complete list of offending callbacks is:
>>       - sdap_account_info_users_done
>>       - sdap_account_info_groups_done
>>       - sdap_account_info_initgr_done
>>       - ldap_id_enum_users_done
>>       - ldap_id_enum_groups_done
>>
>> The crash itself happens in sdap_release_connection.
>>      
> Well sdap_release_connection does not exist in current master :-)
>    
I am sorry for a typo mistake. The function I am talking about is 
sdap_handle_release.
> However thanks to this explanation I think I see where the problem lies.
>
> In these callbacks we call sdap_mark_offline() on some errors, and that
> frees ctx->gsh which causes us this nice loop:
>
> sdap_process_result() ->  sdap_handle_release() ->  callback ->
> sdap_mark_offline() ->  talloc_zfree() ->  sdap_handle_destructor() ->
> sdap_handle_release()
>    
talloc_zfree(sdap_ctx->gsh) is called in many other places as well. All 
reconnect code relies on timely destruction of singleton connection 
(sdap_ctx->gsh). For example take a look at code right after calls to 
sdap_check_gssapi_reconnect.
> And so on depending on how many ops we have.
>
> The solution though is much simpler than the patch you propose, I will
> remove the free from sdap_mark_offline() and replace it with marking
> sdap_handle as to be released, then free it at the end of
> sdap_handle_release()
>    
You can not really rely on the fact that no code calls 
talloc_zfree(sdap_ctx->gsh). This is potential source of future errors. 
Somebody will call it and we will never find out until crash report comes.
> I will also move the current sdap_handle_release() code in
> sdap_handle_release_internal() so that it can be called from the
> destructor without us attempting to free sh within the destructor
> itself.
>
>    
This would not help. I have evaluated this solution. The problem is:
1. The top level call to sdap_handle_release is not a destructor. It's 
bare disconnect.
2. The memory is freed after inner call to sdap_handle_destructor 
completes. You can not change this.
3. When sdap_handle_release iterates over ops and calls callbacks on 
disconnect, it has to have an access to sdap_handle->ops member 
__after__ callback returns. This means that memory sdap_handle->ops 
occupies must survive destructor. That's exactly what my patch does.

>> Actually, I have already sent a patch fixing the issue (please, take
>> a look at it) and it is explained in the fix and accompanying mail
>> how does crash happens and how it has been fixed.
>>      
> Yes I looked at the patch but it didn't make things any clearer, and
> introduced changes that were a bit more invasive than needed IMO.
>
>    
>> Steps to reproduce are extremely simple:
>> 1. Using debugger break in sdap_generic_send
>> 2. Restart LDAP server or network connection leading to it
>> 3. Resume execution
>> 4. Observe crash.
>>      
> I will send an alternative patch shortly.
> Then will resume my review of your patches.
>    
Please, before sending your patch, repeat the test scenario I have 
described above and you will still have a crash. Unless you really 
manage to kill in your patch all occurrences of 
talloc_zfree(sdap_ctx->gsh).
> 1 and 2 looks reasonable although there are 2 minor nitpicks.
> Your email address is probably wrong. And you add trailing spaces in
> some lines.
>    
I am sorry for trailing spaces. It has taken some time to set up 
development environment. I am starting a new Linux project after more 
than 3 years without Linux. So it takes time to get all tools at hand.

I'll find all such formatting errors and correct them.

In what places the e-mail is wrong? It is eindenbom at gmail.com. I have 
sent a test e-mail to myself and appears to work. Sorry about that too.

> On the 4th I'll try to do a bit more analysis later, but it looks a bit
> too complicated and out of style. In particular I don't like much the
> sdap_id_op idea and the associated _create() function. I will try to
> give some more constructive feedback on it later.
>    

OK. One more argument, sdap_id_op is needed for:
1. To keep track of current connection.
2. To reference count the connection (otherwise it would be 
indeterminable when to destroy connection).
3. To keep track of number of retries.

This does not amount to much work and responsibility, but nevertheless 
it does not make sense to repeat retry tracking and other staff in upper 
logical layers.

Regards, Eugene



More information about the sssd-devel mailing list