From ca1c182567add4092e9cb6ea829e9c5193e8fd55 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 13 Aug 2020 16:41:05 +0300 Subject: [PATCH] Sanitize dump payload: ziplist, listpack, zipmap, intset, stream When loading an encoded payload we will at least do a shallow validation to check that the size that's encoded in the payload matches the size of the allocation. This let's us later use this encoded size to make sure the various offsets inside encoded payload don't reach outside the allocation, if they do, we'll assert/panic, but at least we won't segfault or smear memory. We can also do 'deep' validation which runs on all the records of the encoded payload and validates that they don't contain invalid offsets. This lets us detect corruptions early and reject a RESTORE command rather than accepting it and asserting (crashing) later when accessing that payload via some command. configuration: - adding ACL flag skip-sanitize-payload - adding config sanitize-dump-payload [yes/no/clients] For now, we don't have a good way to ensure MIGRATE in cluster resharding isn't being slowed down by these sanitation, so i'm setting the default value to `no`, but later on it should be set to `clients` by default. changes: - changing rdbReportError not to `exit` in RESTORE command - adding a new stat to be able to later check if cluster MIGRATE isn't being slowed down by sanitation. --- redis.conf | 17 ++ src/acl.c | 10 + src/config.c | 8 + src/defrag.c | 1 - src/intset.c | 27 +++ src/intset.h | 1 + src/listpack.c | 146 ++++++++++++- src/listpack.h | 2 + src/rdb.c | 89 ++++++-- src/redis-check-rdb.c | 1 + src/server.c | 3 + src/server.h | 13 ++ src/stream.h | 1 + src/t_hash.c | 6 +- src/t_stream.c | 109 +++++++++- src/ziplist.c | 298 +++++++++++++++++++++----- src/ziplist.h | 3 +- src/zipmap.c | 68 ++++++ src/zipmap.h | 1 + tests/assets/corrupt_ziplist.rdb | Bin 0 -> 1415 bytes tests/cluster/tests/04-resharding.tcl | 7 + tests/integration/corrupt-dump.tcl | 178 +++++++++++++++ tests/support/util.tcl | 16 ++ tests/test_helper.tcl | 1 + 24 files changed, 916 insertions(+), 90 deletions(-) create mode 100644 tests/assets/corrupt_ziplist.rdb create mode 100644 tests/integration/corrupt-dump.tcl diff --git a/redis.conf b/redis.conf index b11459b26..13766171e 100644 --- a/redis.conf +++ b/redis.conf @@ -366,6 +366,21 @@ rdbcompression yes # tell the loading code to skip the check. rdbchecksum yes +# Enables or disables full sanitation checks for ziplist and listpack etc when +# loading an RDB or RESTORE payload. This reduces the chances of a assertion or +# crash later on while processing commands. +# Options: +# no - Never perform full sanitation +# yes - Always perform full sanitation +# clients - Perform full sanitation only for user connections. +# Excludes: RDB files, RESTORE commands received from the master +# connection, and client connections which have the +# skip-sanitize-payload ACL flag. +# The default should be 'clients' but since it currently affects cluster +# resharding via MIGRATE, it is temporarily set to 'no' by default. +# +# sanitize-dump-payload no + # The filename where to dump the DB dbfilename dump.rdb @@ -725,6 +740,8 @@ replica-priority 100 # off Disable the user: it's no longer possible to authenticate # with this user, however the already authenticated connections # will still work. +# skip-sanitize-payload RESTORE dump-payload sanitation is skipped. +# sanitize-payload RESTORE dump-payload is sanitized (default). # + Allow the execution of that command # - Disallow the execution of that command # +@ Allow the execution of all the commands in such category diff --git a/src/acl.c b/src/acl.c index e9acc11a9..a1a7c4237 100644 --- a/src/acl.c +++ b/src/acl.c @@ -89,12 +89,15 @@ struct ACLUserFlag { const char *name; uint64_t flag; } ACLUserFlags[] = { + /* Note: the order here dictates the emitted order at ACLDescribeUser */ {"on", USER_FLAG_ENABLED}, {"off", USER_FLAG_DISABLED}, {"allkeys", USER_FLAG_ALLKEYS}, {"allchannels", USER_FLAG_ALLCHANNELS}, {"allcommands", USER_FLAG_ALLCOMMANDS}, {"nopass", USER_FLAG_NOPASS}, + {"skip-sanitize-payload", USER_FLAG_SANITIZE_PAYLOAD_SKIP}, + {"sanitize-payload", USER_FLAG_SANITIZE_PAYLOAD}, {NULL,0} /* Terminator. */ }; @@ -829,6 +832,12 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { } else if (!strcasecmp(op,"off")) { u->flags |= USER_FLAG_DISABLED; u->flags &= ~USER_FLAG_ENABLED; + } else if (!strcasecmp(op,"skip-sanitize-payload")) { + u->flags |= USER_FLAG_SANITIZE_PAYLOAD_SKIP; + u->flags &= ~USER_FLAG_SANITIZE_PAYLOAD; + } else if (!strcasecmp(op,"sanitize-payload")) { + u->flags &= ~USER_FLAG_SANITIZE_PAYLOAD_SKIP; + u->flags |= USER_FLAG_SANITIZE_PAYLOAD; } else if (!strcasecmp(op,"allkeys") || !strcasecmp(op,"~*")) { @@ -1004,6 +1013,7 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { serverAssert(ACLSetUser(u,"resetkeys",-1) == C_OK); serverAssert(ACLSetUser(u,"resetchannels",-1) == C_OK); serverAssert(ACLSetUser(u,"off",-1) == C_OK); + serverAssert(ACLSetUser(u,"sanitize-payload",-1) == C_OK); serverAssert(ACLSetUser(u,"-@all",-1) == C_OK); } else { errno = EINVAL; diff --git a/src/config.c b/src/config.c index 84b218cd7..5fdde44cd 100644 --- a/src/config.c +++ b/src/config.c @@ -119,6 +119,13 @@ configEnum acl_pubsub_default_enum[] = { {NULL, 0} }; +configEnum sanitize_dump_payload_enum[] = { + {"no", SANITIZE_DUMP_NO}, + {"yes", SANITIZE_DUMP_YES}, + {"clients", SANITIZE_DUMP_CLIENTS}, + {NULL, 0} +}; + /* Output buffer limits presets. */ clientBufferLimitsConfig clientBufferLimitsDefaults[CLIENT_TYPE_OBUF_COUNT] = { {0, 0, 0}, /* normal */ @@ -2361,6 +2368,7 @@ standardConfig configs[] = { createEnumConfig("appendfsync", NULL, MODIFIABLE_CONFIG, aof_fsync_enum, server.aof_fsync, AOF_FSYNC_EVERYSEC, NULL, NULL), createEnumConfig("oom-score-adj", NULL, MODIFIABLE_CONFIG, oom_score_adj_enum, server.oom_score_adj, OOM_SCORE_ADJ_NO, NULL, updateOOMScoreAdj), createEnumConfig("acl-pubsub-default", NULL, MODIFIABLE_CONFIG, acl_pubsub_default_enum, server.acl_pubusub_default, USER_FLAG_ALLCHANNELS, NULL, NULL), + createEnumConfig("sanitize-dump-payload", NULL, MODIFIABLE_CONFIG, sanitize_dump_payload_enum, server.sanitize_dump_payload, SANITIZE_DUMP_NO, NULL, NULL), /* Integer configs */ createIntConfig("databases", NULL, IMMUTABLE_CONFIG, 1, INT_MAX, server.dbnum, 16, INTEGER_CONFIG, NULL, NULL), diff --git a/src/defrag.c b/src/defrag.c index 04ade30ea..165e55f65 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -5,7 +5,6 @@ * We do that by scanning the keyspace and for each pointer we have, we can try to * ask the allocator if moving it to a new address will help reduce fragmentation. * - * Copyright (c) 2020, Oran Agra * Copyright (c) 2020, Redis Labs, Inc * All rights reserved. * diff --git a/src/intset.c b/src/intset.c index 4445a5ca6..0079199cf 100644 --- a/src/intset.c +++ b/src/intset.c @@ -281,6 +281,33 @@ 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; + /* check that we can actually read the header. */ + if (size < sizeof(*is)) + return 0; + + uint32_t encoding = intrev32ifbe(is->encoding); + + size_t record_size; + if (encoding == INTSET_ENC_INT64) { + record_size = INTSET_ENC_INT64; + } else if (encoding == INTSET_ENC_INT32) { + record_size = INTSET_ENC_INT32; + } else if (encoding == INTSET_ENC_INT16){ + record_size = INTSET_ENC_INT16; + } else { + return 0; + } + + /* check that the size matchies (all records are inside the buffer). */ + uint32_t count = intrev32ifbe(is->length); + if (sizeof(*is) + count*record_size != size) + return 0; + + return 1; +} + #ifdef REDIS_TEST #include #include diff --git a/src/intset.h b/src/intset.h index 8119e6636..4fba339d5 100644 --- a/src/intset.h +++ b/src/intset.h @@ -46,6 +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); #ifdef REDIS_TEST int intsetTest(int argc, char *argv[]); diff --git a/src/listpack.c b/src/listpack.c index f8c34429e..580954075 100644 --- a/src/listpack.c +++ b/src/listpack.c @@ -5,6 +5,7 @@ * https://github.com/antirez/listpack * * Copyright (c) 2017, Salvatore Sanfilippo + * Copyright (c) 2020, Redis Labs, Inc * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -41,6 +42,7 @@ #include "listpack.h" #include "listpack_malloc.h" +#include "redisassert.h" #define LP_HDR_SIZE 6 /* 32 bit total len + 16 bit number of elements. */ #define LP_HDR_NUMELE_UNKNOWN UINT16_MAX @@ -114,6 +116,21 @@ (p)[5] = ((v)>>8)&0xff; \ } while(0) +/* Validates that 'p' is not ouside the listpack. + * All function that return a pointer to an element in the listpack will assert + * that this element is valid, so it can be freely used. + * Generally functions such lpNext and lpDelete assume the input pointer is + * already validated (since it's the return value of another function). */ +#define ASSERT_INTEGRITY(lp, p) do { \ + assert((p) >= (lp)+LP_HDR_SIZE && (p) < (lp)+lpGetTotalBytes((lp))); \ +} while (0) + +/* Similar to the above, but validates the entire element lenth rather than just + * it's pointer. */ +#define ASSERT_INTEGRITY_LEN(lp, p, len) do { \ + assert((p) >= (lp)+LP_HDR_SIZE && (p)+(len) < (lp)+lpGetTotalBytes((lp))); \ +} while (0) + /* Convert a string into a signed 64 bit integer. * The function returns 1 if the string could be parsed into a (non-overflowing) * signed 64 bit int, 0 otherwise. The 'value' will be set to the parsed value @@ -367,9 +384,14 @@ void lpEncodeString(unsigned char *buf, unsigned char *s, uint32_t len) { } } -/* Return the encoded length of the listpack element pointed by 'p'. If the - * element encoding is wrong then 0 is returned. */ -uint32_t lpCurrentEncodedSize(unsigned char *p) { +/* Return the encoded length of the listpack element pointed by 'p'. + * This includes the encoding byte, length bytes, and the element data itself. + * If the element encoding is wrong then 0 is returned. + * Note that this method may access additional bytes (in case of 12 and 32 bit + * str), so should only be called when we know 'p' was already validated by + * lpCurrentEncodedSizeBytes or ASSERT_INTEGRITY_LEN (possibly since 'p' is + * a return value of another function that validated its return. */ +uint32_t lpCurrentEncodedSizeUnsafe(unsigned char *p) { if (LP_ENCODING_IS_7BIT_UINT(p[0])) return 1; if (LP_ENCODING_IS_6BIT_STR(p[0])) return 1+LP_ENCODING_6BIT_STR_LEN(p); if (LP_ENCODING_IS_13BIT_INT(p[0])) return 2; @@ -383,12 +405,30 @@ uint32_t lpCurrentEncodedSize(unsigned char *p) { return 0; } +/* Return bytes needed to encode the length of the listpack element pointed by 'p'. + * This includes just the encodign byte, and the bytes needed to encode the length + * of the element (excluding the element data itself) + * If the element encoding is wrong then 0 is returned. */ +uint32_t lpCurrentEncodedSizeBytes(unsigned char *p) { + if (LP_ENCODING_IS_7BIT_UINT(p[0])) return 1; + if (LP_ENCODING_IS_6BIT_STR(p[0])) return 1; + if (LP_ENCODING_IS_13BIT_INT(p[0])) return 1; + if (LP_ENCODING_IS_16BIT_INT(p[0])) return 1; + if (LP_ENCODING_IS_24BIT_INT(p[0])) return 1; + if (LP_ENCODING_IS_32BIT_INT(p[0])) return 1; + if (LP_ENCODING_IS_64BIT_INT(p[0])) return 1; + if (LP_ENCODING_IS_12BIT_STR(p[0])) return 2; + if (LP_ENCODING_IS_32BIT_STR(p[0])) return 5; + if (p[0] == LP_EOF) return 1; + return 0; +} + /* Skip the current entry returning the next. It is invalid to call this * function if the current element is the EOF element at the end of the * listpack, however, while this function is used to implement lpNext(), * it does not return NULL when the EOF element is encountered. */ unsigned char *lpSkip(unsigned char *p) { - unsigned long entrylen = lpCurrentEncodedSize(p); + unsigned long entrylen = lpCurrentEncodedSizeUnsafe(p); entrylen += lpEncodeBacklen(NULL,entrylen); p += entrylen; return p; @@ -398,8 +438,9 @@ unsigned char *lpSkip(unsigned char *p) { * the pointer to the next element (the one on the right), or NULL if 'p' * already pointed to the last element of the listpack. */ unsigned char *lpNext(unsigned char *lp, unsigned char *p) { - ((void) lp); /* lp is not used for now. However lpPrev() uses it. */ + assert(p); p = lpSkip(p); + ASSERT_INTEGRITY(lp, p); if (p[0] == LP_EOF) return NULL; return p; } @@ -408,11 +449,14 @@ unsigned char *lpNext(unsigned char *lp, unsigned char *p) { * the pointer to the previous element (the one on the left), or NULL if 'p' * already pointed to the first element of the listpack. */ unsigned char *lpPrev(unsigned char *lp, unsigned char *p) { + assert(p); if (p-lp == LP_HDR_SIZE) return NULL; p--; /* Seek the first backlen byte of the last element. */ uint64_t prevlen = lpDecodeBacklen(p); prevlen += lpEncodeBacklen(NULL,prevlen); - return p-prevlen+1; /* Seek the first byte of the previous entry. */ + p -= prevlen-1; /* Seek the first byte of the previous entry. */ + ASSERT_INTEGRITY(lp, p); + return p; } /* Return a pointer to the first element of the listpack, or NULL if the @@ -570,8 +614,7 @@ unsigned char *lpGet(unsigned char *p, int64_t *count, unsigned char *intbuf) { /* Insert, delete or replace the specified element 'ele' of length 'len' at * the specified position 'p', with 'p' being a listpack element pointer - * obtained with lpFirst(), lpLast(), lpIndex(), lpNext(), lpPrev() or - * lpSeek(). + * obtained with lpFirst(), lpLast(), lpNext(), lpPrev() or lpSeek(). * * The element is inserted before, after, or replaces the element pointed * by 'p' depending on the 'where' argument, that can be LP_BEFORE, LP_AFTER @@ -610,6 +653,7 @@ unsigned char *lpInsert(unsigned char *lp, unsigned char *ele, uint32_t size, un if (where == LP_AFTER) { p = lpSkip(p); where = LP_BEFORE; + ASSERT_INTEGRITY(lp, p); } /* Store the offset of the element 'p', so that we can obtain its @@ -639,8 +683,9 @@ unsigned char *lpInsert(unsigned char *lp, unsigned char *ele, uint32_t size, un uint64_t old_listpack_bytes = lpGetTotalBytes(lp); uint32_t replaced_len = 0; if (where == LP_REPLACE) { - replaced_len = lpCurrentEncodedSize(p); + replaced_len = lpCurrentEncodedSizeUnsafe(p); replaced_len += lpEncodeBacklen(NULL,replaced_len); + ASSERT_INTEGRITY_LEN(lp, p, replaced_len); } uint64_t new_listpack_bytes = old_listpack_bytes + enclen + backlen_size @@ -801,3 +846,86 @@ unsigned char *lpSeek(unsigned char *lp, long index) { } } +/* Validate the integrity of a single listpack entry and move to the next one. + * The input argument 'pp' is a reference to the current record and is advanced on exit. + * Returns 1 if valid, 0 if invalid. */ +int lpValidateNext(unsigned char *lp, unsigned char **pp, size_t lpbytes) { +#define OUT_OF_RANGE(p) ( \ + (p) < lp + LP_HDR_SIZE || \ + (p) > lp + lpbytes - 1) + unsigned char *p = *pp; + if (!p) + return 0; + + if (*p == LP_EOF) { + *pp = NULL; + return 1; + } + + /* check that we can read the encoded size */ + uint32_t lenbytes = lpCurrentEncodedSizeBytes(p); + if (!lenbytes) + return 0; + + /* make sure the encoded entry length doesn't rech outside the edge of the listpack */ + if (OUT_OF_RANGE(p + lenbytes)) + return 0; + + /* get the entry length and encoded backlen. */ + unsigned long entrylen = lpCurrentEncodedSizeUnsafe(p); + unsigned long encodedBacklen = lpEncodeBacklen(NULL,entrylen); + entrylen += encodedBacklen; + + /* make sure the entry doesn't rech outside the edge of the listpack */ + if (OUT_OF_RANGE(p + entrylen)) + return 0; + + /* move to the next entry */ + p += entrylen; + + /* make sure the encoded length at the end patches the one at the beginning. */ + uint64_t prevlen = lpDecodeBacklen(p-1); + if (prevlen + encodedBacklen != entrylen) + return 0; + + *pp = p; + return 1; +#undef OUT_OF_RANGE +} + +/* 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 lpValidateIntegrity(unsigned char *lp, size_t size, int deep){ + /* Check that we can actually read the header. (and EOF) */ + if (size < LP_HDR_SIZE + 1) + return 0; + + /* Check that the encoded size in the header must match the allocated size. */ + size_t bytes = lpGetTotalBytes(lp); + if (bytes != size) + return 0; + + /* The last byte must be the terminator. */ + if (lp[size-1] != LP_EOF) + return 0; + + if (!deep) + return 1; + + /* Validate the invividual entries. */ + uint32_t count = 0; + unsigned char *p = lpFirst(lp); + while(p) { + if (!lpValidateNext(lp, &p, bytes)) + return 0; + count++; + } + + /* Check that the count in the header is correct */ + uint32_t numele = lpGetNumElements(lp); + if (numele != LP_HDR_NUMELE_UNKNOWN && numele != count) + return 0; + + return 1; +} diff --git a/src/listpack.h b/src/listpack.h index af67b4b41..e8375628b 100644 --- a/src/listpack.h +++ b/src/listpack.h @@ -57,5 +57,7 @@ unsigned char *lpNext(unsigned char *lp, unsigned char *p); unsigned char *lpPrev(unsigned char *lp, unsigned char *p); uint32_t lpBytes(unsigned char *lp); unsigned char *lpSeek(unsigned char *lp, long index); +int lpValidateIntegrity(unsigned char *lp, size_t size, int deep); +int lpValidateNext(unsigned char *lp, unsigned char **pp, size_t lpbytes); #endif diff --git a/src/rdb.c b/src/rdb.c index e3f34b3f0..af527f94c 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -68,17 +68,27 @@ void rdbReportError(int corruption_error, int linenum, char *reason, ...) { vsnprintf(msg+len,sizeof(msg)-len,reason,ap); va_end(ap); - if (!rdbCheckMode) { - if (rdbFileBeingLoaded || corruption_error) { - serverLog(LL_WARNING, "%s", msg); - char *argv[2] = {"",rdbFileBeingLoaded}; - redis_check_rdb_main(2,argv,NULL); - } else { - serverLog(LL_WARNING, "%s. Failure loading rdb format from socket, assuming connection error, resuming operation.", msg); - return; - } - } else { + if (!server.loading) { + /* If we're in the context of a RESTORE command, just propagate the error. */ + /* log in VERBOSE, and return (don't exit). */ + serverLog(LL_VERBOSE, "%s", msg); + return; + } else if (rdbCheckMode) { + /* If we're inside the rdb checker, let it handle the error. */ rdbCheckError("%s",msg); + } else if (rdbFileBeingLoaded) { + /* If we're loading an rdb file form disk, run rdb check (and exit) */ + serverLog(LL_WARNING, "%s", msg); + char *argv[2] = {"",rdbFileBeingLoaded}; + redis_check_rdb_main(2,argv,NULL); + } else if (corruption_error) { + /* In diskless loading, in case of corrupt file, log and exit. */ + serverLog(LL_WARNING, "%s. Failure loading rdb format", msg); + } else { + /* In diskless loading, in case of a short read (not a corrupt + * file), log and proceed (don't exit). */ + serverLog(LL_WARNING, "%s. Failure loading rdb format from socket, assuming connection error, resuming operation.", msg); + return; } serverLog(LL_WARNING, "Terminating server after rdb file reading failure."); exit(1); @@ -1489,6 +1499,17 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key) { uint64_t len; unsigned int i; + int deep_integrity_validation = server.sanitize_dump_payload == SANITIZE_DUMP_YES; + if (server.sanitize_dump_payload == SANITIZE_DUMP_CLIENTS) { + /* Skip sanitization when loading (an RDB), or getting a RESTORE command + * from either the master or a client using an ACL user with the skip-sanitize-payload flag. */ + int skip = server.loading || + (server.current_client && (server.current_client->flags & CLIENT_MASTER)); + if (!skip && server.current_client && server.current_client->user) + skip = !!(server.current_client->user->flags & USER_FLAG_SANITIZE_PAYLOAD_SKIP); + deep_integrity_validation = !skip; + } + if (rdbtype == RDB_TYPE_STRING) { /* Read string value */ if ((o = rdbLoadEncodedStringObject(rdb)) == NULL) return NULL; @@ -1685,12 +1706,20 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key) { server.list_compress_depth); while (len--) { + size_t encoded_len; unsigned char *zl = - rdbGenericLoadStringObject(rdb,RDB_LOAD_PLAIN,NULL); + rdbGenericLoadStringObject(rdb,RDB_LOAD_PLAIN,&encoded_len); if (zl == NULL) { decrRefCount(o); return NULL; } + if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; + if (!ziplistValidateIntegrity(zl, encoded_len, deep_integrity_validation)) { + rdbExitReportCorruptRDB("Ziplist integrity check failed."); + decrRefCount(o); + zfree(zl); + return NULL; + } quicklistAppendZiplist(o->ptr, zl); } } else if (rdbtype == RDB_TYPE_HASH_ZIPMAP || @@ -1699,9 +1728,32 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key) { rdbtype == RDB_TYPE_ZSET_ZIPLIST || rdbtype == RDB_TYPE_HASH_ZIPLIST) { + size_t encoded_len; unsigned char *encoded = - rdbGenericLoadStringObject(rdb,RDB_LOAD_PLAIN,NULL); + rdbGenericLoadStringObject(rdb,RDB_LOAD_PLAIN,&encoded_len); if (encoded == NULL) return NULL; + if (rdbtype == RDB_TYPE_HASH_ZIPMAP) { + /* Since we don't keep zipmaps anymore, the rdb loading for these + * is O(n) anyway, use `deep` validation. */ + if (!zipmapValidateIntegrity(encoded, encoded_len, 1)) { + rdbExitReportCorruptRDB("Zipmap integrity check failed."); + zfree(encoded); + return NULL; + } + } else if (rdbtype == RDB_TYPE_SET_INTSET) { + if (!intsetValidateIntegrity(encoded, encoded_len)) { + rdbExitReportCorruptRDB("Intset integrity check failed."); + zfree(encoded); + return NULL; + } + } else { /* ziplist */ + if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; + if (!ziplistValidateIntegrity(encoded, encoded_len, deep_integrity_validation)) { + rdbExitReportCorruptRDB("Ziplist integrity check failed."); + zfree(encoded); + return NULL; + } + } o = createObject(OBJ_STRING,encoded); /* Obj type fixed below. */ /* Fix the object encoding, and make sure to convert the encoded @@ -1794,14 +1846,24 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key) { } /* Load the listpack. */ + size_t lp_size; unsigned char *lp = - rdbGenericLoadStringObject(rdb,RDB_LOAD_PLAIN,NULL); + rdbGenericLoadStringObject(rdb,RDB_LOAD_PLAIN,&lp_size); if (lp == NULL) { rdbReportReadError("Stream listpacks loading failed."); sdsfree(nodekey); decrRefCount(o); return NULL; } + if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; + if (!streamValidateListpackIntegrity(lp, lp_size, deep_integrity_validation)) { + rdbExitReportCorruptRDB("Stream listpack integrity check failed."); + sdsfree(nodekey); + decrRefCount(o); + zfree(lp); + return NULL; + } + unsigned char *first = lpFirst(lp); if (first == NULL) { /* Serialized listpacks should never be empty, since on @@ -2389,6 +2451,7 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) { (unsigned long long)expected, (unsigned long long)cksum); rdbExitReportCorruptRDB("RDB CRC error"); + return C_ERR; } } } diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index f399864dd..79dbf3229 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -372,6 +372,7 @@ int redis_check_rdb_main(int argc, char **argv, FILE *fp) { if (shared.integers[0] == NULL) createSharedObjects(); server.loading_process_events_interval_bytes = 0; + server.sanitize_dump_payload = SANITIZE_DUMP_YES; rdbCheckMode = 1; rdbCheckInfo("Checking RDB file %s", argv[1]); rdbCheckSetupSignals(); diff --git a/src/server.c b/src/server.c index c30577da4..1b6f71aef 100644 --- a/src/server.c +++ b/src/server.c @@ -2934,6 +2934,7 @@ void resetServerStats(void) { atomicSet(server.stat_net_input_bytes, 0); atomicSet(server.stat_net_output_bytes, 0); server.stat_unexpected_error_replies = 0; + server.stat_dump_payload_sanitizations = 0; server.aof_delayed_fsync = 0; server.blocked_last_cron = 0; } @@ -4654,6 +4655,7 @@ sds genRedisInfoString(const char *section) { "tracking_total_items:%lld\r\n" "tracking_total_prefixes:%lld\r\n" "unexpected_error_replies:%lld\r\n" + "dump_payload_sanitizations:%lld\r\n" "total_reads_processed:%lld\r\n" "total_writes_processed:%lld\r\n" "io_threaded_reads_processed:%lld\r\n" @@ -4689,6 +4691,7 @@ sds genRedisInfoString(const char *section) { (unsigned long long) trackingGetTotalItems(), (unsigned long long) trackingGetTotalPrefixes(), server.stat_unexpected_error_replies, + server.stat_dump_payload_sanitizations, stat_total_reads_processed, stat_total_writes_processed, server.stat_io_reads_processed, diff --git a/src/server.h b/src/server.h index 5bbe7876c..8a35d48df 100644 --- a/src/server.h +++ b/src/server.h @@ -378,6 +378,11 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT]; #define TLS_CLIENT_AUTH_YES 1 #define TLS_CLIENT_AUTH_OPTIONAL 2 +/* Sanitize dump payload */ +#define SANITIZE_DUMP_NO 0 +#define SANITIZE_DUMP_YES 1 +#define SANITIZE_DUMP_CLIENTS 2 + /* Sets operations codes */ #define SET_OP_UNION 0 #define SET_OP_DIFF 1 @@ -781,6 +786,12 @@ typedef struct readyList { authenticated. */ #define USER_FLAG_ALLCHANNELS (1<<5) /* The user can mention any Pub/Sub channel. */ +#define USER_FLAG_SANITIZE_PAYLOAD (1<<6) /* The user require a deep RESTORE + * payload sanitization. */ +#define USER_FLAG_SANITIZE_PAYLOAD_SKIP (1<<7) /* The user should skip the + * deep sanitization of RESTORE + * payload. */ + typedef struct { sds name; /* The username as an SDS string. */ uint64_t flags; /* See USER_FLAG_* */ @@ -1213,6 +1224,7 @@ struct redisServer { size_t stat_module_cow_bytes; /* Copy on write bytes during module fork. */ uint64_t stat_clients_type_memory[CLIENT_TYPE_COUNT];/* Mem usage by type */ long long stat_unexpected_error_replies; /* Number of unexpected (aof-loading, replica to master, etc.) error replies */ + long long stat_dump_payload_sanitizations; /* Number deep dump payloads integrity validations. */ long long stat_io_reads_processed; /* Number of read events processed by IO / Main threads */ long long stat_io_writes_processed; /* Number of write events processed by IO / Main threads */ redisAtomic long long stat_total_reads_processed; /* Total number of read events processed */ @@ -1232,6 +1244,7 @@ struct redisServer { int active_expire_enabled; /* Can be disabled for testing purposes. */ int active_expire_effort; /* From 1 (default) to 10, active effort. */ int active_defrag_enabled; + int sanitize_dump_payload; /* Enables deep sanitization for ziplist and listpack in RDB and RESTORE. */ int jemalloc_bg_thread; /* Enable jemalloc background thread */ size_t active_defrag_ignore_bytes; /* minimum amount of fragmentation waste to start active defrag */ int active_defrag_threshold_lower; /* minimum percentage of fragmentation to start active defrag */ diff --git a/src/stream.h b/src/stream.h index 1a1e271a8..c7acee719 100644 --- a/src/stream.h +++ b/src/stream.h @@ -120,5 +120,6 @@ int streamIncrID(streamID *id); int streamDecrID(streamID *id); void streamPropagateConsumerCreation(client *c, robj *key, robj *groupname, sds consumername); robj *streamDup(robj *o); +int streamValidateListpackIntegrity(unsigned char *lp, size_t size, int deep); #endif diff --git a/src/t_hash.c b/src/t_hash.c index f3fde87aa..e6f14091b 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -67,7 +67,7 @@ int hashTypeGetFromZiplist(robj *o, sds field, zl = o->ptr; fptr = ziplistIndex(zl, ZIPLIST_HEAD); if (fptr != NULL) { - fptr = ziplistFind(fptr, (unsigned char*)field, sdslen(field), 1); + fptr = ziplistFind(zl, fptr, (unsigned char*)field, sdslen(field), 1); if (fptr != NULL) { /* Grab pointer to the value (fptr points to the field) */ vptr = ziplistNext(zl, fptr); @@ -208,7 +208,7 @@ int hashTypeSet(robj *o, sds field, sds value, int flags) { zl = o->ptr; fptr = ziplistIndex(zl, ZIPLIST_HEAD); if (fptr != NULL) { - fptr = ziplistFind(fptr, (unsigned char*)field, sdslen(field), 1); + fptr = ziplistFind(zl, fptr, (unsigned char*)field, sdslen(field), 1); if (fptr != NULL) { /* Grab pointer to the value (fptr points to the field) */ vptr = ziplistNext(zl, fptr); @@ -285,7 +285,7 @@ int hashTypeDelete(robj *o, sds field) { zl = o->ptr; fptr = ziplistIndex(zl, ZIPLIST_HEAD); if (fptr != NULL) { - fptr = ziplistFind(fptr, (unsigned char*)field, sdslen(field), 1); + fptr = ziplistFind(zl, fptr, (unsigned char*)field, sdslen(field), 1); if (fptr != NULL) { zl = ziplistDelete(zl,&fptr); /* Delete the key. */ zl = ziplistDelete(zl,&fptr); /* Delete the value. */ diff --git a/src/t_stream.c b/src/t_stream.c index bb38b0fb8..70cc8bfa1 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -255,21 +255,33 @@ unsigned char *lpReplaceInteger(unsigned char *lp, unsigned char **pos, int64_t /* This is a wrapper function for lpGet() to directly get an integer value * from the listpack (that may store numbers as a string), converting - * the string if needed. */ -int64_t lpGetInteger(unsigned char *ele) { + * the string if needed. + * The 'valid" argument is an optional output parameter to get an indication + * if the record was valid, when this parameter is NULL, the function will + * fail with an assertion. */ +static inline int64_t lpGetIntegerIfValid(unsigned char *ele, int *valid) { int64_t v; unsigned char *e = lpGet(ele,&v,NULL); - if (e == NULL) return v; + if (e == NULL) { + if (valid) + *valid = 1; + return v; + } /* The following code path should never be used for how listpacks work: * they should always be able to store an int64_t value in integer * encoded form. However the implementation may change. */ long long ll; - int retval = string2ll((char*)e,v,&ll); - serverAssert(retval != 0); + int ret = string2ll((char*)e,v,&ll); + if (valid) + *valid = ret; + else + serverAssert(ret != 0); v = ll; return v; } +#define lpGetInteger(ele) lpGetIntegerIfValid(ele, NULL) + /* Debugging function to log the full content of a listpack. Useful * for development and debugging. */ void streamLogListpackContent(unsigned char *lp) { @@ -3037,3 +3049,90 @@ NULL addReplySubcommandSyntaxError(c); } } + +/* Validate the integrity stream listpack entries stracture. Both in term of a + * valid listpack, but also that the stracture of the entires matches a valid + * stream. return 1 if valid 0 if not valid. */ +int streamValidateListpackIntegrity(unsigned char *lp, size_t size, int deep) { + int valid_record; + unsigned char *p, *next; + + /* Since we don't want to run validation of all records twice, we'll + * run the listpack validation of just the header and do the rest here. */ + if (!lpValidateIntegrity(lp, size, 0)) + return 0; + + /* In non-deep mode we just validated the listpack header (encoded size) */ + if (!deep) return 1; + + next = p = lpFirst(lp); + if (!lpValidateNext(lp, &next, size)) return 0; + if (!p) return 0; + + /* entry count */ + int64_t entry_count = lpGetIntegerIfValid(p, &valid_record); + if (!valid_record) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + /* deleted */ + lpGetIntegerIfValid(p, &valid_record); + if (!valid_record) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + /* num-of-fields */ + int64_t master_fields = lpGetIntegerIfValid(p, &valid_record); + if (!valid_record) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + /* the field names */ + for (int64_t j = 0; j < master_fields; j++) { + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + } + + /* the zero master entry terminator. */ + int64_t zero = lpGetIntegerIfValid(p, &valid_record); + if (!valid_record || zero != 0) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + while (entry_count--) { + if (!p) return 0; + int64_t fields = master_fields, extra_fields = 3; + int64_t flags = lpGetIntegerIfValid(p, &valid_record); + if (!valid_record) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + /* entry id */ + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + if (!(flags & STREAM_ITEM_FLAG_SAMEFIELDS)) { + /* num-of-fields */ + fields = lpGetIntegerIfValid(p, &valid_record); + if (!valid_record) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + + /* the field names */ + for (int64_t j = 0; j < fields; j++) { + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + } + + extra_fields += fields + 1; + } + + /* the values */ + for (int64_t j = 0; j < fields; j++) { + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + } + + /* lp-count */ + int64_t lp_count = lpGetIntegerIfValid(p, &valid_record); + if (!valid_record) return 0; + if (lp_count != fields + extra_fields) return 0; + p = next; if (!lpValidateNext(lp, &next, size)) return 0; + } + + if (next) + return 0; + + return 1; +} diff --git a/src/ziplist.c b/src/ziplist.c index dd7452006..a8ab57cfe 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -152,6 +152,7 @@ * * Copyright (c) 2009-2012, Pieter Noordhuis * Copyright (c) 2009-2017, Salvatore Sanfilippo + * Copyright (c) 2020, Redis Labs, Inc * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -298,10 +299,34 @@ typedef struct zlentry { /* Extract the encoding from the byte pointed by 'ptr' and set it into * 'encoding' field of the zlentry structure. */ #define ZIP_ENTRY_ENCODING(ptr, encoding) do { \ - (encoding) = (ptr[0]); \ + (encoding) = ((ptr)[0]); \ if ((encoding) < ZIP_STR_MASK) (encoding) &= ZIP_STR_MASK; \ } while(0) +#define ZIP_ENCODING_SIZE_INVALID 0xff +/* Return the number of bytes required to encode the entry type + length. + * On error, return ZIP_ENCODING_SIZE_INVALID */ +static inline unsigned int zipEncodingLenSize(unsigned char encoding) { + if (encoding == ZIP_INT_16B || encoding == ZIP_INT_32B || + encoding == ZIP_INT_24B || encoding == ZIP_INT_64B || + encoding == ZIP_INT_8B) + return 1; + if (encoding >= ZIP_INT_IMM_MIN && encoding <= ZIP_INT_IMM_MAX) + return 1; + if (encoding == ZIP_STR_06B) + return 1; + if (encoding == ZIP_STR_14B) + return 2; + if (encoding == ZIP_STR_32B) + return 5; + return ZIP_ENCODING_SIZE_INVALID; +} + +#define ZIP_ASSERT_ENCODING(encoding) do { \ + if (zipEncodingLenSize(encoding) == ZIP_ENCODING_SIZE_INVALID) \ + panic("Invalid encoding 0x%02X", encoding); \ +} while (0) + /* Return bytes needed to store integer encoded by 'encoding'. */ unsigned int zipIntSize(unsigned char encoding) { switch(encoding) { @@ -313,7 +338,7 @@ unsigned int zipIntSize(unsigned char encoding) { } if (encoding >= ZIP_INT_IMM_MIN && encoding <= ZIP_INT_IMM_MAX) return 0; /* 4 bit immediate */ - panic("Invalid integer encoding 0x%02X", encoding); + /* bad encoding, covered by a previous call to ZIP_ASSERT_ENCODING */ return 0; } @@ -365,11 +390,10 @@ unsigned int zipStoreEntryEncoding(unsigned char *p, unsigned char encoding, uns /* Decode the entry encoding type and data length (string length for strings, * number of bytes used for the integer for integer entries) encoded in 'ptr'. - * The 'encoding' variable will hold the entry encoding, the 'lensize' + * The 'encoding' variable is input, extracted by the caller, the 'lensize' * variable will hold the number of bytes required to encode the entry * length, and the 'len' variable will hold the entry length. */ #define ZIP_DECODE_LENGTH(ptr, encoding, lensize, len) do { \ - ZIP_ENTRY_ENCODING((ptr), (encoding)); \ if ((encoding) < ZIP_STR_MASK) { \ if ((encoding) == ZIP_STR_06B) { \ (lensize) = 1; \ @@ -384,7 +408,8 @@ unsigned int zipStoreEntryEncoding(unsigned char *p, unsigned char encoding, uns ((ptr)[3] << 8) | \ ((ptr)[4]); \ } else { \ - panic("Invalid string encoding 0x%02X", (encoding)); \ + len = 0; /* bad encoding, dead code */ \ + /* covered by a previous call to ZIP_ASSERT_ENCODING */ \ } \ } else { \ (lensize) = 1; \ @@ -440,9 +465,10 @@ unsigned int zipStorePrevEntryLength(unsigned char *p, unsigned int len) { if ((prevlensize) == 1) { \ (prevlen) = (ptr)[0]; \ } else if ((prevlensize) == 5) { \ - assert(sizeof((prevlen)) == 4); \ - memcpy(&(prevlen), ((char*)(ptr)) + 1, 4); \ - memrev32ifbe(&prevlen); \ + (prevlen) = ((ptr)[4] << 24) | \ + ((ptr)[3] << 16) | \ + ((ptr)[2] << 8) | \ + ((ptr)[1]); \ } \ } while(0) @@ -467,14 +493,6 @@ int zipPrevLenByteDiff(unsigned char *p, unsigned int len) { return zipStorePrevEntryLength(NULL, len) - prevlensize; } -/* Return the total number of bytes used by the entry pointed to by 'p'. */ -unsigned int zipRawEntryLength(unsigned char *p) { - unsigned int prevlensize, encoding, lensize, len; - ZIP_DECODE_PREVLENSIZE(p, prevlensize); - ZIP_DECODE_LENGTH(p + prevlensize, encoding, lensize, len); - return prevlensize + lensize + len; -} - /* Check if string pointed to by 'entry' can be encoded as an integer. * Stores the integer value in 'v' and its encoding in 'encoding'. */ int zipTryEncoding(unsigned char *entry, unsigned int entrylen, long long *v, unsigned char *encoding) { @@ -565,15 +583,87 @@ int64_t zipLoadInteger(unsigned char *p, unsigned char encoding) { return ret; } -/* Return a struct with all information about an entry. */ +/* Fills a struct with all information about an entry. + * This function is the "unsafe" alternative to the one blow. + * Generally, all function that return a pointer to an element in the ziplist + * will assert that this element is valid, so it can be freely used. + * Generally functions such ziplistGet assume the input pointer is already + * validated (since it's the return value of another function). */ void zipEntry(unsigned char *p, zlentry *e) { - ZIP_DECODE_PREVLEN(p, e->prevrawlensize, e->prevrawlen); + ZIP_ENTRY_ENCODING(p + e->prevrawlensize, e->encoding); + ZIP_ASSERT_ENCODING(e->encoding); ZIP_DECODE_LENGTH(p + e->prevrawlensize, e->encoding, e->lensize, e->len); e->headersize = e->prevrawlensize + e->lensize; e->p = p; } +/* Fills a struct with all information about an entry. + * This function is safe to use on untrusted pointers, it'll make sure not to + * try to access memory outside the ziplist payload. + * Returns 1 if the entry is valid, and 0 otherwise. */ +static inline int zipEntrySafe(unsigned char* zl, size_t zlbytes, unsigned char *p, zlentry *e, int validate_prevlen) { +#define OUT_OF_RANGE(p) ( \ + (p) < zl + ZIPLIST_HEADER_SIZE || \ + (p) > zl + zlbytes - ZIPLIST_END_SIZE) + + /* Make sure the pointer doesn't rech outside the edge of the ziplist */ + if (OUT_OF_RANGE(p)) + return 0; + + /* Make sure the encoded prevlen header doesn't reach outside the allocation */ + ZIP_DECODE_PREVLENSIZE(p, e->prevrawlensize); + if (OUT_OF_RANGE(p + e->prevrawlensize)) + return 0; + + /* Make sure encoded entry header is valid. */ + ZIP_ENTRY_ENCODING(p + e->prevrawlensize, e->encoding); + e->lensize = zipEncodingLenSize(e->encoding); + if (e->lensize == ZIP_ENCODING_SIZE_INVALID) + return 0; + + /* Make sure the encoded entry header doesn't reach outside the allocation */ + if (OUT_OF_RANGE(p + e->prevrawlensize + e->lensize)) + return 0; + + /* Decode the prevlen and entry len headers. */ + ZIP_DECODE_PREVLEN(p, e->prevrawlensize, e->prevrawlen); + ZIP_DECODE_LENGTH(p + e->prevrawlensize, e->encoding, e->lensize, e->len); + e->headersize = e->prevrawlensize + e->lensize; + + /* Make sure the entry doesn't rech outside the edge of the ziplist */ + if (OUT_OF_RANGE(p + e->headersize + e->len)) + return 0; + + /* Make sure prevlen doesn't rech outside the edge of the ziplist */ + if (validate_prevlen && OUT_OF_RANGE(p - e->prevrawlen)) + return 0; + + e->p = p; + return 1; +#undef OUT_OF_RANGE +} + +/* Return the total number of bytes used by the entry pointed to by 'p'. */ +static inline unsigned int zipRawEntryLengthSafe(unsigned char* zl, size_t zlbytes, unsigned char *p) { + zlentry e; + assert(zipEntrySafe(zl, zlbytes, p, &e, 0)); + return e.headersize + e.len; +} + +/* Return the total number of bytes used by the entry pointed to by 'p'. */ +static inline unsigned int zipRawEntryLength(unsigned char *p) { + zlentry e; + zipEntry(p, &e); + return e.headersize + e.len; +} + +/* Validate that the entry doesn't reach outside the ziplist allocation. */ +static inline void zipAssertValidEntry(unsigned char* zl, size_t zlbytes, unsigned char *p) { + zlentry e; + assert(zipEntrySafe(zl, zlbytes, p, &e, 1)); +} + /* Create a new empty ziplist. */ unsigned char *ziplistNew(void) { unsigned int bytes = ZIPLIST_HEADER_SIZE+ZIPLIST_END_SIZE; @@ -625,7 +715,7 @@ unsigned char *__ziplistCascadeUpdate(unsigned char *zl, unsigned char *p) { /* Empty ziplist */ if (p[0] == ZIP_END) return zl; - zipEntry(p, &cur); + zipEntry(p, &cur); /* no need for "safe" variant since the input pointer was validated by the function that returned it. */ firstentrylen = prevlen = cur.headersize + cur.len; prevlensize = zipStorePrevEntryLength(NULL, prevlen); prevoffset = p - zl; @@ -633,7 +723,7 @@ unsigned char *__ziplistCascadeUpdate(unsigned char *zl, unsigned char *p) { /* Iterate ziplist to find out how many extra bytes do we need to update it. */ while (p[0] != ZIP_END) { - zipEntry(p, &cur); + assert(zipEntrySafe(zl, curlen, p, &cur, 0)); /* Abort when "prevlen" has not changed. */ if (cur.prevrawlen == prevlen) break; @@ -690,7 +780,7 @@ unsigned char *__ziplistCascadeUpdate(unsigned char *zl, unsigned char *p) { /* Iterate all entries that need to be updated tail to head. */ while (cnt) { - zipEntry(zl + prevoffset, &cur); + zipEntry(zl + prevoffset, &cur); /* no need for "safe" variant since we already iterated on all these entries above. */ rawlen = cur.headersize + cur.len; /* Move entry to tail and reset prevlen. */ memmove(p - (rawlen - cur.prevrawlensize), @@ -717,15 +807,18 @@ unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsigned int size_t offset; int nextdiff = 0; zlentry first, tail; + size_t zlbytes = intrev32ifbe(ZIPLIST_BYTES(zl)); - zipEntry(p, &first); + zipEntry(p, &first); /* no need for "safe" variant since the input pointer was validated by the function that returned it. */ for (i = 0; p[0] != ZIP_END && i < num; i++) { - p += zipRawEntryLength(p); + p += zipRawEntryLengthSafe(zl, zlbytes, p); deleted++; } + assert(p >= first.p); totlen = p-first.p; /* Bytes taken by the element(s) to delete. */ if (totlen > 0) { + uint32_t set_tail; if (p[0] != ZIP_END) { /* Storing `prevrawlen` in this entry may increase or decrease the * number of bytes required compare to the current `prevrawlen`. @@ -738,36 +831,44 @@ unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsigned int * had a 5 bytes prevlen header, so there is for sure at least * 5 bytes free and we need just 4. */ p -= nextdiff; + assert(p >= first.p && p= first.p. we know totlen >= 0, + * so we know that p > first.p and this is guaranteed not to reach + * beyond the allocation, even if the entries lens are corrupted. */ + size_t bytes_to_move = zlbytes-(p-zl)-1; + memmove(first.p,p,bytes_to_move); } else { /* The entire tail was deleted. No need to move memory. */ - ZIPLIST_TAIL_OFFSET(zl) = - intrev32ifbe((first.p-zl)-first.prevrawlen); + set_tail = (first.p-zl)-first.prevrawlen; } - /* Resize and update length */ + /* Resize the ziplist */ offset = first.p-zl; - zl = ziplistResize(zl, intrev32ifbe(ZIPLIST_BYTES(zl))-totlen+nextdiff); - ZIPLIST_INCR_LENGTH(zl,-deleted); + zlbytes -= totlen - nextdiff; + zl = ziplistResize(zl, zlbytes); p = zl+offset; + /* Update record count */ + ZIPLIST_INCR_LENGTH(zl,-deleted); + + /* Set the tail offset computed above */ + assert(set_tail <= zlbytes - ZIPLIST_END_SIZE); + ZIPLIST_TAIL_OFFSET(zl) = intrev32ifbe(set_tail); + /* When nextdiff != 0, the raw length of the next entry has changed, so * we need to cascade the update throughout the ziplist */ if (nextdiff != 0) @@ -778,7 +879,7 @@ unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsigned int /* Insert item at "p". */ unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsigned char *s, unsigned int slen) { - size_t curlen = intrev32ifbe(ZIPLIST_BYTES(zl)), reqlen; + size_t curlen = intrev32ifbe(ZIPLIST_BYTES(zl)), reqlen, newlen; unsigned int prevlensize, prevlen = 0; size_t offset; int nextdiff = 0; @@ -794,7 +895,7 @@ unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsigned cha } else { unsigned char *ptail = ZIPLIST_ENTRY_TAIL(zl); if (ptail[0] != ZIP_END) { - prevlen = zipRawEntryLength(ptail); + prevlen = zipRawEntryLengthSafe(zl, curlen, ptail); } } @@ -824,7 +925,8 @@ unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsigned cha /* Store offset because a realloc may change the address of zl. */ offset = p-zl; - zl = ziplistResize(zl,curlen+reqlen+nextdiff); + newlen = curlen+reqlen+nextdiff; + zl = ziplistResize(zl,newlen); p = zl+offset; /* Apply memory move when necessary and update tail offset. */ @@ -845,7 +947,7 @@ unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsigned cha /* When the tail contains more than one entry, we need to take * "nextdiff" in account as well. Otherwise, a change in the * size of prevlen doesn't have an effect on the *tail* offset. */ - zipEntry(p+reqlen, &tail); + assert(zipEntrySafe(zl, newlen, p+reqlen, &tail, 1)); if (p[reqlen+tail.headersize+tail.len] != ZIP_END) { ZIPLIST_TAIL_OFFSET(zl) = intrev32ifbe(intrev32ifbe(ZIPLIST_TAIL_OFFSET(zl))+nextdiff); @@ -1002,23 +1104,35 @@ unsigned char *ziplistPush(unsigned char *zl, unsigned char *s, unsigned int sle unsigned char *ziplistIndex(unsigned char *zl, int index) { unsigned char *p; unsigned int prevlensize, prevlen = 0; + size_t zlbytes = intrev32ifbe(ZIPLIST_BYTES(zl)); if (index < 0) { index = (-index)-1; p = ZIPLIST_ENTRY_TAIL(zl); if (p[0] != ZIP_END) { + /* No need for "safe" check: when going backwards, we know the header + * we're parsing is in the range, we just need to assert (below) that + * the size we take doesn't cause p to go outside the allocation. */ ZIP_DECODE_PREVLEN(p, prevlensize, prevlen); while (prevlen > 0 && index--) { p -= prevlen; + assert(p >= zl + ZIPLIST_HEADER_SIZE && p < zl + zlbytes - ZIPLIST_END_SIZE); ZIP_DECODE_PREVLEN(p, prevlensize, prevlen); } } } else { p = ZIPLIST_ENTRY_HEAD(zl); - while (p[0] != ZIP_END && index--) { - p += zipRawEntryLength(p); + while (index--) { + /* Use the "safe" length: When we go forward, we need to be careful + * not to decode an entry header if it's past the ziplist allocation. */ + p += zipRawEntryLengthSafe(zl, zlbytes, p); + if (p[0] == ZIP_END) + break; } } - return (p[0] == ZIP_END || index > 0) ? NULL : p; + if (p[0] == ZIP_END || index > 0) + return NULL; + zipAssertValidEntry(zl, zlbytes, p); + return p; } /* Return pointer to next entry in ziplist. @@ -1029,6 +1143,7 @@ unsigned char *ziplistIndex(unsigned char *zl, int index) { * The element after 'p' is returned, otherwise NULL if we are at the end. */ unsigned char *ziplistNext(unsigned char *zl, unsigned char *p) { ((void) zl); + size_t zlbytes = intrev32ifbe(ZIPLIST_BYTES(zl)); /* "p" could be equal to ZIP_END, caused by ziplistDelete, * and we should return NULL. Otherwise, we should return NULL @@ -1042,6 +1157,7 @@ unsigned char *ziplistNext(unsigned char *zl, unsigned char *p) { return NULL; } + zipAssertValidEntry(zl, zlbytes, p); return p; } @@ -1060,7 +1176,10 @@ unsigned char *ziplistPrev(unsigned char *zl, unsigned char *p) { } else { ZIP_DECODE_PREVLEN(p, prevlensize, prevlen); assert(prevlen > 0); - return p-prevlen; + p-=prevlen; + size_t zlbytes = intrev32ifbe(ZIPLIST_BYTES(zl)); + zipAssertValidEntry(zl, zlbytes, p); + return p; } } @@ -1073,7 +1192,7 @@ unsigned int ziplistGet(unsigned char *p, unsigned char **sstr, unsigned int *sl if (p == NULL || p[0] == ZIP_END) return 0; if (sstr) *sstr = NULL; - zipEntry(p, &entry); + zipEntry(p, &entry); /* no need for "safe" variant since the input pointer was validated by the function that returned it. */ if (ZIP_IS_STR(entry.encoding)) { if (sstr) { *slen = entry.len; @@ -1121,7 +1240,7 @@ unsigned int ziplistCompare(unsigned char *p, unsigned char *sstr, unsigned int long long zval, sval; if (p[0] == ZIP_END) return 0; - zipEntry(p, &entry); + zipEntry(p, &entry); /* no need for "safe" variant since the input pointer was validated by the function that returned it. */ if (ZIP_IS_STR(entry.encoding)) { /* Raw compare */ if (entry.len == slen) { @@ -1142,23 +1261,23 @@ unsigned int ziplistCompare(unsigned char *p, unsigned char *sstr, unsigned int /* Find pointer to the entry equal to the specified entry. Skip 'skip' entries * between every comparison. Returns NULL when the field could not be found. */ -unsigned char *ziplistFind(unsigned char *p, unsigned char *vstr, unsigned int vlen, unsigned int skip) { +unsigned char *ziplistFind(unsigned char *zl, unsigned char *p, unsigned char *vstr, unsigned int vlen, unsigned int skip) { int skipcnt = 0; unsigned char vencoding = 0; long long vll = 0; + size_t zlbytes = ziplistBlobLen(zl); while (p[0] != ZIP_END) { - unsigned int prevlensize, encoding, lensize, len; + struct zlentry e; unsigned char *q; - ZIP_DECODE_PREVLENSIZE(p, prevlensize); - ZIP_DECODE_LENGTH(p + prevlensize, encoding, lensize, len); - q = p + prevlensize + lensize; + assert(zipEntrySafe(zl, zlbytes, p, &e, 1)); + q = p + e.prevrawlensize + e.lensize; if (skipcnt == 0) { /* Compare current entry with specified entry */ - if (ZIP_IS_STR(encoding)) { - if (len == vlen && memcmp(q, vstr, vlen) == 0) { + if (ZIP_IS_STR(e.encoding)) { + if (e.len == vlen && memcmp(q, vstr, vlen) == 0) { return p; } } else { @@ -1180,7 +1299,7 @@ unsigned char *ziplistFind(unsigned char *p, unsigned char *vstr, unsigned int v * if vencoding != UCHAR_MAX because if there is no encoding * possible for the field it can't be a valid integer. */ if (vencoding != UCHAR_MAX) { - long long ll = zipLoadInteger(q, encoding); + long long ll = zipLoadInteger(q, e.encoding); if (ll == vll) { return p; } @@ -1195,7 +1314,7 @@ unsigned char *ziplistFind(unsigned char *p, unsigned char *vstr, unsigned int v } /* Move to next entry */ - p = q + len; + p = q + e.len; } return NULL; @@ -1208,8 +1327,9 @@ unsigned int ziplistLen(unsigned char *zl) { len = intrev16ifbe(ZIPLIST_LENGTH(zl)); } else { unsigned char *p = zl+ZIPLIST_HEADER_SIZE; + size_t zlbytes = intrev32ifbe(ZIPLIST_BYTES(zl)); while (*p != ZIP_END) { - p += zipRawEntryLength(p); + p += zipRawEntryLengthSafe(zl, zlbytes, p); len++; } @@ -1228,6 +1348,7 @@ void ziplistRepr(unsigned char *zl) { unsigned char *p; int index = 0; zlentry entry; + size_t zlbytes = ziplistBlobLen(zl); printf( "{total bytes %u} " @@ -1238,7 +1359,7 @@ void ziplistRepr(unsigned char *zl) { intrev32ifbe(ZIPLIST_TAIL_OFFSET(zl))); p = ZIPLIST_ENTRY_HEAD(zl); while(*p != ZIP_END) { - zipEntry(p, &entry); + assert(zipEntrySafe(zl, zlbytes, p, &entry, 1)); printf( "{\n" "\taddr 0x%08lx,\n" @@ -1282,6 +1403,63 @@ void ziplistRepr(unsigned char *zl) { printf("{end}\n\n"); } +/* 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) { + /* check that we can actually read the header. (and ZIP_END) */ + if (size < ZIPLIST_HEADER_SIZE + ZIPLIST_END_SIZE) + return 0; + + /* check that the encoded size in the header must match the allocated size. */ + size_t bytes = intrev32ifbe(ZIPLIST_BYTES(zl)); + if (bytes != size) + return 0; + + /* the last byte must be the terminator. */ + if (zl[size - ZIPLIST_END_SIZE] != ZIP_END) + return 0; + + /* make sure the tail offset isn't reaching outside the allocation. */ + if (intrev32ifbe(ZIPLIST_TAIL_OFFSET(zl)) > size - ZIPLIST_END_SIZE) + return 0; + + if (!deep) + return 1; + + unsigned int count = 0; + unsigned char *p = ZIPLIST_ENTRY_HEAD(zl); + unsigned char *prev = NULL; + size_t prev_raw_size = 0; + while(*p != ZIP_END) { + struct zlentry e; + /* Decode the entry headers and fail if invalid or reaches outside the allocation */ + if (!zipEntrySafe(zl, size, p, &e, 1)) + return 0; + + /* Make sure the record stating the prev entry size is correct. */ + if (e.prevrawlen != prev_raw_size) + return 0; + + /* Move to the next entry */ + prev_raw_size = e.headersize + e.len; + prev = p; + p += e.headersize + e.len; + count++; + } + + /* Make sure the entry really do point to the start of the last entry. */ + if (prev != ZIPLIST_ENTRY_TAIL(zl)) + return 0; + + /* Check that the count in the header is correct */ + unsigned int header_count = intrev16ifbe(ZIPLIST_LENGTH(zl)); + if (header_count != UINT16_MAX && count != header_count) + return 0; + + return 1; +} + #ifdef REDIS_TEST #include #include "adlist.h" @@ -1784,6 +1962,7 @@ int ziplistTest(int argc, char **argv) { printf("Create long list and check indices:\n"); { + unsigned long long start = usec(); zl = ziplistNew(); char buf[32]; int i,len; @@ -1800,7 +1979,7 @@ int ziplistTest(int argc, char **argv) { assert(ziplistGet(p,NULL,NULL,&value)); assert(999-i == value); } - printf("SUCCESS\n\n"); + printf("SUCCESS. usec=%lld\n\n", usec()-start); zfree(zl); } @@ -1907,6 +2086,7 @@ int ziplistTest(int argc, char **argv) { printf("Stress with random payloads of different encoding:\n"); { + unsigned long long start = usec(); int i,j,len,where; unsigned char *p; char buf[1024]; @@ -1979,13 +2159,15 @@ int ziplistTest(int argc, char **argv) { zfree(zl); listRelease(ref); } - printf("SUCCESS\n\n"); + printf("Done. usec=%lld\n\n", usec()-start); } printf("Stress with variable ziplist size:\n"); { + unsigned long long start = usec(); stress(ZIPLIST_HEAD,100000,16384,256); stress(ZIPLIST_TAIL,100000,16384,256); + printf("Done. usec=%lld\n\n", usec()-start); } printf("Stress __ziplistCascadeUpdate:\n"); diff --git a/src/ziplist.h b/src/ziplist.h index 964a47f6d..8d1ac1691 100644 --- a/src/ziplist.h +++ b/src/ziplist.h @@ -45,10 +45,11 @@ unsigned char *ziplistInsert(unsigned char *zl, unsigned char *p, unsigned char unsigned char *ziplistDelete(unsigned char *zl, unsigned char **p); unsigned char *ziplistDeleteRange(unsigned char *zl, int index, unsigned int num); unsigned int ziplistCompare(unsigned char *p, unsigned char *s, unsigned int slen); -unsigned char *ziplistFind(unsigned char *p, unsigned char *vstr, unsigned int vlen, unsigned int skip); +unsigned char *ziplistFind(unsigned char *zl, unsigned char *p, unsigned char *vstr, unsigned int vlen, unsigned int skip); 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); #ifdef REDIS_TEST int ziplistTest(int argc, char *argv[]); diff --git a/src/zipmap.c b/src/zipmap.c index 365c4aea4..bd41fe6a5 100644 --- a/src/zipmap.c +++ b/src/zipmap.c @@ -111,6 +111,10 @@ static unsigned int zipmapDecodeLength(unsigned char *p) { return len; } +static unsigned int zipmapGetEncodedLengthSize(unsigned char *p) { + return (*p < ZIPMAP_BIGLEN) ? 1: 5; +} + /* Encode the length 'l' writing it in 'p'. If p is NULL it just returns * the amount of bytes required to encode such a length. */ static unsigned int zipmapEncodeLength(unsigned char *p, unsigned int len) { @@ -370,6 +374,70 @@ size_t zipmapBlobLen(unsigned char *zm) { return totlen; } +/* 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 zipmapValidateIntegrity(unsigned char *zm, size_t size, int deep) { +#define OUT_OF_RANGE(p) ( \ + (p) < zm + 2 || \ + (p) > zm + size - 1) + unsigned int l, s, e; + + /* check that we can actually read the header (or ZIPMAP_END). */ + if (size < 2) + return 0; + + /* the last byte must be the terminator. */ + if (zm[size-1] != ZIPMAP_END) + return 0; + + if (!deep) + return 1; + + unsigned int count = 0; + unsigned char *p = zm + 1; /* skip the count */ + while(*p != ZIPMAP_END) { + /* read the field name length encoding type */ + s = zipmapGetEncodedLengthSize(p); + /* make sure the entry length doesn't rech outside the edge of the zipmap */ + if (OUT_OF_RANGE(p+s)) + return 0; + + /* read the field name length */ + l = zipmapDecodeLength(p); + p += s; /* skip the encoded field size */ + p += l; /* skip the field */ + + /* make sure the entry doesn't rech outside the edge of the zipmap */ + if (OUT_OF_RANGE(p)) + return 0; + + /* read the value length encoding type */ + s = zipmapGetEncodedLengthSize(p); + /* make sure the entry length doesn't rech outside the edge of the zipmap */ + if (OUT_OF_RANGE(p+s)) + return 0; + + /* read the value length */ + l = zipmapDecodeLength(p); + p += s; /* skip the encoded value size*/ + e = *p++; /* skip the encoded free space (always encoded in one byte) */ + p += l+e; /* skip the value and free space */ + count++; + + /* make sure the entry doesn't rech outside the edge of the zipmap */ + if (OUT_OF_RANGE(p)) + return 0; + } + + /* check that the count in the header is correct */ + if (zm[0] != ZIPMAP_BIGLEN && zm[0] != count) + return 0; + + return 1; +#undef OUT_OF_RANGE +} + #ifdef REDIS_TEST static void zipmapRepr(unsigned char *p) { unsigned int l; diff --git a/src/zipmap.h b/src/zipmap.h index ac588f05a..daf8430a0 100644 --- a/src/zipmap.h +++ b/src/zipmap.h @@ -45,6 +45,7 @@ int zipmapExists(unsigned char *zm, unsigned char *key, unsigned int klen); unsigned int zipmapLen(unsigned char *zm); size_t zipmapBlobLen(unsigned char *zm); void zipmapRepr(unsigned char *p); +int zipmapValidateIntegrity(unsigned char *zm, size_t size, int deep); #ifdef REDIS_TEST int zipmapTest(int argc, char *argv[]); diff --git a/tests/assets/corrupt_ziplist.rdb b/tests/assets/corrupt_ziplist.rdb new file mode 100644 index 0000000000000000000000000000000000000000..b40ada8d634d094ba2f20d728f20b19596e3e7ef GIT binary patch literal 1415 zcmaJ>yH3ME5WE!T6@e)D2D(_g=jTyTKtT%yJ;@ao7$n<4$tUnlG*No~f$uO9#!1cy z*|V&CI%%|?ot<6X?e*2o9T6q~& zFfZ+FlG}X!`94

MK_2^wBOaS~?d==q)zU!&yf={A4d*K4oP!@_nV$d3(I}+y1iu z&()~2>C61Nvc;cTI$n)>btn+(lX1ELPr^eUtzCYvzbM9d*I!=`yS@i_ig{uQPjG&y zy{3)DaZjgVn9o|!Pgo}tVIIahnXzxo0py_+*QFHeClYX>E=5=;8Qd3{ZcloCh5}D2 z%q77(nK4c&o(mJkN%B);{rtPnVE+R>+<$}TNQr=_xGrg--&n|J!OsZiGYkI2!oEp< bM#3J39Y65RY~Brk>u~62;CyS^7jGLs8LoRE literal 0 HcmV?d00001 diff --git a/tests/cluster/tests/04-resharding.tcl b/tests/cluster/tests/04-resharding.tcl index cee2ec5ba..1dcdb5a2c 100644 --- a/tests/cluster/tests/04-resharding.tcl +++ b/tests/cluster/tests/04-resharding.tcl @@ -172,3 +172,10 @@ test "Verify slaves consistency" { } assert {$verified_masters >= 5} } + +test "Dump sanitization was skipped for migrations" { + set verified_masters 0 + foreach_redis_id id { + assert {[RI $id dump_payload_sanitizations] == 0} + } +} diff --git a/tests/integration/corrupt-dump.tcl b/tests/integration/corrupt-dump.tcl new file mode 100644 index 000000000..9864036d2 --- /dev/null +++ b/tests/integration/corrupt-dump.tcl @@ -0,0 +1,178 @@ +# tests of corrupt ziplist payload with valid CRC + +tags {"dump" "corruption"} { + +set corrupt_payload_7445 "\x0E\x01\x1D\x1D\x00\x00\x00\x16\x00\x00\x00\x03\x00\x00\x04\x43\x43\x43\x43\x06\x04\x42\x42\x42\x42\x06\x3F\x41\x41\x41\x41\xFF\x09\x00\x88\xA5\xCA\xA8\xC5\x41\xF4\x35" + +test {corrupt payload: #7445 - with sanitize} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload yes + catch { + r restore key 0 $corrupt_payload_7445 + } err + assert_match "*Bad data format*" $err + verify_log_message 0 "*Ziplist integrity check failed*" 0 + } +} + +test {corrupt payload: #7445 - without sanitize - 1} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload no + r config set crash-memcheck-enabled no ;# avoid valgrind issues + r restore key 0 $corrupt_payload_7445 + catch {r lindex key 2} + verify_log_message 0 "*ASSERTION FAILED*" 0 + } +} + +test {corrupt payload: #7445 - without sanitize - 2} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload no + r config set crash-memcheck-enabled no ;# avoid valgrind issues + r restore key 0 $corrupt_payload_7445 + catch {r lset key 2 "BEEF"} + verify_log_message 0 "*ASSERTION FAILED*" 0 + } +} + +test {corrupt payload: hash with valid zip list header, invalid entry len} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r restore key 0 "\x0D\x1B\x1B\x00\x00\x00\x16\x00\x00\x00\x04\x00\x00\x02\x61\x00\x04\x02\x62\x00\x04\x14\x63\x00\x04\x02\x64\x00\xFF\x09\x00\xD9\x10\x54\x92\x15\xF5\x5F\x52" + r config set crash-memcheck-enabled no ;# avoid valgrind issues + r config set hash-max-ziplist-entries 1 + catch {r hset key b b} + verify_log_message 0 "*zipEntrySafe*" 0 + } +} + +test {corrupt payload: invalid zlbytes header} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + catch { + r restore key 0 "\x0D\x1B\x25\x00\x00\x00\x16\x00\x00\x00\x04\x00\x00\x02\x61\x00\x04\x02\x62\x00\x04\x02\x63\x00\x04\x02\x64\x00\xFF\x09\x00\xB7\xF7\x6E\x9F\x43\x43\x14\xC6" + } err + assert_match "*Bad data format*" $err + } +} + +test {corrupt payload: valid zipped hash header, dup records} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r restore key 0 "\x0D\x1B\x1B\x00\x00\x00\x16\x00\x00\x00\x04\x00\x00\x02\x61\x00\x04\x02\x62\x00\x04\x02\x61\x00\x04\x02\x64\x00\xFF\x09\x00\xA1\x98\x36\x78\xCC\x8E\x93\x2E" + r config set crash-memcheck-enabled no ;# avoid valgrind issues + r config set hash-max-ziplist-entries 1 + # cause an assertion when converting to hash table + catch {r hset key b b} + verify_log_message 0 "*ziplist with dup elements dump*" 0 + } +} + +test {corrupt payload: quicklist big ziplist prev len} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r restore key 0 "\x0E\x01\x13\x13\x00\x00\x00\x0E\x00\x00\x00\x02\x00\x00\x02\x61\x00\x0E\x02\x62\x00\xFF\x09\x00\x49\x97\x30\xB2\x0D\xA1\xED\xAA" + r config set crash-memcheck-enabled no ;# avoid valgrind issues + catch {r lindex key -2} + verify_log_message 0 "*ASSERTION FAILED*" 0 + } +} + +test {corrupt payload: quicklist small ziplist prev len} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload yes + catch { + 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 + } +} + +test {corrupt payload: quicklist ziplist wrong count} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r restore key 0 "\x0E\x01\x13\x13\x00\x00\x00\x0E\x00\x00\x00\x03\x00\x00\x02\x61\x00\x04\x02\x62\x00\xFF\x09\x00\x4D\xE2\x0A\x2F\x08\x25\xDF\x91" + r lpush key a + # check that the server didn't crash + r ping + } +} + +test {corrupt payload: #3080 - quicklist} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + catch { + r RESTORE key 0 "\x0E\x01\x80\x00\x00\x00\x10\x41\x41\x41\x41\x41\x41\x41\x41\x02\x00\x00\x80\x41\x41\x41\x41\x07\x00\x03\xC7\x1D\xEF\x54\x68\xCC\xF3" + r DUMP key + } + assert_match "*Bad data format*" $err + verify_log_message 0 "*Ziplist integrity check failed*" 0 + } +} + +test {corrupt payload: #3080 - ziplist} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + catch { + 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" + } + assert_match "*Bad data format*" $err + verify_log_message 0 "*Ziplist integrity check failed*" 0 + } +} + +test {corrupt payload: load corrupted rdb with no CRC - #3505} { + set server_path [tmpdir "server.rdb-corruption-test"] + exec cp tests/assets/corrupt_ziplist.rdb $server_path + set srv [start_server [list overrides [list "dir" $server_path "dbfilename" "corrupt_ziplist.rdb" loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no]]] + + # wait for termination + wait_for_condition 100 50 { + ! [is_alive $srv] + } else { + fail "rdb loading didn't fail" + } + + 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"] + kill_server $srv ;# let valgrind look for issues +} + +test {corrupt payload: listpack invalid size header} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + catch { + r restore key 0 "\x0F\x01\x10\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x40\x55\x5F\x00\x00\x00\x0F\x00\x01\x01\x00\x01\x02\x01\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x00\x01\x00\x01\x00\x01\x00\x01\x02\x02\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x61\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x88\x62\x00\x00\x00\x00\x00\x00\x00\x09\x08\x01\xFF\x0A\x01\x00\x00\x09\x00\x45\x91\x0A\x87\x2F\xA5\xF9\x2E" + } err + assert_match "*Bad data format*" $err + verify_log_message 0 "*Stream listpack integrity check failed*" 0 + } +} + +test {corrupt payload: listpack too long entry len} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r restore key 0 "\x0F\x01\x10\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x40\x55\x55\x00\x00\x00\x0F\x00\x01\x01\x00\x01\x02\x01\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x00\x01\x00\x01\x00\x01\x00\x01\x02\x02\x89\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x61\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x88\x62\x00\x00\x00\x00\x00\x00\x00\x09\x08\x01\xFF\x0A\x01\x00\x00\x09\x00\x40\x63\xC9\x37\x03\xA2\xE5\x68" + catch { + r xinfo stream key full + } err + verify_log_message 0 "*ASSERTION FAILED*" 0 + } +} + +test {corrupt payload: listpack very long entry len} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r restore key 0 "\x0F\x01\x10\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x40\x55\x55\x00\x00\x00\x0F\x00\x01\x01\x00\x01\x02\x01\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x00\x01\x00\x01\x00\x01\x00\x01\x02\x02\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x61\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x9C\x62\x00\x00\x00\x00\x00\x00\x00\x09\x08\x01\xFF\x0A\x01\x00\x00\x09\x00\x63\x6F\x42\x8E\x7C\xB5\xA2\x9D" + catch { + r xinfo stream key full + } err + verify_log_message 0 "*ASSERTION FAILED*" 0 + } +} + +test {corrupt payload: listpack too long entry prev len} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload yes + catch { + r restore key 0 "\x0F\x01\x10\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x40\x55\x55\x00\x00\x00\x0F\x00\x01\x01\x00\x15\x02\x01\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x00\x01\x00\x01\x00\x01\x00\x01\x02\x02\x88\x31\x00\x00\x00\x00\x00\x00\x00\x09\x88\x61\x00\x00\x00\x00\x00\x00\x00\x09\x88\x32\x00\x00\x00\x00\x00\x00\x00\x09\x88\x62\x00\x00\x00\x00\x00\x00\x00\x09\x08\x01\xFF\x0A\x01\x00\x00\x09\x00\x06\xFB\x44\x24\x0A\x8E\x75\xEA" + } err + assert_match "*Bad data format*" $err + verify_log_message 0 "*Stream listpack integrity check failed*" 0 + } +} + +} ;# tags + diff --git a/tests/support/util.tcl b/tests/support/util.tcl index 3268bc974..aadec7cd1 100644 --- a/tests/support/util.tcl +++ b/tests/support/util.tcl @@ -112,6 +112,22 @@ proc count_log_lines {srv_idx} { set _ [exec wc -l < [srv $srv_idx stdout]] } +# returns the number of times a line with that pattern appears in a file +proc count_message_lines {file pattern} { + set res 0 + # exec fails when grep exists with status other than 0 (when the patter wasn't found) + catch { + set res [exec grep $pattern $file 2> /dev/null | wc -l] + } + return $res +} + +# returns the number of times a line with that pattern appears in the log +proc count_log_message {srv_idx pattern} { + set stdout [srv $srv_idx stdout] + return [count_message_lines $stdout $pattern] +} + # verify pattern exists in server's sdtout after a certain line number proc verify_log_message {srv_idx pattern from_line} { incr from_line diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 6a38cc001..998e82bd9 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -44,6 +44,7 @@ set ::all_tests { integration/replication-psync integration/aof integration/rdb + integration/corrupt-dump integration/convert-zipmap-hash-on-load integration/logging integration/psync2