Security fixes (#373)

Fixing potential buffer overflows when using the bitop shift and cron commands, as well as comparing ACL user passwords in constant time.

Former-commit-id: 3c716ee33d2d14d517271b674dd57c8328d14522

src/acl.cpp

@@ -223,12 +223,18 @@ uint64_t ACLGetCommandCategoryFlagByName(const char *name) {
     return 0; /* No match. */
 }
 
-/* Method for passwords/pattern comparison used for the user->passwords list
+/* Method for pattern comparison used for the user->patterns list
  * so that we can search for items with listSearchKey(). */
 int ACLListMatchSds(void *a, void *b) {
     return sdscmp((sds)a,(sds)b) == 0;
 }
 
+/* Method for password comparison used for the user->passwords list
+ * Like the above, but uses a time independant compare for security reasons */
+int ACLListMatchSdsSecure(void *a, void* b) {
+    return time_independent_strcmp((sds)a,(sds)b) == 0;
+}
+
 /* Method to free list elements from ACL users password/patterns lists. */
 void ACLListFreeSds(const void *item) {
     sdsfree((sds)item);
@@ -253,7 +259,7 @@ user *ACLCreateUser(const char *name, size_t namelen) {
     u->passwords = listCreate();
     u->patterns = listCreate();
     u->channels = listCreate();
-    listSetMatchMethod(u->passwords,ACLListMatchSds);
+    listSetMatchMethod(u->passwords,ACLListMatchSdsSecure);
     listSetFreeMethod(u->passwords,ACLListFreeSds);
     listSetDupMethod(u->passwords,ACLListDupSds);
     listSetMatchMethod(u->patterns,ACLListMatchSds);

src/bitops.cpp

@@ -405,6 +405,10 @@ void printBits(unsigned char *p, unsigned long count) {
 #define BITFIELDOP_SET 1
 #define BITFIELDOP_INCRBY 2
 
+/* Make sure we don't bit shift too many bits at a time.
+ * Even this amount is probably way too large. */
+#define BITOP_SHIFT_MAX (1<<30)
+
 /* This helper function used by GETBIT / SETBIT parses the bit offset argument
  * making sure an error is returned if it is negative or if it overflows
  * Redis 512 MB limit for the string value or more (server.proto_max_bulk_len).
@@ -642,6 +646,11 @@ void bitopCommand(client *c) {
             addReplyError(c, "BITOP SHIFT's last parameter must be an integer");
             return;
         }
+        if (shift > BITOP_SHIFT_MAX) {
+            addReplyError(c, "BITOP SHIFT value is too large.");
+            return;
+        }
+
         if (op == BITOP_RSHIFT)
             shift = -shift;
     }
@@ -682,7 +691,7 @@ void bitopCommand(client *c) {
 
     if (fShiftOp)
     {
-        long newlen = (long)maxlen + shift/CHAR_BIT;
+        long long newlen = (long long)maxlen + shift/CHAR_BIT;
         if (shift > 0 && (shift % CHAR_BIT) != 0)
             newlen++;
 
@@ -694,14 +703,14 @@ void bitopCommand(client *c) {
             res = (unsigned char*) sdsnewlen(NULL,newlen);
             if (shift >= 0)
             {   // left shift
-                long byteoffset = shift/CHAR_BIT;
+                long long byteoffset = shift/CHAR_BIT;
                 memset(res, 0, byteoffset);
-                long srcLen = newlen - byteoffset - ((shift % CHAR_BIT) ? 1 : 0);
+                long long srcLen = newlen - byteoffset - ((shift % CHAR_BIT) ? 1 : 0);
 
                 // now the bitshift+copy
                 unsigned bitshift = shift % CHAR_BIT;
                 unsigned char carry = 0;
-                for (long iSrc = 0; iSrc < srcLen; ++iSrc)
+                for (long long iSrc = 0; iSrc < srcLen; ++iSrc)
                 {
                     res[byteoffset+iSrc] = (src[0][iSrc] << bitshift) | carry;
                     carry = src[0][iSrc] >> (CHAR_BIT - bitshift);
@@ -711,14 +720,14 @@ void bitopCommand(client *c) {
             } 
             else 
             {   // right shift
-                long byteoffset = -shift/CHAR_BIT;
+                long long byteoffset = -shift/CHAR_BIT;
                 unsigned bitshift = -shift % CHAR_BIT;
                 if (bitshift)
                     ++byteoffset;
                 res[0] = (src[0][byteoffset] << (CHAR_BIT-bitshift));
                 if (byteoffset > 0)
                     res[0] |= (src[0][byteoffset-1] >> bitshift);
-                for (long idx = 1; idx < newlen; ++idx)
+                for (long long idx = 1; idx < newlen; ++idx)
                 {
                     res[idx] = (src[0][byteoffset+idx] << (CHAR_BIT-bitshift)) | (src[0][byteoffset+idx-1] >> bitshift);
                 }

src/cron.cpp

@@ -51,6 +51,7 @@ void cronCommand(client *c)
         
         if (c->argc < (6 + numkeys)) {
             addReplyError(c, "Missing arguments or numkeys is too big");
+            return;
         }
     }