Bug 12568 - Integer overflow still exists in xattrs.c, leading to buffer overflow
Summary: Integer overflow still exists in xattrs.c, leading to buffer overflow
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.1.2
Hardware: All All
: P5 critical (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
Depends on:
Reported: 2017-02-07 08:45 UTC by shqking
Modified: 2017-10-08 15:38 UTC (History)
0 users

See Also:


Note You need to log in before you can comment on or make changes to this bug.
Description shqking 2017-02-07 08:45:14 UTC
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".

Comment 1 Wayne Davison 2017-10-08 15:38:17 UTC
I've committed a fix for this into git. Many thanks for pointing this out. Sorry for how slow I've been lately.