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:47:51 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Sven Joachim <svenjoac@....de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.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>,
	Andy Lutomirski <luto@...capital.net>,
	Karel Srot <ksrot@...hat.com>
Subject: Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

Eric Dumazet <eric.dumazet@...il.com> writes:

> On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
>> Sven Joachim <svenjoac@....de> writes:
>> 
>> > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
>> >
>> >> 3.8-stable review patch.  If anyone has any objections, please let me know.
>> >
>> > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
>> > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored".  Reverting the
>> > patch below on top of 3.8.6-rc1 fixes that.  I'm using udev version 175
>> > here, and 65534 is the uid of user "nobody".
>> 
>> Hmm.
>> 
>> Ok.  I don't understand the commit that was being backported here.  I am
>> pretty certain it a fix for a problem that did not exist.
>> 
>> Unless I am completely mis-reading scm_recv we only generate a
>> SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
>> Which means that the only harm that can come from adding scm credentials
>> to a disconnected af_unix socket is a loss in efficiency.
>> 
>> Not adding scm credentials to be passed to userspace as the commit below
>> is doing can result is bogus data being passed to userspace.  Which is
>> very actively WRONG.
>> 
>> Now before scm_recv does anything we first call scm_set_cred.  If no
>> credential was passed to scm_set_cred we set the uid to INVALID_UID.
>> Which scm_recv in the call from_kuid_munged translates into 65534 for
>> reporting to userspace.
>> 
>> So this is is pretty clearly a case of us not sending the unix
>> credentials.
>> 
>> Since not sending credential is just a performance optimization I can
>> see no earthly reason why the commit below should have been applied in
>> the first place, and no reason why it should have been backported in the
>> second place.  So my vote is that we revert this bogus commit.  Upstream
>> and then backport the revert.
>> 
>> Am I missing something?
>
> Well, yes, this commit fixes a real bug : We were coalescing two
> messages into a single one, even if the senders were different.

What???

As far as I can tell this patch can only server to _allow_ coalescing two
messages into a single one.

> Copy of a reply I did :
>
> So the problem is that two messages have different credentials,
> because other->sk_socket changed between first and second message.


> and unix_stream_recvmsg() has the following check :
>
>                 if (check_creds) {
>                         /* Never glue messages from different writers */
>                         if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
>                             (UNIXCB(skb).cred != siocb->scm->cred))
>                                 break;
>                 } else {
>                         /* Copy credentials */
>                         scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
>                         check_creds = 1;
>                 }
>
> So the patch was good, and we need a followup, like the one I posted
> today ?

No.  The patch is still bogus.

If the problem is that we are not coallescing messages in stream_recvmsg
we need a different fix.

Probably something like:

                  if (check_creds) {
                          /* Never glue messages from different writers */
                          if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
                              (UNIXCB(skb).cred != siocb->scm->cred))
                                  break;
-                 } else {
+                 } else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
                          /* Copy credentials */
                          scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
                          check_creds = 1;
                  }

Although comapring comparing the applicable uids and gids might be
sensible as well.

> Some user apps dont know about uid 65534.

What???  The problem is that the app wanted the uid and we gave it
garbage.  You can't fix wanting a valid uid by not passing a uid.

> 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,

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/