netdev - Re: [Security] TIPC security issues

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]

Message-ID: <AANLkTi=jDDDk15KDhsC=qTnmgU5Urhc9LTrROkFOsY_Q@mail.gmail.com>
Date:	Wed, 27 Oct 2010 14:27:13 -0400
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	David Miller <davem@...emloft.net>
Cc:	torvalds@...ux-foundation.org, drosenberg@...curity.com,
	jon.maloy@...csson.com, allan.stephens@...driver.com,
	netdev@...r.kernel.org, security@...nel.org
Subject: Re: [Security] TIPC security issues

On Wed, Oct 27, 2010 at 1:50 PM, David Miller <davem@...emloft.net> 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;
> +

I've got some patches from Al that I'm pre-reviewing/testing
before spamming everyone with them (will send within 24h, if
that is OK).   Anyway, in the above, does it make sense to
check for the overflow incrementally, i.e. something like this?

-       for (i = 0; i < num_sect; i++)
-               dsz += msg_sect[i].iov_len;
+       for (i = 0; i < num_sect; i++) {
+               len = msg_sect[i].iov_len;
+               if (len > LONG_MAX - dsz)
+                       return LONG_MAX;
+               dsz += len;
+       }

(where len and dsz are both size_t).

Thanks,
Paul.

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