Commit 3716950c authored by Oran Agra's avatar Oran Agra
Browse files

Sanitize dump payload: validate no duplicate records in hash/zset/intset

If RESTORE passes successfully with full sanitization, we can't affort
to crash later on assertion due to duplicate records in a hash when
converting it form ziplist to dict.
This means that when doing full sanitization, we must make sure there
are no duplicate records in any of the collections.
parent 5b446313
......@@ -284,8 +284,11 @@ size_t intsetBlobLen(intset *is) {
return sizeof(intset)+intrev32ifbe(is->length)*intrev32ifbe(is->encoding);
}
int intsetValidateIntegrity(const unsigned char *p, size_t size) {
const intset *is = (const intset *)p;
/* Validate the integrity of the data stracture.
* when `deep` is 0, only the integrity of the header is validated.
* when `deep` is 1, we make sure there are no duplicate or out of order records. */
int intsetValidateIntegrity(const unsigned char *p, size_t size, int deep) {
intset *is = (intset *)p;
/* check that we can actually read the header. */
if (size < sizeof(*is))
return 0;
......@@ -308,6 +311,22 @@ int intsetValidateIntegrity(const unsigned char *p, size_t size) {
if (sizeof(*is) + count*record_size != size)
return 0;
/* check that the set is not empty. */
if (count==0)
return 0;
if (!deep)
return 1;
/* check that there are no dup or out of order records. */
int64_t prev = _intsetGet(is,0);
for (uint32_t i=1; i<count; i++) {
int64_t cur = _intsetGet(is,i);
if (cur <= prev)
return 0;
prev = cur;
}
return 1;
}
......
......@@ -46,7 +46,7 @@ int64_t intsetRandom(intset *is);
uint8_t intsetGet(intset *is, uint32_t pos, int64_t *value);
uint32_t intsetLen(const intset *is);
size_t intsetBlobLen(intset *is);
int intsetValidateIntegrity(const unsigned char *is, size_t size);
int intsetValidateIntegrity(const unsigned char *is, size_t size, int deep);
#ifdef REDIS_TEST
int intsetTest(int argc, char *argv[]);
......
This diff is collapsed.
......@@ -2071,6 +2071,7 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore);
long zsetRank(robj *zobj, sds ele, int reverse);
int zsetDel(robj *zobj, sds ele);
robj *zsetDup(robj *o);
int zsetZiplistValidateIntegrity(unsigned char *zl, size_t size, int deep);
void genericZpopCommand(client *c, robj **keyv, int keyc, int where, int emitkey, robj *countarg);
sds ziplistGetObject(unsigned char *sptr);
int zslValueGteMin(double value, zrangespec *spec);
......@@ -2177,6 +2178,7 @@ robj *hashTypeLookupWriteOrCreate(client *c, robj *key);
robj *hashTypeGetValueObject(robj *o, sds field);
int hashTypeSet(robj *o, sds field, sds value, int flags);
robj *hashTypeDup(robj *o);
int hashZiplistValidateIntegrity(unsigned char *zl, size_t size, int deep);
/* Pub / Sub */
int pubsubUnsubscribeAllChannels(client *c, int notify);
......
......@@ -549,6 +549,55 @@ robj *hashTypeDup(robj *o) {
return hobj;
}
/* callback for to check the ziplist doesn't have duplicate recoreds */
static int _hashZiplistEntryValidation(unsigned char *p, void *userdata) {
struct {
long count;
dict *fields;
} *data = userdata;
/* Odd records are field names, add to dict and check that's not a dup */
if (((data->count) & 1) == 0) {
unsigned char *str;
unsigned int slen;
long long vll;
if (!ziplistGet(p, &str, &slen, &vll))
return 0;
sds field = str? sdsnewlen(str, slen): sdsfromlonglong(vll);;
if (dictAdd(data->fields, field, NULL) != DICT_OK) {
/* Duplicate, return an error */
sdsfree(field);
return 0;
}
}
(data->count)++;
return 1;
}
/* Validate the integrity of the data stracture.
* when `deep` is 0, only the integrity of the header is validated.
* when `deep` is 1, we scan all the entries one by one. */
int hashZiplistValidateIntegrity(unsigned char *zl, size_t size, int deep) {
if (!deep)
return ziplistValidateIntegrity(zl, size, 0, NULL, NULL);
/* Keep track of the field names to locate duplicate ones */
struct {
long count;
dict *fields;
} data = {0, dictCreate(&hashDictType, NULL)};
int ret = ziplistValidateIntegrity(zl, size, 1, _hashZiplistEntryValidation, &data);
/* make sure we have an even number of records. */
if (data.count & 1)
ret = 0;
dictRelease(data.fields);
return ret;
}
/*-----------------------------------------------------------------------------
* Hash type commands
*----------------------------------------------------------------------------*/
......
......@@ -1604,6 +1604,55 @@ robj *zsetDup(robj *o) {
return zobj;
}
/* callback for to check the ziplist doesn't have duplicate recoreds */
static int _zsetZiplistValidateIntegrity(unsigned char *p, void *userdata) {
struct {
long count;
dict *fields;
} *data = userdata;
/* Even records are field names, add to dict and check that's not a dup */
if (((data->count) & 1) == 1) {
unsigned char *str;
unsigned int slen;
long long vll;
if (!ziplistGet(p, &str, &slen, &vll))
return 0;
sds field = str? sdsnewlen(str, slen): sdsfromlonglong(vll);;
if (dictAdd(data->fields, field, NULL) != DICT_OK) {
/* Duplicate, return an error */
sdsfree(field);
return 0;
}
}
(data->count)++;
return 1;
}
/* Validate the integrity of the data stracture.
* when `deep` is 0, only the integrity of the header is validated.
* when `deep` is 1, we scan all the entries one by one. */
int zsetZiplistValidateIntegrity(unsigned char *zl, size_t size, int deep) {
if (!deep)
return ziplistValidateIntegrity(zl, size, 0, NULL, NULL);
/* Keep track of the field names to locate duplicate ones */
struct {
long count;
dict *fields;
} data = {0, dictCreate(&hashDictType, NULL)};
int ret = ziplistValidateIntegrity(zl, size, 1, _zsetZiplistValidateIntegrity, &data);
/* make sure we have an even number of records. */
if (data.count & 1)
ret = 0;
dictRelease(data.fields);
return ret;
}
/*-----------------------------------------------------------------------------
* Sorted set commands
*----------------------------------------------------------------------------*/
......
......@@ -1406,7 +1406,8 @@ void ziplistRepr(unsigned char *zl) {
/* Validate the integrity of the data stracture.
* when `deep` is 0, only the integrity of the header is validated.
* when `deep` is 1, we scan all the entries one by one. */
int ziplistValidateIntegrity(unsigned char *zl, size_t size, int deep) {
int ziplistValidateIntegrity(unsigned char *zl, size_t size, int deep,
ziplistValidateEntryCB entry_cb, void *cb_userdata) {
/* check that we can actually read the header. (and ZIP_END) */
if (size < ZIPLIST_HEADER_SIZE + ZIPLIST_END_SIZE)
return 0;
......@@ -1441,6 +1442,10 @@ int ziplistValidateIntegrity(unsigned char *zl, size_t size, int deep) {
if (e.prevrawlen != prev_raw_size)
return 0;
/* Optionally let the caller validate the entry too. */
if (entry_cb && !entry_cb(p, cb_userdata))
return 0;
/* Move to the next entry */
prev_raw_size = e.headersize + e.len;
prev = p;
......
......@@ -49,7 +49,9 @@ unsigned char *ziplistFind(unsigned char *zl, unsigned char *p, unsigned char *v
unsigned int ziplistLen(unsigned char *zl);
size_t ziplistBlobLen(unsigned char *zl);
void ziplistRepr(unsigned char *zl);
int ziplistValidateIntegrity(unsigned char *zl, size_t size, int deep);
typedef int (*ziplistValidateEntryCB)(unsigned char* p, void* userdata);
int ziplistValidateIntegrity(unsigned char *zl, size_t size, int deep,
ziplistValidateEntryCB entry_cb, void *cb_userdata);
#ifdef REDIS_TEST
int ziplistTest(int argc, char *argv[]);
......
......@@ -17,7 +17,7 @@ test {corrupt payload: #7445 - with sanitize} {
r restore key 0 $corrupt_payload_7445
} err
assert_match "*Bad data format*" $err
verify_log_message 0 "*Ziplist integrity check failed*" 0
verify_log_message 0 "*integrity check failed*" 0
}
}
......@@ -89,7 +89,7 @@ test {corrupt payload: quicklist small ziplist prev len} {
r restore key 0 "\x0E\x01\x13\x13\x00\x00\x00\x0E\x00\x00\x00\x02\x00\x00\x02\x61\x00\x02\x02\x62\x00\xFF\x09\x00\xC7\x71\x03\x97\x07\x75\xB0\x63"
} err
assert_match "*Bad data format*" $err
verify_log_message 0 "*Ziplist integrity check failed*" 0
verify_log_message 0 "*integrity check failed*" 0
}
}
......@@ -114,7 +114,7 @@ test {corrupt payload: #3080 - quicklist} {
r DUMP key ;# DUMP was used in the original issue, but now even with shallow sanitization restore safely fails, so this is dead code
} err
assert_match "*Bad data format*" $err
verify_log_message 0 "*Ziplist integrity check failed*" 0
verify_log_message 0 "*integrity check failed*" 0
}
}
......@@ -126,7 +126,7 @@ test {corrupt payload: #3080 - ziplist} {
r RESTORE key 0 "\x0A\x80\x00\x00\x00\x10\x41\x41\x41\x41\x41\x41\x41\x41\x02\x00\x00\x80\x41\x41\x41\x41\x07\x00\x39\x5B\x49\xE0\xC1\xC6\xDD\x76"
} err
assert_match "*Bad data format*" $err
verify_log_message 0 "*Ziplist integrity check failed*" 0
verify_log_message 0 "*integrity check failed*" 0
}
}
......@@ -144,7 +144,7 @@ test {corrupt payload: load corrupted rdb with no CRC - #3505} {
set stdout [dict get $srv stdout]
assert_equal [count_message_lines $stdout "Terminating server after rdb file reading failure."] 1
assert_lessthan 1 [count_message_lines $stdout "Ziplist integrity check failed"]
assert_lessthan 1 [count_message_lines $stdout "integrity check failed"]
kill_server $srv ;# let valgrind look for issues
}
......@@ -194,6 +194,36 @@ test {corrupt payload: listpack too long entry prev len} {
}
}
test {corrupt payload: hash ziplist with duplicate records} {
# when we do perform full sanitization, we expect duplicate records to fail the restore
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r config set sanitize-dump-payload yes
r debug set-skip-checksum-validation 1
catch { r RESTORE _hash 0 "\x0D\x3D\x3D\x00\x00\x00\x3A\x00\x00\x00\x14\x13\x00\xF5\x02\xF5\x02\xF2\x02\x53\x5F\x31\x04\xF3\x02\xF3\x02\xF7\x02\xF7\x02\xF8\x02\x02\x5F\x37\x04\xF1\x02\xF1\x02\xF6\x02\x02\x5F\x35\x04\xF4\x02\x02\x5F\x33\x04\xFA\x02\x02\x5F\x39\x04\xF9\x02\xF9\xFF\x09\x00\xB5\x48\xDE\x62\x31\xD0\xE5\x63" } err
assert_match "*Bad data format*" $err
}
}
test {corrupt payload: hash ziplist uneven record count} {
# when we do perform full sanitization, we expect duplicate records to fail the restore
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r config set sanitize-dump-payload yes
r debug set-skip-checksum-validation 1
catch { r RESTORE _hash 0 "\r\x1b\x1b\x00\x00\x00\x16\x00\x00\x00\x04\x00\x00\x02a\x00\x04\x02b\x00\x04\x02a\x00\x04\x02d\x00\xff\t\x00\xa1\x98\x36x\xcc\x8e\x93\x2e" } err
assert_match "*Bad data format*" $err
}
}
test {corrupt payload: hash dupliacte records} {
# when we do perform full sanitization, we expect duplicate records to fail the restore
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r config set sanitize-dump-payload yes
r debug set-skip-checksum-validation 1
catch { r RESTORE _hash 0 "\x04\x02\x01a\x01b\x01a\x01d\t\x00\xc6\x9c\xab\xbc\bk\x0c\x06" } err
assert_match "*Bad data format*" $err
}
}
test {corrupt payload: fuzzer findings - NPD in streamIteratorGetID} {
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r config set sanitize-dump-payload no
......@@ -255,7 +285,7 @@ test {corrupt payload: fuzzer findings - invalid ziplist encoding} {
r RESTORE _listbig 0 "\x0E\x02\x1B\x1B\x00\x00\x00\x16\x00\x00\x00\x05\x00\x00\x02\x5F\x39\x04\xF9\x02\x86\x5F\x37\x04\xF7\x02\x02\x5F\x35\xFF\x19\x19\x00\x00\x00\x16\x00\x00\x00\x05\x00\x00\xF5\x02\x02\x5F\x33\x04\xF3\x02\x02\x5F\x31\x04\xF1\xFF\x09\x00\x0C\xFC\x99\x2C\x23\x45\x15\x60"
} err
assert_match "*Bad data format*" $err
verify_log_message 0 "*Ziplist integrity check failed*" 0
verify_log_message 0 "*integrity check failed*" 0
}
}
......@@ -402,6 +432,18 @@ test {corrupt payload: fuzzer findings - infinite loop} {
}
}
test {corrupt payload: fuzzer findings - hash convert asserts on RESTORE with shallow sanitization} {
# if we don't perform full sanitization, and the next command can assert on converting
# a ziplist to hash records, then we're ok with that happning in RESTORE too
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r config set sanitize-dump-payload no
r debug set-skip-checksum-validation 1
catch { r RESTORE _hash 0 "\x0D\x3D\x3D\x00\x00\x00\x3A\x00\x00\x00\x14\x13\x00\xF5\x02\xF5\x02\xF2\x02\x53\x5F\x31\x04\xF3\x02\xF3\x02\xF7\x02\xF7\x02\xF8\x02\x02\x5F\x37\x04\xF1\x02\xF1\x02\xF6\x02\x02\x5F\x35\x04\xF4\x02\x02\x5F\x33\x04\xFA\x02\x02\x5F\x39\x04\xF9\x02\xF9\xFF\x09\x00\xB5\x48\xDE\x62\x31\xD0\xE5\x63" }
assert_equal [count_log_message 0 "crashed by signal"] 0
assert_equal [count_log_message 0 "ASSERTION FAILED"] 1
}
}
test {corrupt payload: fuzzer findings - invalid tail offset after removal} {
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r config set sanitize-dump-payload no
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment