linux-kernel - Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

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:	Wed, 03 Apr 2013 17:14:31 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Sven Joachim <svenjoac@....de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	stable@...r.kernel.org, Ding Tianhong <dingtianhong@...wei.com>,
	Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

Andy Lutomirski <luto@...capital.net> writes:

> On Wed, Apr 3, 2013 at 11:43 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>> On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
>>
>>>
>>> This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
>>> race.  There's a fix (that needs both a new version from me and a review
>>> by someone) here:
>>>
>>> http://www.spinics.net/lists/netdev/msg229948.html
>>
>> Hmm... this is not a stable candidate, IMHO.
>
> Agreed.
>
>>
>> This has to be fixed (if needed) in a more easy way.
>>
>> What about this one liner ?
>>
>> CC Eric W. Biederman  as he wrote commit
>> dbe9a4173ea53b72b2c3
>> (scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
>>
>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index 975cca0..42359d8 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg,
>>                 return;
>>         }
>>
>> -       if (test_bit(SOCK_PASSCRED, &sock->flags)) {
>> +       if (test_bit(SOCK_PASSCRED, &sock->flags) && scm->creds.pid) {
>>                 struct user_namespace *current_ns = current_user_ns();
>>                 struct ucred ucreds = {
>>                         .pid = scm->creds.pid,
>>
>>
>
> That looks like it's correct.  If it gets applied, I'll respin my
> patches on top of it.
>
> (This approach may be a POSIX violation for all I know, and it's even
> possible that some really fragile userspace breaks.  But I doubt it,
> and anything that will break as a result is already operating in a
> highly confused state; hence the original problem.)

It certainly looks like we are not giving userspace what userspace asked
for, which can break in all kinds of subtle ways.  And I can't possibly
see how not giving udev any information will when udev asked for the
sender will fix anything.

I think we need to answer why in the world do we do not want to pass
credentials from an unconnected unix mode socket, before we ask
why don't we want to deliver credentials that we didn't pass when
passing of credentials was explicitly requested.

If the only concern about the LSB test case is performance I think we
need to revert the original commit and just stop passing a struct cred
pointer.  If there is a concern about the data I think we need a better
explanation of what those LSB test cases were that broke.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/