[Bug 12568] New: Integer overflow still affects xattrs.c

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 12568] New: Integer overflow still affects xattrs.c

samba-bugs
https://bugzilla.samba.org/show_bug.cgi?id=12568

            Bug ID: 12568
           Summary: Integer overflow still affects xattrs.c
           Product: rsync
           Version: 3.1.2
          Hardware: All
                OS: All
            Status: NEW
          Severity: critical
          Priority: P5
         Component: core
          Assignee: [hidden email]
          Reporter: [hidden email]
        QA Contact: [hidden email]

A suspicious integer overflow is found in xattrs.c:692.
The code snippet is as follows.

684 for (num = 1; num <= count; num++) {
685    char *ptr, *name;
686    rsync_xa *rxa;
687    size_t name_len = read_varint(f);
688    size_t datum_len = read_varint(f);
689    size_t dget_len = datum_len > MAX_FULL_DATUM ? 1 + MAX_DIGEST_LEN :
datum_len;
690    size_t extra_len = MIGHT_NEED_RPRE ? RPRE_LEN : 0;
691    if ((dget_len + extra_len < dget_len)
692     || (dget_len + extra_len + name_len < dget_len))
693        overflow_exit("receive_xattr");
694    ptr = new_array(char, dget_len + extra_len + name_len);
695    if (!ptr)
696        out_of_memory("receive_xattr");
697    name = ptr + dget_len + extra_len;
698    read_buf(f, name, name_len);

From the code we can see that the security checks at line 691 and line 692 aim
to filter integer overflows. Specifically, a handler, i.e. function
"overflow_exit" will be invoked if the first addition "dget_len + extra_len"
overflows (protected by the check at line 691) or the second addition "dget_len
+ extra_len + name_len" overflows (protected by the check at line 692).

Here, we want to say that the later check at line 692 is insufficient to catch
integer overflow. That means, there exist some integer overflows, which can
bypass the later check.

Assume that it's on a 32-bit machine, and "dget_len" is 100, "extra_len" is
also 100, whereas "name_len" takes a very big integer value, e.g., 0xffff ffff.
Hence, "dget_len + extra_len + name_len" overflows to 199, which is bigger than
"dget_len", i.e. 100. As a result, an integer overflow indeed happens here,
however, the overflow check at line 692 doesn't catch it. Furthermore, buffer
overflow would occur at line 698.

One possible workaround is to use a much stricter overflow check at line 692,
as below:
    "dget_len + extra_len + name_len < dget_len + extra_len".

Thanks.

--
You are receiving this mail because:
You are the QA Contact for the bug.

--
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html
Reply | Threaded
Open this post in threaded view
|

[Bug 12568] Integer overflow still exists in xattrs.c, leading to buffer overflow

Samba - rsync mailing list
https://bugzilla.samba.org/show_bug.cgi?id=12568

Wayne Davison <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #1 from Wayne Davison <[hidden email]> ---
I've committed a fix for this into git. Many thanks for pointing this out.
Sorry for how slow I've been lately.

--
You are receiving this mail because:
You are the QA Contact for the bug.

--
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html
Reply | Threaded
Open this post in threaded view
|

Re: [Bug 12568] Integer overflow still exists in xattrs.c, leading to buffer overflow

Samba - rsync mailing list
On Sun 08 Oct 2017, just subscribed for rsync-qa from bugzilla via rsync wrote:
>
> --- Comment #1 from Wayne Davison <[hidden email]> ---
> I've committed a fix for this into git. Many thanks for pointing this out.
> Sorry for how slow I've been lately.

Hey, I'm just happy you're still around :)


Paul

--
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html