[tipc] dest_name_check() is racy (potential security hole)

[tipc] dest_name_check() is racy (potential security hole)

[Al Viro]

	dest_name_check() is called by tipc sendmsg(2).  What it does ends
with
        if (!m->msg_iovlen || (m->msg_iov[0].iov_len < sizeof(hdr)))
                return -EMSGSIZE;
        if (copy_from_user(&hdr, m->msg_iov[0].iov_base, sizeof(hdr)))
                return -EFAULT;
        if ((ntohs(hdr.tcm_type) & 0xC000) && (!capable(CAP_NET_ADMIN)))
                return -EACCES;

        return 0;
IOW, it checks that iovec we'd been given is large enough to contain
struct tipc_cfg_msg_hdr and that non-priveleged sender doesn't have
->tcm_type in that header with bits 14 or 15 set.

According to the comment in front of it, it "prevents restricted configuration
commands from being issued by unauthorized users".  Makes sense, right?
Except that it obviously doesn't provide any security whatsoever, because
the value we'd read from the iovec is immediately discarded and later
we reread it again.

There is nothing to stop the caller from spawning a threar that would flip
the bits in question back and forth, while the parent keeps calling sendmsg().
Sooner or later we will have dest_name_check() pick the harmless value,
with subsequent memcpy_fromiovecend() picking the modified one.

AFAICS, that part of dest_name_check() must be delayed until tipc_msg_build(),
when we read that header for real.  Brute-force way to do that would be to
pass a flag to tipc_msg_build() ("do we want to check tcm_type?") and have it
set on call chains coming from tipc_send2name() and tipc_multicast().

Again, in the current form the check doesn't do much good; I've no idea
how much nastiness can be achieved by fooling it, but it *can* be fooled.

Comments?

Re: [tipc] dest_name_check() is racy (potential security hole)

[Erik Hugne]

On Sun, Apr 06, 2014 at 05:39:34AM +0100, Al Viro wrote:
> According to the comment in front of it, it "prevents restricted configuration
> commands from being issued by unauthorized users".  Makes sense, right?
> Except that it obviously doesn't provide any security whatsoever, because
> the value we'd read from the iovec is immediately discarded and later
> we reread it again.
> 
> There is nothing to stop the caller from spawning a threar that would flip
> the bits in question back and forth, while the parent keeps calling sendmsg().
> Sooner or later we will have dest_name_check() pick the harmless value,
> with subsequent memcpy_fromiovecend() picking the modified one.

Ack, but as of commit 5902385a2440a55f005b266c93e0bb9398e5a62b ("tipc: obsolete the
remote management feature) this is no longer an issue. We missed to remove the
TIPC_CFG_SRV/CAP_NET_ADMIN check in that commit aswell.

//W

Re: [tipc] dest_name_check() is racy (potential security hole)

[Jon Maloy]

On 04/06/2014 06:39 AM, Al Viro wrote:
> 	dest_name_check() is called by tipc sendmsg(2).  What it does ends
> with
>         if (!m->msg_iovlen || (m->msg_iov[0].iov_len < sizeof(hdr)))
>                 return -EMSGSIZE;
>         if (copy_from_user(&hdr, m->msg_iov[0].iov_base, sizeof(hdr)))
>                 return -EFAULT;
>         if ((ntohs(hdr.tcm_type) & 0xC000) && (!capable(CAP_NET_ADMIN)))
>                 return -EACCES;
> 
>         return 0;
> IOW, it checks that iovec we'd been given is large enough to contain
> struct tipc_cfg_msg_hdr and that non-priveleged sender doesn't have
> ->tcm_type in that header with bits 14 or 15 set.
> 
> According to the comment in front of it, it "prevents restricted configuration
> commands from being issued by unauthorized users".  Makes sense, right?
> Except that it obviously doesn't provide any security whatsoever, because
> the value we'd read from the iovec is immediately discarded and later
> we reread it again.
> 
> There is nothing to stop the caller from spawning a threar that would flip
> the bits in question back and forth, while the parent keeps calling sendmsg().
> Sooner or later we will have dest_name_check() pick the harmless value,
> with subsequent memcpy_fromiovecend() picking the modified one.

It seems you are right here. I haven't been aware of this kind of attacks.

> 
> AFAICS, that part of dest_name_check() must be delayed until tipc_msg_build(),
> when we read that header for real.  Brute-force way to do that would be to
> pass a flag to tipc_msg_build() ("do we want to check tcm_type?") and have it
> set on call chains coming from tipc_send2name() and tipc_multicast().
> 
> Again, in the current form the check doesn't do much good; I've no idea
> how much nastiness can be achieved by fooling it, but it *can* be fooled.

Not that much, I think, except that the sender can send messages to non-existing
addresses, whereafter they will be dropped.

These addresses are primarily *reserved*, so that only TIPC itself can bind to them.
Only two such addresses are used at the moment. One of the two also had *restricted*
access, in the sense that only privileged users could send messages to it.
The only server binding to that address was the remote configuration server, but we
just removed that one, exactly for security reasons. 
So an option may be to just remove this check altogether at sendto(), but enforce it
more robustly at bind(), maybe the way you suggest.

We will of course fix this, one way or another.

Regards
///jon

> 
> Comments?
>