On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS may overflow by huangzhw · Pull Request #9191 · redis/redis

huangzhw · 2021-07-04T03:02:40Z

GETBIT, SETBIT may access wrong address because of wrap.
BITCOUNT and BITPOS may return wrapped results.
BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761).

This commit uses uint64_t or long long instead of size_t.
related #8096

At 32bit platform:

setbit bit 4294967295 1
(integer) 0
config set proto-max-bulk-len 536870913
OK
append bit "\xFF"
(integer) 536870913
getbit bit 4294967296
(integer) 0

When the bit index is larger than 4294967295, size_t can't hold bit index. In the past, proto-max-bulk-len is limit to 536870912, so there is no problem.

After this commit, bit position is stored in uint64_t or long long. So when proto-max-bulk-len > 536870912, 32bit platforms can still be correct.

For 64bit platform, this problem still exists. The major reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow.
But at 64bit platform, we don't have so long string. So this bug may never happen.

Additionally this commit add a test cost 512MB memory which is tag as large-memory. Make freebsd ci and valgrind ci ignore this test.

size_t. > setbit bit 4294967295 1 (integer) 0 > config set proto-max-bulk-len 536870913 OK > append bit "\xFF" (integer) 536870913 > getbit bit 4294967296 (integer) 0 When the bit index is larger than 4294967295, size_t can't hold bit index.

oranagra · 2021-07-04T05:06:38Z

i'm not certain it it would be a realistic case to store such large strings on 64 bit platforms (today and in the foreseeable future)
i.e. it's over 2,000,000,000 GB.
For 32 bit, indeed a string larger than 512 MB would be problematic, and that seems realistic, so i think we can take this patch.

huangzhw · 2021-07-04T05:17:19Z

Agree. So ignore it.

tests/unit/type/string.tcl

- Add more test - fix bitfield overflow

tests/unit/bitops.tcl

This reverts commit 3d6a32d.

This reverts commit cee348d.

huangzhw · 2021-07-08T03:34:34Z

@oranagra I trigger a ci https://github.com/huangzhw/redis/actions/runs/1010003276

oranagra · 2021-07-08T06:12:56Z

@huangzhw the errors we see where in unstable when you branched from it, resolved by #9192
what was important to see in the CI you triggered is that the test was indeed skipped in valgrind and not skipped in the normal run, and that seems good.

src/bitops.c

sundb · 2021-07-09T10:20:46Z

Crash in my pc under 32bit.

config set proto-max-bulk-len 2147483647
setbit key 17179869175 1

huangzhw · 2021-07-09T10:41:10Z

setbit key 17179869175 1

What's the trace. In my machine it's out of memory.

sundb · 2021-07-09T10:55:58Z

@huangzhw It is out of memory that causes crashes.

huangzhw · 2021-07-09T11:03:07Z

So it's not about this bug.

oranagra · 2021-07-11T06:11:19Z

@sundb so the reason it crashes on your PC is that you didn't have enough (512mb) free memory?
for this reason we added the --tags -large-memory option, but maybe we wanna re-consider that.

we can try to automatically detect that there's insufficient memory, by calling free, but that heuristics can get broken.

the other alternative is to detect that specific crash and ignore it.
i.e. add a catch on the line that creates the allocation, and possibly use count_log_message to see if the server panic d on insufficient memory, then print something to stdout and skip the rest of the test.

considering @sundb can reproduce it, maybe you wanna do that part?

huangzhw · 2021-07-11T07:03:40Z

It's 2GB not 512MB.

oranagra · 2021-07-11T07:14:01Z

forgive me for not being in focus, but why / where do we consume 2gb?
don't we have the overflow issue in 2^32 / 8?

huangzhw · 2021-07-11T07:15:27Z

I mean @sundb's test cost 2GB. It is setbit key 17179869175 1.

oranagra · 2021-07-11T07:27:32Z

ohh, so not related to the test in this PR..
i.e. he thought he found another issue, but was just insufficient memory (actually not because insufficient host memory, but rather just a limit of allocations on 32 bit builds)

oranagra · 2021-07-11T07:45:03Z

@huangzhw please make sure the title and top comment reflect the end result.
i.e. which commands are affected, what was the outcome, and what was resolved.

huangzhw · 2021-07-11T08:31:08Z

I edited it.

…NT,BITPOS may overflow (see CVE-2021-32761) (#9191) GETBIT, SETBIT may access wrong address because of wrap. BITCOUNT and BITPOS may return wrapped results. BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761). This commit uses `uint64_t` or `long long` instead of `size_t`. related #8096 At 32bit platform: > setbit bit 4294967295 1 (integer) 0 > config set proto-max-bulk-len 536870913 OK > append bit "\xFF" (integer) 536870913 > getbit bit 4294967296 (integer) 0 When the bit index is larger than 4294967295, size_t can't hold bit index. In the past, `proto-max-bulk-len` is limit to 536870912, so there is no problem. After this commit, bit position is stored in `uint64_t` or `long long`. So when `proto-max-bulk-len > 536870912`, 32bit platforms can still be correct. For 64bit platform, this problem still exists. The major reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow. But at 64bit platform, we don't have so long string. So this bug may never happen. Additionally this commit add a test cost `512MB` memory which is tag as `large-memory`. Make freebsd ci and valgrind ci ignore this test. * This test is disabled in this version since bitops doesn't rely on proto-max-bulk-len. some of the overflows can still occur so we do want the fixes. (cherry picked from commit 71d4528)

…NT,BITPOS may overflow (see CVE-2021-32761) (#9191) GETBIT, SETBIT may access wrong address because of wrap. BITCOUNT and BITPOS may return wrapped results. BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761). This commit uses `uint64_t` or `long long` instead of `size_t`. related #8096 At 32bit platform: > setbit bit 4294967295 1 (integer) 0 > config set proto-max-bulk-len 536870913 OK > append bit "\xFF" (integer) 536870913 > getbit bit 4294967296 (integer) 0 When the bit index is larger than 4294967295, size_t can't hold bit index. In the past, `proto-max-bulk-len` is limit to 536870912, so there is no problem. After this commit, bit position is stored in `uint64_t` or `long long`. So when `proto-max-bulk-len > 536870912`, 32bit platforms can still be correct. For 64bit platform, this problem still exists. The major reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow. But at 64bit platform, we don't have so long string. So this bug may never happen. Additionally this commit add a test cost `512MB` memory which is tag as `large-memory`. Make freebsd ci and valgrind ci ignore this test. * This test is disabled in this version since bitops doesn't rely on proto-max-bulk-len. some of the overflows can still occur so we do want the fixes. (cherry picked from commit 71d4528) (cherry picked from commit 2be8f1d)

…NT,BITPOS may overflow (see CVE-2021-32761) (#9191) GETBIT, SETBIT may access wrong address because of wrap. BITCOUNT and BITPOS may return wrapped results. BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761). This commit uses `uint64_t` or `long long` instead of `size_t`. related #8096 At 32bit platform: > setbit bit 4294967295 1 (integer) 0 > config set proto-max-bulk-len 536870913 OK > append bit "\xFF" (integer) 536870913 > getbit bit 4294967296 (integer) 0 When the bit index is larger than 4294967295, size_t can't hold bit index. In the past, `proto-max-bulk-len` is limit to 536870912, so there is no problem. After this commit, bit position is stored in `uint64_t` or `long long`. So when `proto-max-bulk-len > 536870912`, 32bit platforms can still be correct. For 64bit platform, this problem still exists. The major reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow. But at 64bit platform, we don't have so long string. So this bug may never happen. Additionally this commit add a test cost `512MB` memory which is tag as `large-memory`. Make freebsd ci and valgrind ci ignore this test. (cherry picked from commit 71d4528)

steward007 · 2021-07-22T05:25:14Z

Does CVE-2021-32761 only affect windows platform？

huangzhw · 2021-07-22T05:27:03Z

Official Redis doesn't have windows platform. It affect all platforms.

oranagra · 2021-07-22T05:28:56Z

It affects only 32 bit builds (which isn't the default, but note that these builds can also be run on 64 bit platforms. i.e. Linux / Unix).
Anyway, this project (redis), doesn't support windows. so depending on which redis fork for windows you're using, you may wanna ask there.

oranagra · 2021-07-22T05:31:38Z

or maybe i didn't understand your question, and you don't care about windows.
in which case, this issue affects redis that's either built using make 32bit or built on a 32-bit OS / toolchain (i.e. including Linux in theory, but that's not very common today).

steward007 · 2021-07-22T05:51:43Z

ok. i know it.thanks a lot. 32-bit redis is not very common today.

…NT,BITPOS may overflow (see CVE-2021-32761) (redis#9191) GETBIT, SETBIT may access wrong address because of wrap. BITCOUNT and BITPOS may return wrapped results. BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761). This commit uses `uint64_t` or `long long` instead of `size_t`. related redis#8096 At 32bit platform: > setbit bit 4294967295 1 (integer) 0 > config set proto-max-bulk-len 536870913 OK > append bit "\xFF" (integer) 536870913 > getbit bit 4294967296 (integer) 0 When the bit index is larger than 4294967295, size_t can't hold bit index. In the past, `proto-max-bulk-len` is limit to 536870912, so there is no problem. After this commit, bit position is stored in `uint64_t` or `long long`. So when `proto-max-bulk-len > 536870912`, 32bit platforms can still be correct. For 64bit platform, this problem still exists. The major reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow. But at 64bit platform, we don't have so long string. So this bug may never happen. Additionally this commit add a test cost `512MB` memory which is tag as `large-memory`. Make freebsd ci and valgrind ci ignore this test.

oranagra added state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten release-notes indication that this issue needs to be mentioned in the release notes labels Jul 4, 2021

oranagra changed the title ~~At 32 platform, bit pos may overflow, use type uint64_t instead of size_t~~ On 32 bit platform, bit position of GETBIT, SETBIT and BITFIELD may overflow. Jul 5, 2021

oranagra reviewed Jul 5, 2021

View reviewed changes

tests/unit/type/string.tcl Outdated Show resolved Hide resolved

huangzhw added 2 commits July 5, 2021 19:25

fix lookupStringForBitCommand param for corrupt

8fb77b7

fix bitpos and bitcount return value maybe larger then 32bit

92910f2

huangzhw marked this pull request as draft July 5, 2021 11:36

oranagra changed the title ~~On 32 bit platform, bit position of GETBIT, SETBIT and BITFIELD may overflow.~~ On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS may overflow Jul 5, 2021

- Add --tags -large-memory in freebsd and valgrind CI

c9f260a

- Add more test - fix bitfield overflow

oranagra reviewed Jul 7, 2021

View reviewed changes

tests/unit/bitops.tcl Outdated Show resolved Hide resolved

huangzhw added 4 commits July 8, 2021 08:57

trigger ci

cee348d

trigger ci

3d6a32d

Revert "trigger ci"

65c9cec

This reverts commit 3d6a32d.

Revert "trigger ci"

bcd992c

This reverts commit cee348d.

huangzhw marked this pull request as ready for review July 8, 2021 03:34

oranagra reviewed Jul 8, 2021

View reviewed changes

src/bitops.c Show resolved Hide resolved

huangzhw added 2 commits July 8, 2021 18:58

fix bug and add test

77242b8

add more tests

a47dbb8

oranagra approved these changes Jul 11, 2021

View reviewed changes

oranagra merged commit 71d4528 into redis:unstable Jul 21, 2021

huangzhw deleted the bit_32_overflow branch July 21, 2021 13:32

This was referenced Jul 21, 2021

Release 6.2.5 #9264

Merged

Release 6.0.15 #9266

Merged

Release 5.0.13 #9267

Merged

Conversation

huangzhw commented Jul 4, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jul 4, 2021

Uh oh!

huangzhw commented Jul 4, 2021

Uh oh!

Uh oh!

Uh oh!

huangzhw commented Jul 8, 2021

Uh oh!

oranagra commented Jul 8, 2021

Uh oh!

Uh oh!

sundb commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huangzhw commented Jul 9, 2021

Uh oh!

sundb commented Jul 9, 2021

Uh oh!

huangzhw commented Jul 9, 2021

Uh oh!

oranagra commented Jul 11, 2021

Uh oh!

huangzhw commented Jul 11, 2021

Uh oh!

oranagra commented Jul 11, 2021

Uh oh!

huangzhw commented Jul 11, 2021

Uh oh!

oranagra commented Jul 11, 2021

Uh oh!

oranagra commented Jul 11, 2021

Uh oh!

huangzhw commented Jul 11, 2021

Uh oh!

steward007 commented Jul 22, 2021

Uh oh!

huangzhw commented Jul 22, 2021

Uh oh!

oranagra commented Jul 22, 2021

Uh oh!

oranagra commented Jul 22, 2021

Uh oh!

steward007 commented Jul 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

huangzhw commented Jul 4, 2021 •

edited by oranagra

Loading

sundb commented Jul 9, 2021 •

edited

Loading