[suggestion] tcache double-free check

[suggestion] tcache double-free check

[Eyal Itkin]

Hello,

Going over the internals of the tcache entries, I stumbled upon the
entry->key field used for double-free checks. The full thread behind
this field can be found here:
http://sourceware-org.1504.n7.nabble.com/patch-tcache-double-free-check-td544878.html.

While the double-free check is a good idea, I think that Florian was
correct when he asked about the reason behind storing pointers to the
tcache control block on the heap itself. In the current
implementation, free()ed tcache allocations will contain a pointer to
the tcache control block, thus exposing it to corruption in case the
programmer mistakenly used the allocation after it was freed / freed
some buffers in the wrong way.

The reason behind using "tcache" as the entry key was explained by
Delorie (the developer of this patch):
"
... The value is arbitrary, it can be anything that we can argue won't
come up in usual program flows.
"

Instead of using some arbitrary constant or coming up with a fancy
random value, is it possible we update the key to something that won't
point to a critical memory management struct such as the tcache
control block? I suggest a simple change that will ensure that the
value used won't be a pointer that can be dereferenced: ~tcache
(instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
making sure the key won't be a valid memory address.

Before submitting a patch for this change, I wanted to hear your
opinion about it.
Thanks, and credit to Awarau for pointing this out to me.
Eyal.

Re: [suggestion] tcache double-free check

[Carlos O'Donell]

On 7/19/20 2:40 AM, Eyal Itkin via Libc-alpha wrote:
> Hello,
> 
> Going over the internals of the tcache entries, I stumbled upon the
> entry->key field used for double-free checks. The full thread behind
> this field can be found here:
> http://sourceware-org.1504.n7.nabble.com/patch-tcache-double-free-check-td544878.html.

Just for reference the ML list URL is here:
https://sourceware.org/pipermail/libc-alpha/2018-November/098357.html

> While the double-free check is a good idea, I think that Florian was
> correct when he asked about the reason behind storing pointers to the
> tcache control block on the heap itself. In the current
> implementation, free()ed tcache allocations will contain a pointer to
> the tcache control block, thus exposing it to corruption in case the
> programmer mistakenly used the allocation after it was freed / freed
> some buffers in the wrong way.
> 
> The reason behind using "tcache" as the entry key was explained by
> Delorie (the developer of this patch):
> "
> ... The value is arbitrary, it can be anything that we can argue won't
> come up in usual program flows.
> "

Correct.

> Instead of using some arbitrary constant or coming up with a fancy
> random value, is it possible we update the key to something that won't
> point to a critical memory management struct such as the tcache
> control block? I suggest a simple change that will ensure that the
> value used won't be a pointer that can be dereferenced: ~tcache
> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
> making sure the key won't be a valid memory address.

That sounds good to me.

I assume the point being that you can't use a "memory derefernce"
gadget directly with that memory, you'd need some other primitive
to process the ~tcache.

> Before submitting a patch for this change, I wanted to hear your
> opinion about it.
> Thanks, and credit to Awarau for pointing this out to me.
> Eyal.

I have no objections. I'd like to hear Florian's input (on TO).

-- 
Cheers,
Carlos.

Re: [suggestion] tcache double-free check

[Florian Weimer]

* Carlos O'Donell:

>> Instead of using some arbitrary constant or coming up with a fancy
>> random value, is it possible we update the key to something that won't
>> point to a critical memory management struct such as the tcache
>> control block? I suggest a simple change that will ensure that the
>> value used won't be a pointer that can be dereferenced: ~tcache
>> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
>> making sure the key won't be a valid memory address.
>
> That sounds good to me.
>
> I assume the point being that you can't use a "memory derefernce"
> gadget directly with that memory, you'd need some other primitive
> to process the ~tcache.

Why can't we use a random marker value?  Then we don't leak an address,
either.

Thanks,
Florian

Re: [suggestion] tcache double-free check

[Carlos O'Donell]

On 7/21/20 2:03 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> Instead of using some arbitrary constant or coming up with a fancy
>>> random value, is it possible we update the key to something that won't
>>> point to a critical memory management struct such as the tcache
>>> control block? I suggest a simple change that will ensure that the
>>> value used won't be a pointer that can be dereferenced: ~tcache
>>> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
>>> making sure the key won't be a valid memory address.
>>
>> That sounds good to me.
>>
>> I assume the point being that you can't use a "memory derefernce"
>> gadget directly with that memory, you'd need some other primitive
>> to process the ~tcache.
> 
> Why can't we use a random marker value?  Then we don't leak an address,
> either.

I'm not against it, but we'd need something that is random, and for that
we need entropy. What have we got to use?

-- 
Cheers,
Carlos.

Re: [suggestion] tcache double-free check

[Adhemerval Zanella]

On 22/07/2020 23:35, Carlos O'Donell via Libc-alpha wrote:
> On 7/21/20 2:03 AM, Florian Weimer wrote:
>> * Carlos O'Donell:
>>
>>>> Instead of using some arbitrary constant or coming up with a fancy
>>>> random value, is it possible we update the key to something that won't
>>>> point to a critical memory management struct such as the tcache
>>>> control block? I suggest a simple change that will ensure that the
>>>> value used won't be a pointer that can be dereferenced: ~tcache
>>>> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
>>>> making sure the key won't be a valid memory address.
>>>
>>> That sounds good to me.
>>>
>>> I assume the point being that you can't use a "memory derefernce"
>>> gadget directly with that memory, you'd need some other primitive
>>> to process the ~tcache.
>>
>> Why can't we use a random marker value?  Then we don't leak an address,
>> either.
> 
> I'm not against it, but we'd need something that is random, and for that
> we need entropy. What have we got to use?

We have include/random-bits.h which get some entropy from the clock
(and shuffle the bits to avoid some clock bias). It is far from perfect,
but it is used internally on some specific cases.

Another possibility if a more robust random source is requires would be
to try work on the arc4random patch proposal from Florian.

Re: [suggestion] tcache double-free check

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 22/07/2020 23:35, Carlos O'Donell via Libc-alpha wrote:
>> On 7/21/20 2:03 AM, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>>> Instead of using some arbitrary constant or coming up with a fancy
>>>>> random value, is it possible we update the key to something that won't
>>>>> point to a critical memory management struct such as the tcache
>>>>> control block? I suggest a simple change that will ensure that the
>>>>> value used won't be a pointer that can be dereferenced: ~tcache
>>>>> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
>>>>> making sure the key won't be a valid memory address.
>>>>
>>>> That sounds good to me.
>>>>
>>>> I assume the point being that you can't use a "memory derefernce"
>>>> gadget directly with that memory, you'd need some other primitive
>>>> to process the ~tcache.
>>>
>>> Why can't we use a random marker value?  Then we don't leak an address,
>>> either.
>> 
>> I'm not against it, but we'd need something that is random, and for that
>> we need entropy. What have we got to use?
>
> We have include/random-bits.h which get some entropy from the clock
> (and shuffle the bits to avoid some clock bias). It is far from perfect,
> but it is used internally on some specific cases.
>
> Another possibility if a more robust random source is requires would be
> to try work on the arc4random patch proposal from Florian.

For a one-time initialization, we could call getrandom directly.  (And
hope that seccomp filters do not terminate the process.)

Thanks,
Florian

Re: [suggestion] tcache double-free check

[Carlos O'Donell]

On 7/23/20 8:06 AM, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 22/07/2020 23:35, Carlos O'Donell via Libc-alpha wrote:
>>> On 7/21/20 2:03 AM, Florian Weimer wrote:
>>>> * Carlos O'Donell:
>>>>
>>>>>> Instead of using some arbitrary constant or coming up with a fancy
>>>>>> random value, is it possible we update the key to something that won't
>>>>>> point to a critical memory management struct such as the tcache
>>>>>> control block? I suggest a simple change that will ensure that the
>>>>>> value used won't be a pointer that can be dereferenced: ~tcache
>>>>>> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
>>>>>> making sure the key won't be a valid memory address.
>>>>>
>>>>> That sounds good to me.
>>>>>
>>>>> I assume the point being that you can't use a "memory derefernce"
>>>>> gadget directly with that memory, you'd need some other primitive
>>>>> to process the ~tcache.
>>>>
>>>> Why can't we use a random marker value?  Then we don't leak an address,
>>>> either.
>>>
>>> I'm not against it, but we'd need something that is random, and for that
>>> we need entropy. What have we got to use?
>>
>> We have include/random-bits.h which get some entropy from the clock
>> (and shuffle the bits to avoid some clock bias). It is far from perfect,
>> but it is used internally on some specific cases.
>>
>> Another possibility if a more robust random source is requires would be
>> to try work on the arc4random patch proposal from Florian.
> 
> For a one-time initialization, we could call getrandom directly.  (And
> hope that seccomp filters do not terminate the process.)

I'm OK with that.

-- 
Cheers,
Carlos.

Re: [suggestion] tcache double-free check

[Eyal Itkin]

OK, in a few days I will send a patch that will use a direct call to
getrandom so to init the key.
As the function call may fail (maybe there won't be enough entropy),
on failure it will default to the alternative suggestion of using
"!tcache" instead of "tcache".


On Fri, Jul 24, 2020 at 12:26 AM Carlos O'Donell via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On 7/23/20 8:06 AM, Florian Weimer wrote:
> > * Adhemerval Zanella via Libc-alpha:
> >
> >> On 22/07/2020 23:35, Carlos O'Donell via Libc-alpha wrote:
> >>> On 7/21/20 2:03 AM, Florian Weimer wrote:
> >>>> * Carlos O'Donell:
> >>>>
> >>>>>> Instead of using some arbitrary constant or coming up with a fancy
> >>>>>> random value, is it possible we update the key to something that won't
> >>>>>> point to a critical memory management struct such as the tcache
> >>>>>> control block? I suggest a simple change that will ensure that the
> >>>>>> value used won't be a pointer that can be dereferenced: ~tcache
> >>>>>> (instead of tcache). The bitwise not costs a mere 1 CPU cycle, while
> >>>>>> making sure the key won't be a valid memory address.
> >>>>>
> >>>>> That sounds good to me.
> >>>>>
> >>>>> I assume the point being that you can't use a "memory derefernce"
> >>>>> gadget directly with that memory, you'd need some other primitive
> >>>>> to process the ~tcache.
> >>>>
> >>>> Why can't we use a random marker value?  Then we don't leak an address,
> >>>> either.
> >>>
> >>> I'm not against it, but we'd need something that is random, and for that
> >>> we need entropy. What have we got to use?
> >>
> >> We have include/random-bits.h which get some entropy from the clock
> >> (and shuffle the bits to avoid some clock bias). It is far from perfect,
> >> but it is used internally on some specific cases.
> >>
> >> Another possibility if a more robust random source is requires would be
> >> to try work on the arc4random patch proposal from Florian.
> >
> > For a one-time initialization, we could call getrandom directly.  (And
> > hope that seccomp filters do not terminate the process.)
>
> I'm OK with that.
>
> --
> Cheers,
> Carlos.
>

Re: [suggestion] tcache double-free check

[Carlos O'Donell]

On 7/23/20 6:07 PM, Eyal Itkin wrote:
> OK, in a few days I will send a patch that will use a direct call to
> getrandom so to init the key.
> As the function call may fail (maybe there won't be enough entropy),
> on failure it will default to the alternative suggestion of using
> "!tcache" instead of "tcache".

That sounds good to me. Thanks.

-- 
Cheers,
Carlos.

Re: [suggestion] tcache double-free check

[Adhemerval Zanella]

On 23/07/2020 19:07, Eyal Itkin via Libc-alpha wrote:
> OK, in a few days I will send a patch that will use a direct call to
> getrandom so to init the key.
> As the function call may fail (maybe there won't be enough entropy),
> on failure it will default to the alternative suggestion of using
> "!tcache" instead of "tcache".

You could also get some entropy from random-bits.h, although I am not
sure if clock entropy is suffice for this specific case.