netdev - Re: [PATCH net] sctp: assign assoc_id earlier in __sctp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Nov 2016 09:10:46 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        Vlad Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        "syzkaller@...glegroups.com" <syzkaller@...glegroups.com>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "glider@...gle.com" <glider@...gle.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "dvyukov@...gle.com" <dvyukov@...gle.com>,
        "andreyknvl@...gle.com" <andreyknvl@...gle.com>
Subject: Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect

Em 04-11-2016 08:55, David Laight escreveu:
> From: Of Marcelo Ricardo Leitner
>> Sent: 03 November 2016 19:04
>> sctp_wait_for_connect() currently already holds the asoc to keep it
>> alive during the sleep, in case another thread release it. But Andrey
>> Konovalov and Dmitry Vyukov reported an use-after-free in such
>> situation.
>>
>> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
>> do a read on the asoc after calling sctp_wait_for_connect(), but by then
>> another thread may have closed it and the _put on sctp_wait_for_connect
>> will actually release it, causing the use-after-free.
>>
>> Fix is, instead of doing the read after waiting for the connect, do it
>> before so, and avoid this issue as the socket is still locked by then.
>> There should be no issue on returning the asoc id in case of failure as
>> the application shouldn't trust on that number in such situations
>> anyway.
>>
>> This issue doesn't exist in sctp_sendmsg() path.
>>
>> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
>> Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
>> Tested-by: Andrey Konovalov <andreyknvl@...gle.com>
>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>> ---
>>  net/sctp/socket.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>>
>>  	timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>>
>> -	err = sctp_wait_for_connect(asoc, &timeo);
>> -	if ((err == 0 || err == -EINPROGRESS) && assoc_id)
>> +	if (assoc_id)
>>  		*assoc_id = asoc->assoc_id;
>> +	err = sctp_wait_for_connect(asoc, &timeo);
>> +	/* Note: the asoc may be freed after the return of
>> +	 * sctp_wait_for_connect.
>> +	 */
>
> Is it worth ensuring that *assoc_id is NULL on error?

I don't think so. An error was returned, the value shouldn't be trusted 
anyway and it's not leaking any sort of critical data.

Note that original code doesn't touch assoc_id in case of error. It's 
different than zeroing it out.

> Maybe change the code to:
> 	assoc_id_val = asoc->assoc_id;
> 	rval = sctp_wait_for_connect(asoc, &timeo);
> 	if (err != 0 && err != -EINPROGRESS)
> 		assoc_id_val = 0;
> 	if (assoc_id)
> 		*assoc_id = assoc_id_val;

Or just clear it in case of error..
  	if (assoc_id && (err != 0 && err != -EINPROGRESS))
  		*assoc_id = 0;
Amount of code is probably the same but avoids a temporary var.

   Marcelo