sgx_validate_offset_length bug

sgx_validate_offset_length bug

[Borys]

Hi,

I've stumbled upon "sgx_validate_offset_length" function in "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 version), which does not entirely do what it claims. "offset" and "length" parameters are provided by userspace and as such their addition can overflow, which may result in this function approving malicious values. Fortunately this does not result in any exploitable bugs at the moment (or at least I couldn't find any), but this might change if "sgx_validate_offset_length" is used in a new context or current usages are changed, so it might be worth fixing anyway. Simple overflow check `offset + length < offset` should be enough.

Best regards,

Borys

Re: sgx_validate_offset_length bug

[Reinette Chatre]

Hi Borys,

On 10/3/2022 10:19 AM, Borys wrote:
> I've stumbled upon "sgx_validate_offset_length" function in
> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
> version), which does not entirely do what it claims. "offset" and
> "length" parameters are provided by userspace and as such their
> addition can overflow, which may result in this function approving
> malicious values. Fortunately this does not result in any exploitable
> bugs at the moment (or at least I couldn't find any), but this might
> change if "sgx_validate_offset_length" is used in a new context or
> current usages are changed, so it might be worth fixing anyway.
> Simple overflow check `offset + length < offset` should be enough.> 

Could you please elaborate where you see a possibility for overflow?

Together the provided values, offset and length, are already ensured to
not exceed the total size of the enclave in the following check:

sgx_validate_offset_length() {
	...
	if (offset + length - PAGE_SIZE >= encl->size)
		return -EINVAL;
	...
}

Reinette

Re: sgx_validate_offset_length bug

[Reinette Chatre]

Hi Borys,

On 10/3/2022 10:33 AM, Reinette Chatre wrote:
> On 10/3/2022 10:19 AM, Borys wrote:
>> I've stumbled upon "sgx_validate_offset_length" function in
>> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
>> version), which does not entirely do what it claims. "offset" and
>> "length" parameters are provided by userspace and as such their
>> addition can overflow, which may result in this function approving
>> malicious values. Fortunately this does not result in any exploitable
>> bugs at the moment (or at least I couldn't find any), but this might
>> change if "sgx_validate_offset_length" is used in a new context or
>> current usages are changed, so it might be worth fixing anyway.
>> Simple overflow check `offset + length < offset` should be enough.> 
> 
> Could you please elaborate where you see a possibility for overflow?
> 
> Together the provided values, offset and length, are already ensured to
> not exceed the total size of the enclave in the following check:
> 
> sgx_validate_offset_length() {
> 	...
> 	if (offset + length - PAGE_SIZE >= encl->size)
> 		return -EINVAL;
> 	...
> }

I think I see what you mean now ... if offset and length are
sufficiently large the above check can still pass but loops 
that have the following pattern may have issues:

for (c = 0 ; c < length; c += PAGE_SIZE) {

	... 
	/* do something at <offset> */

}

Are you planning to submit a patch for the check you propose?

Reinette

Re: sgx_validate_offset_length bug

[Borys]

Hi,

On 10/3/22 19:58, Reinette Chatre wrote:
> Hi Borys,
>
> On 10/3/2022 10:33 AM, Reinette Chatre wrote:
>> On 10/3/2022 10:19 AM, Borys wrote:
>>> I've stumbled upon "sgx_validate_offset_length" function in
>>> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
>>> version), which does not entirely do what it claims. "offset" and
>>> "length" parameters are provided by userspace and as such their
>>> addition can overflow, which may result in this function approving
>>> malicious values. Fortunately this does not result in any exploitable
>>> bugs at the moment (or at least I couldn't find any), but this might
>>> change if "sgx_validate_offset_length" is used in a new context or
>>> current usages are changed, so it might be worth fixing anyway.
>>> Simple overflow check `offset + length < offset` should be enough.>
>> Could you please elaborate where you see a possibility for overflow?
>>
>> Together the provided values, offset and length, are already ensured to
>> not exceed the total size of the enclave in the following check:
>>
>> sgx_validate_offset_length() {
>> 	...
>> 	if (offset + length - PAGE_SIZE >= encl->size)
>> 		return -EINVAL;
>> 	...
>> }
> I think I see what you mean now ... if offset and length are
> sufficiently large the above check can still pass but loops
> that have the following pattern may have issues:
>
> for (c = 0 ; c < length; c += PAGE_SIZE) {
>
> 	...
> 	/* do something at <offset> */
>
> }
>
> Are you planning to submit a patch for the check you propose?
>
> Reinette

Sure, I'll try to submit a patch later today.

Borys

Re: sgx_validate_offset_length bug

[Reinette Chatre]

Hi Borys,

On 10/4/2022 6:22 AM, Borys wrote:
> On 10/3/22 19:58, Reinette Chatre wrote:
>> On 10/3/2022 10:33 AM, Reinette Chatre wrote:
>>> On 10/3/2022 10:19 AM, Borys wrote:
>>>> I've stumbled upon "sgx_validate_offset_length" function in
>>>> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
>>>> version), which does not entirely do what it claims. "offset" and
>>>> "length" parameters are provided by userspace and as such their
>>>> addition can overflow, which may result in this function approving
>>>> malicious values. Fortunately this does not result in any exploitable
>>>> bugs at the moment (or at least I couldn't find any), but this might
>>>> change if "sgx_validate_offset_length" is used in a new context or
>>>> current usages are changed, so it might be worth fixing anyway.
>>>> Simple overflow check `offset + length < offset` should be enough.>
>>> Could you please elaborate where you see a possibility for overflow?
>>>
>>> Together the provided values, offset and length, are already ensured to
>>> not exceed the total size of the enclave in the following check:
>>>
>>> sgx_validate_offset_length() {
>>>     ...
>>>     if (offset + length - PAGE_SIZE >= encl->size)
>>>         return -EINVAL;
>>>     ...
>>> }
>> I think I see what you mean now ... if offset and length are
>> sufficiently large the above check can still pass but loops
>> that have the following pattern may have issues:
>>
>> for (c = 0 ; c < length; c += PAGE_SIZE) {
>>
>>     ...
>>     /* do something at <offset> */
>>
>> }
>>
>> Are you planning to submit a patch for the check you propose?
>>
>> Reinette
> 
> Sure, I'll try to submit a patch later today.

Thank you very much.

Please do take care when determining the "Fixes" tag. You identified
the issue within sgx_validate_offset_length() but please note that
this is a function recently introduced by refactoring code that has been
in SGX since the beginning. Please see commit:
dda03e2c331b ("x86/sgx: Create utility to validate user
provided offset and length")

While the initial fix will be to sgx_validate_offset_length()
care should be taken that the fix also propagates to older kernels that
do not have this utility. Either a new fix can be created for older
kernels or perhaps the stable team could backport dda03e2c331b
together with your fix. There are ways to create a patch that
communicates this to stable team's automation.

Reinette

Re: sgx_validate_offset_length bug

[Jarkko Sakkinen]

On Mon, Oct 03, 2022 at 07:19:21PM +0200, Borys wrote:
> Hi,
> 
> I've stumbled upon "sgx_validate_offset_length" function in
> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
> version), which does not entirely do what it claims. "offset" and
> "length" parameters are provided by userspace and as such their addition
> can overflow, which may result in this function approving malicious
> values. Fortunately this does not result in any exploitable bugs at the
> moment (or at least I couldn't find any), but this might change if
> "sgx_validate_offset_length" is used in a new context or current usages
> are changed, so it might be worth fixing anyway. Simple overflow check
> `offset + length < offset` should be enough.
> 
> Best regards,
> 
> Borys
> 

I agree with the bug but not on security issue.

If you can call the ioctl API in the first place, you can already apply
the operations in arbitrary locations inside the enclave, i.e. it does
not introduce any new capability to the untrusted runtime.

BR, Jarkko

Re: sgx_validate_offset_length bug

[Borys]

On 10/4/22 23:50, Jarkko Sakkinen wrote:
> On Mon, Oct 03, 2022 at 07:19:21PM +0200, Borys wrote:
>> Hi,
>>
>> I've stumbled upon "sgx_validate_offset_length" function in
>> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
>> version), which does not entirely do what it claims. "offset" and
>> "length" parameters are provided by userspace and as such their addition
>> can overflow, which may result in this function approving malicious
>> values. Fortunately this does not result in any exploitable bugs at the
>> moment (or at least I couldn't find any), but this might change if
>> "sgx_validate_offset_length" is used in a new context or current usages
>> are changed, so it might be worth fixing anyway. Simple overflow check
>> `offset + length < offset` should be enough.
>>
>> Best regards,
>>
>> Borys
>>
> 
> I agree with the bug but not on security issue.
> 
> If you can call the ioctl API in the first place, you can already apply
> the operations in arbitrary locations inside the enclave, i.e. it does
> not introduce any new capability to the untrusted runtime.
> 
> BR, Jarkko

I meant it could possibly enable some local priv escalation, if other
code has wrong assumptions. But again, this is purely theoretical,
current usages fail on invalid values anyway.

Best regards,
Borys

Re: sgx_validate_offset_length bug

[Jarkko Sakkinen]

On Wed, Oct 05, 2022 at 01:02:39AM +0200, Borys wrote:
> On 10/4/22 23:50, Jarkko Sakkinen wrote:
> > On Mon, Oct 03, 2022 at 07:19:21PM +0200, Borys wrote:
> >> Hi,
> >>
> >> I've stumbled upon "sgx_validate_offset_length" function in
> >> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7
> >> version), which does not entirely do what it claims. "offset" and
> >> "length" parameters are provided by userspace and as such their addition
> >> can overflow, which may result in this function approving malicious
> >> values. Fortunately this does not result in any exploitable bugs at the
> >> moment (or at least I couldn't find any), but this might change if
> >> "sgx_validate_offset_length" is used in a new context or current usages
> >> are changed, so it might be worth fixing anyway. Simple overflow check
> >> `offset + length < offset` should be enough.
> >>
> >> Best regards,
> >>
> >> Borys
> >>
> > 
> > I agree with the bug but not on security issue.
> > 
> > If you can call the ioctl API in the first place, you can already apply
> > the operations in arbitrary locations inside the enclave, i.e. it does
> > not introduce any new capability to the untrusted runtime.
> > 
> > BR, Jarkko
> 
> I meant it could possibly enable some local priv escalation, if other
> code has wrong assumptions. But again, this is purely theoretical,
> current usages fail on invalid values anyway.
> 
> Best regards,
> Borys

Yeah, in all cases it needs to be fixed. Thanks for pointing it out.

BR, Jarkko