Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length

[Julien Gomes <julien@arista.com>]

On 2/6/19 1:39 PM, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 01:26:55PM -0800, Julien Gomes wrote:
>>
>>
>> On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
>>> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>>>
>>>>
>>>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>>>> structures longer than the current definitions.
>>>>>>
>>>>>> This should prevent unjustified setsockopt() failures due to struct
>>>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>>>> binaries that should be compatible, but were built with later kernel
>>>>>> uapi headers.
>>>>>
>>>>> Not sure if we support backwards compatibility like this?
>>>>>
>>>>> My issue with this change is that by doing this, application will have
>>>>> no clue if the new bits were ignored or not and it may think that an
>>>>> event is enabled while it is not.
>>>>>
>>>>> A workaround would be to do a getsockopt and check the size that was
>>>>> returned. But then, it might as well use the right struct here in the
>>>>> first place.
>>>>>
>>>>> I'm seeing current implementation as an implicitly versioned argument:
>>>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>>>> be rejected. Pretty much like using a newer setsockopt on an old
>>>>> system.
>>>>
>>>> With the current implementation, given sources that say are supposed to
>>>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>>>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>>>> them on 4.9 without messing with structures re-definition.
>>>
>>> Maybe what we want(ed) here then is explicit versioning, to have the 3
>>> definitions available. Then the application is able to use, say struct
>>> sctp_event_subscribe, and be happy with it, while there is struct
>>> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>>>
>>> But it's too late for that now because that would break applications
>>> already using the new fields in sctp_event_subscribe.
>>
>> Right.
>>
>>>
>>>>
>>>> I understand your point, but this still looks like a sort of uapi
>>>> breakage to me.
>>>
>>> Not disagreeing. I really just don't know how supported that is.
>>> Willing to know so I can pay more attention to this on future changes.
>>>
>>> Btw, is this the only occurrence?
>>
>> Can't really say, this is one I witnessed, I haven't really looked for
>> others.
>>
>>>
>>>>
>>>>
>>>> I also had another way to work-around this in mind, by copying optlen
>>>> bytes and checking that any additional field (not included in the
>>>> "current" kernel structure definition) is not set, returning EINVAL in
>>>> such case to keep a similar to current behavior.
>>>> The issue with this is that I didn't find a suitable (ie not totally
>>>> arbitrary such as "twice the existing structure size") upper limit to
>>>> optlen.
>>>
>>> Seems interesting. Why would it need that upper limit to optlen?
>>>
>>> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
>>> to the kernel that only knows about 4 bytes. It can check that (12-4)
>>> bytes in the end, make sure no bit is on and use only the first 4.
>>>
>>> The fact that it was 12 or 200 shouldn't matter, should it? As long as
>>> the (200-4) bytes are 0'ed, only the first 4 will be used and it
>>> should be ok, otherwise EINVAL. No need to know how big the current
>>> current actually is because it wouldn't be validating that here: just
>>> that it can safely use the first 4 bytes.
>>
>> The upper limit concern is more regarding the call to copy_from_user
>> with an unrestricted/unchecked value.
> Copy_from_user should be safe to copy an arbitrary amount, the only restriction
> is that optlen can't exceed the size of the buffer receiving the data in the
> kernel.  From that standpoint your patch is safe.  However,  that exposes the
> problem of checking any tail data on the userspace buffer.  That is to say, if
> you want to ensure that any extra data that gets sent from userspace isn't
> 'set', you would have to copy that extra data in consumable chunks and check
> them individaully, and that screams DOS to me (i.e. imagine a user passing in a
> 4GB buffer, and having to wait for the kernel to copy each X sized chunk,
> looking for non-zero values).

There probably is a decent compromise to find between "not accepting a
single additional byte" and accepting several GB.
For example how likely is it that the growth of this structure make it
go over a page? I would hope not at all.

By choosing a large but decent high limit, I think we can find a
future-compatible compromise that doesn't rely on a preliminary
getsockopt() just for structure trucation decision...

> 
>> I am not sure of how much of a risk/how exploitable this could be,
>> that's why I cautiously wanted to limit it in the first place just in case.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>>>>> ---
>>>>>>  net/sctp/socket.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>>>> --- a/net/sctp/socket.c
>>>>>> +++ b/net/sctp/socket.c
>>>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>>>>  	int i;
>>>>>>  
>>>>>>  	if (optlen > sizeof(struct sctp_event_subscribe))
>>>>>> -		return -EINVAL;
>>>>>> +		optlen = sizeof(struct sctp_event_subscribe);
>>>>>>  
>>>>>>  	if (copy_from_user(&subscribe, optval, optlen))
>>>>>>  		return -EFAULT;
>>>>>> -- 
>>>>>> 2.20.1
>>>>>>
>>>>
>>
>> -- 
>> Julien Gomes
>>

-- 
Julien Gomes