From: Al Viro <viro@ZenIV.linux.org.uk>
To: netdev@vger.kernel.org
Cc: Jon Maloy <jon.maloy@ericsson.com>
Subject: [tipc] dest_name_check() is racy (potential security hole)
Date: Sun, 6 Apr 2014 05:39:34 +0100 [thread overview]
Message-ID: <20140406043934.GW18016@ZenIV.linux.org.uk> (raw)
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?
next reply other threads:[~2014-04-06 4:39 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-06 4:39 Al Viro [this message]
2014-04-07 8:08 ` [tipc] dest_name_check() is racy (potential security hole) Erik Hugne
2014-04-07 8:57 ` Jon Maloy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140406043934.GW18016@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=jon.maloy@ericsson.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).