diff options
author | Huang Zhw <huang_zhw@126.com> | 2021-07-21 21:25:19 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-07-21 16:25:19 +0300 |
commit | 71d452876ebf8456afaadd6b3c27988abadd1148 (patch) | |
tree | 349fcae100a2892751b2ea939fdedae07981eeac | |
parent | 32e61ee295beb8a45b66c4488937f17d0cd0fca7 (diff) | |
download | redis-71d452876ebf8456afaadd6b3c27988abadd1148.tar.gz |
On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,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 https://github.com/redis/redis/pull/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.
-rw-r--r-- | .github/workflows/daily.yml | 6 | ||||
-rw-r--r-- | src/bitops.c | 32 | ||||
-rw-r--r-- | src/server.h | 2 | ||||
-rw-r--r-- | tests/unit/bitops.tcl | 26 |
4 files changed, 46 insertions, 20 deletions
diff --git a/.github/workflows/daily.yml b/.github/workflows/daily.yml index ceb82f5c1..800016508 100644 --- a/.github/workflows/daily.yml +++ b/.github/workflows/daily.yml @@ -254,7 +254,7 @@ jobs: sudo apt-get install tcl8.6 tclx valgrind -y - name: test if: true && !contains(github.event.inputs.skiptests, 'redis') - run: ./runtest --valgrind --verbose --clients 1 --dump-logs ${{github.event.inputs.test_args}} + run: ./runtest --valgrind --verbose --clients 1 --tags -large-memory --dump-logs ${{github.event.inputs.test_args}} - name: module api test if: true && !contains(github.event.inputs.skiptests, 'modules') run: ./runtest-moduleapi --valgrind --no-latency --verbose --clients 1 ${{github.event.inputs.test_args}} @@ -285,7 +285,7 @@ jobs: sudo apt-get install tcl8.6 tclx valgrind -y - name: test if: true && !contains(github.event.inputs.skiptests, 'redis') - run: ./runtest --valgrind --verbose --clients 1 --dump-logs ${{github.event.inputs.test_args}} + run: ./runtest --valgrind --verbose --clients 1 --tags -large-memory --dump-logs ${{github.event.inputs.test_args}} - name: module api test if: true && !contains(github.event.inputs.skiptests, 'modules') run: ./runtest-moduleapi --valgrind --no-latency --verbose --clients 1 ${{github.event.inputs.test_args}} @@ -420,7 +420,7 @@ jobs: prepare: pkg install -y bash gmake lang/tcl86 lang/tclx run: > gmake || exit 1 ; - if echo "${{github.event.inputs.skiptests}}" | grep -vq redis ; then ./runtest --accurate --verbose --no-latency --dump-logs ${{github.event.inputs.test_args}} || exit 1 ; fi ; + if echo "${{github.event.inputs.skiptests}}" | grep -vq redis ; then ./runtest --accurate --verbose --no-latency --tags -large-memory --dump-logs ${{github.event.inputs.test_args}} || exit 1 ; fi ; if echo "${{github.event.inputs.skiptests}}" | grep -vq modules ; then MAKE=gmake ./runtest-moduleapi --verbose ${{github.event.inputs.test_args}} || exit 1 ; fi ; if echo "${{github.event.inputs.skiptests}}" | grep -vq sentinel ; then ./runtest-sentinel ${{github.event.inputs.cluster_test_args}} || exit 1 ; fi ; if echo "${{github.event.inputs.skiptests}}" | grep -vq cluster ; then ./runtest-cluster ${{github.event.inputs.cluster_test_args}} || exit 1 ; fi ; diff --git a/src/bitops.c b/src/bitops.c index 3994b01c6..47da72142 100644 --- a/src/bitops.c +++ b/src/bitops.c @@ -37,8 +37,8 @@ /* Count number of bits set in the binary array pointed by 's' and long * 'count' bytes. The implementation of this function is required to * work with an input string length up to 512 MB or more (server.proto_max_bulk_len) */ -size_t redisPopcount(void *s, long count) { - size_t bits = 0; +long long redisPopcount(void *s, long count) { + long long bits = 0; unsigned char *p = s; uint32_t *p4; static const unsigned char bitsinbyte[256] = {0,1,1,2,1,2,2,3,1,2,2,3,2,3,3,4,1,2,2,3,2,3,3,4,2,3,3,4,3,4,4,5,1,2,2,3,2,3,3,4,2,3,3,4,3,4,4,5,2,3,3,4,3,4,4,5,3,4,4,5,4,5,5,6,1,2,2,3,2,3,3,4,2,3,3,4,3,4,4,5,2,3,3,4,3,4,4,5,3,4,4,5,4,5,5,6,2,3,3,4,3,4,4,5,3,4,4,5,4,5,5,6,3,4,4,5,4,5,5,6,4,5,5,6,5,6,6,7,1,2,2,3,2,3,3,4,2,3,3,4,3,4,4,5,2,3,3,4,3,4,4,5,3,4,4,5,4,5,5,6,2,3,3,4,3,4,4,5,3,4,4,5,4,5,5,6,3,4,4,5,4,5,5,6,4,5,5,6,5,6,6,7,2,3,3,4,3,4,4,5,3,4,4,5,4,5,5,6,3,4,4,5,4,5,5,6,4,5,5,6,5,6,6,7,3,4,4,5,4,5,5,6,4,5,5,6,5,6,6,7,4,5,5,6,5,6,6,7,5,6,6,7,6,7,7,8}; @@ -98,11 +98,11 @@ size_t redisPopcount(void *s, long count) { * no zero bit is found, it returns count*8 assuming the string is zero * padded on the right. However if 'bit' is 1 it is possible that there is * not a single set bit in the bitmap. In this special case -1 is returned. */ -long redisBitpos(void *s, unsigned long count, int bit) { +long long redisBitpos(void *s, unsigned long count, int bit) { unsigned long *l; unsigned char *c; unsigned long skipval, word = 0, one; - long pos = 0; /* Position of bit, to return to the caller. */ + long long pos = 0; /* Position of bit, to return to the caller. */ unsigned long j; int found; @@ -410,7 +410,7 @@ void printBits(unsigned char *p, unsigned long count) { * If the 'hash' argument is true, and 'bits is positive, then the command * will also parse bit offsets prefixed by "#". In such a case the offset * is multiplied by 'bits'. This is useful for the BITFIELD command. */ -int getBitOffsetFromArgument(client *c, robj *o, size_t *offset, int hash, int bits) { +int getBitOffsetFromArgument(client *c, robj *o, uint64_t *offset, int hash, int bits) { long long loffset; char *err = "bit offset is not an integer or out of range"; char *p = o->ptr; @@ -435,7 +435,7 @@ int getBitOffsetFromArgument(client *c, robj *o, size_t *offset, int hash, int b return C_ERR; } - *offset = (size_t)loffset; + *offset = loffset; return C_OK; } @@ -477,7 +477,7 @@ int getBitfieldTypeFromArgument(client *c, robj *o, int *sign, int *bits) { * so that the 'maxbit' bit can be addressed. The object is finally * returned. Otherwise if the key holds a wrong type NULL is returned and * an error is sent to the client. */ -robj *lookupStringForBitCommand(client *c, size_t maxbit) { +robj *lookupStringForBitCommand(client *c, uint64_t maxbit) { size_t byte = maxbit >> 3; robj *o = lookupKeyWrite(c->db,c->argv[1]); if (checkType(c,o,OBJ_STRING)) return NULL; @@ -527,7 +527,7 @@ unsigned char *getObjectReadOnlyString(robj *o, long *len, char *llbuf) { void setbitCommand(client *c) { robj *o; char *err = "bit is not an integer or out of range"; - size_t bitoffset; + uint64_t bitoffset; ssize_t byte, bit; int byteval, bitval; long on; @@ -566,7 +566,7 @@ void setbitCommand(client *c) { void getbitCommand(client *c) { robj *o; char llbuf[32]; - size_t bitoffset; + uint64_t bitoffset; size_t byte, bit; size_t bitval = 0; @@ -888,7 +888,7 @@ void bitposCommand(client *c) { addReplyLongLong(c, -1); } else { long bytes = end-start+1; - long pos = redisBitpos(p+start,bytes,bit); + long long pos = redisBitpos(p+start,bytes,bit); /* If we are looking for clear bits, and the user specified an exact * range with start-end, we can't consider the right of the range as @@ -897,11 +897,11 @@ void bitposCommand(client *c) { * So if redisBitpos() returns the first bit outside the range, * we return -1 to the caller, to mean, in the specified range there * is not a single "0" bit. */ - if (end_given && bit == 0 && pos == bytes*8) { + if (end_given && bit == 0 && pos == (long long)bytes<<3) { addReplyLongLong(c,-1); return; } - if (pos != -1) pos += start*8; /* Adjust for the bytes we skipped. */ + if (pos != -1) pos += (long long)start<<3; /* Adjust for the bytes we skipped. */ addReplyLongLong(c,pos); } } @@ -933,12 +933,12 @@ struct bitfieldOp { * GET subcommand is allowed, other subcommands will return an error. */ void bitfieldGeneric(client *c, int flags) { robj *o; - size_t bitoffset; + uint64_t bitoffset; int j, numops = 0, changes = 0; struct bitfieldOp *ops = NULL; /* Array of ops to execute at end. */ int owtype = BFOVERFLOW_WRAP; /* Overflow type. */ int readonly = 1; - size_t highest_write_offset = 0; + uint64_t highest_write_offset = 0; for (j = 2; j < c->argc; j++) { int remargs = c->argc-j-1; /* Remaining args other than current. */ @@ -1128,9 +1128,9 @@ void bitfieldGeneric(client *c, int flags) { * object boundaries. */ memset(buf,0,9); int i; - size_t byte = thisop->offset >> 3; + uint64_t byte = thisop->offset >> 3; for (i = 0; i < 9; i++) { - if (src == NULL || i+byte >= (size_t)strlen) break; + if (src == NULL || i+byte >= (uint64_t)strlen) break; buf[i] = src[i+byte]; } diff --git a/src/server.h b/src/server.h index e113f05fa..a7e2f49d2 100644 --- a/src/server.h +++ b/src/server.h @@ -1845,7 +1845,7 @@ void getRandomHexChars(char *p, size_t len); void getRandomBytes(unsigned char *p, size_t len); uint64_t crc64(uint64_t crc, const unsigned char *s, uint64_t l); void exitFromChild(int retcode); -size_t redisPopcount(void *s, long count); +long long redisPopcount(void *s, long count); int redisSetProcTitle(char *title); int validateProcTitleTemplate(const char *template); int redisCommunicateSystemd(const char *sd_notify_msg); diff --git a/tests/unit/bitops.tcl b/tests/unit/bitops.tcl index 7e2280575..bd6b8b635 100644 --- a/tests/unit/bitops.tcl +++ b/tests/unit/bitops.tcl @@ -348,4 +348,30 @@ start_server {tags {"bitops"}} { } } } + + test "BIT pos larger than UINT_MAX" { + set bytes [expr (1 << 29) + 1] + set bitpos [expr (1 << 32)] + set oldval [lindex [r config get proto-max-bulk-len] 1] + r config set proto-max-bulk-len $bytes + r setbit mykey $bitpos 1 + assert_equal $bytes [r strlen mykey] + assert_equal 1 [r getbit mykey $bitpos] + assert_equal [list 128 128 -1] [r bitfield mykey get u8 $bitpos set u8 $bitpos 255 get i8 $bitpos] + assert_equal $bitpos [r bitpos mykey 1] + assert_equal $bitpos [r bitpos mykey 1 [expr $bytes - 1]] + if {$::accurate} { + # set all bits to 1 + set mega [expr (1 << 23)] + set part [string repeat "\xFF" $mega] + for {set i 0} {$i < 64} {incr i} { + r setrange mykey [expr $i * $mega] $part + } + r setrange mykey [expr $bytes - 1] "\xFF" + assert_equal [expr $bitpos + 8] [r bitcount mykey] + assert_equal -1 [r bitpos mykey 0 0 [expr $bytes - 1]] + } + r config set proto-max-bulk-len $oldval + r del mykey + } {1} {large-memory} } |