[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1288203979.1836.2.camel@dan>
Date: Wed, 27 Oct 2010 14:26:19 -0400
From: Dan Rosenberg <drosenberg@...curity.com>
To: David Miller <davem@...emloft.net>
Cc: torvalds@...ux-foundation.org, jon.maloy@...csson.com,
allan.stephens@...driver.com, netdev@...r.kernel.org,
security@...nel.org
Subject: Re: [Security] TIPC security issues
The proposed fix is a start, but it's not sufficient to completely fix
the problem. What if the total of the iovecs wraps around back to 0?
The total size will be returned as a small number, but large amounts of
data will be copied into the allocated buffer since the individual
iovecs can have arbitrary sizes.
-Dan
On Wed, 2010-10-27 at 10:50 -0700, David Miller wrote:
> From: Linus Torvalds <torvalds@...ux-foundation.org>
> Date: Wed, 27 Oct 2010 10:37:46 -0700
>
> > If you _really_ care deeply, then some packet-oriented protocol can
> > just have its own private packet size limit (which would be way less
> > than 2GB), and then just look at the total size and say "oh, the total
> > size is bigger than my limit, so I'll just error out". Then, the fact
> > that verify_iovec() may have truncated the message to 2GB-1 doesn't
> > matter at all.
> >
> > (Practically speaking, I bet all packet-oriented protocols already
> > have a limit that is enforced by simply allocation patterns, so I
> > don't think it's actually a problem even now)
>
> This is, as it turns out, effectively what the TIPC socket layer
> already does.
>
> Most of the send calls that propagate down to this code adding up the
> iov_len lengths gets passed a maximum packet size.
>
> Anyways, here is what I came up with to kill this specific bug in
> TIPC:
>
> From 39dc17049a5ed989bab8997945a048ffddf48387 Mon Sep 17 00:00:00 2001
> From: David S. Miller <davem@...emloft.net>
> Date: Wed, 27 Oct 2010 10:46:59 -0700
> Subject: [PATCH] tipc: Fix iov_len handling in message send path.
>
> Use size_t to add together iov_len's from the iovec, error
> out with -EMSGSIZE if total is greater than INT_MAX.
>
> Reported-by: Dan Rosenberg <drosenberg@...curity.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> ---
> net/tipc/msg.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index ecb532f..b29eb8b 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
>
> int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
> {
> - int dsz = 0;
> + size_t dsz = 0;
> int i;
>
> for (i = 0; i < num_sect; i++)
> dsz += msg_sect[i].iov_len;
> +
> + if (dsz > INT_MAX)
> + return -EMSGSIZE;
> +
> return dsz;
> }
>
> @@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
> */
>
> int tipc_msg_build(struct tipc_msg *hdr,
> - struct iovec const *msg_sect, u32 num_sect,
> - int max_size, int usrmem, struct sk_buff** buf)
> + struct iovec const *msg_sect, u32 num_sect,
> + int max_size, int usrmem, struct sk_buff** buf)
> {
> int dsz, sz, hsz, pos, res, cnt;
>
> dsz = tipc_msg_calc_data_size(msg_sect, num_sect);
> + if (dsz < 0)
> + return dsz;
> +
> if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) {
> *buf = NULL;
> return -EINVAL;
> --
> 1.7.3.2
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists