Harden redis parser for unauthenticated clients

CHANGELOG.md

 # Changelog
 
+## Unreleased
+
+### Improvements
+- Security hardening of the redis parser for unauthenticated clients.
+
 ## 0.4.1 (2020-01-17)
 
 ### Bug fixes

src/BufferedReader.cc

@@ -99,12 +99,10 @@ LinkStatus BufferedReader::consumeInternal(size_t len, std::string &str) {
     remaining -= available_bytes;
 
     // add them
-    qdb_debug("Appending " << available_bytes << " bytes to str");
     str.append(buffers.front()->data() + position_read, available_bytes);
     position_read += available_bytes;
 
     if(position_read >= buffer_size) {
-      qdb_debug("An entire buffer has been consumed, releasing");
       // an entire buffer has been consumed
       buffers.pop_front();
       position_read = 0;

src/Connection.cc

@@ -283,7 +283,7 @@ LinkStatus Connection::processRequests(Dispatcher *dispatcher, const InFlightTra
       qdb_throw("should never reach here");
     }
 
-    LinkStatus status = parser.fetch(currentRequest);
+    LinkStatus status = parser.fetch(currentRequest, authorization);
     InternalFilter::process(currentRequest);
 
     if(status < 0) {

src/RedisParser.cc

@@ -46,7 +46,7 @@ int RedisParser::purge() {
   }
 }
 
-int RedisParser::fetch(RedisRequest &req, bool allowZeroSizedStrings) {
+int RedisParser::fetch(RedisRequest &req, bool authenticated) {
   if(request_size == 0) {
     req.clear();
 
@@ -57,23 +57,21 @@ int RedisParser::fetch(RedisRequest &req, bool allowZeroSizedStrings) {
     element_size = -1;
     current_element = 0;
 
+    if(!authenticated && request_size >= 5) {
+      qdb_warn("Unauthenticated client attempted to send request with " << request_size << " elements - shutting the connection down");
+      return -2;
+    }
+
     req.resize(request_size);
-    qdb_debug("Received size of redis request: " << request_size);
   }
 
   for( ; current_element < request_size; current_element++) {
-    int rc = readElement(req.getPinnedBuffer(current_element));
+    int rc = readElement(req.getPinnedBuffer(current_element), authenticated);
     if(rc <= 0) return rc;
     element_size = -1;
   }
 
   request_size = 0;
-
-  qdb_debug("Parsed redis request successfully.");
-  for(size_t i = 0; i < req.size(); i++) {
-    qdb_debug(req[i]);
-  }
-
   req.parseCommand();
 
   if(encounteredZeroSize) {
@@ -92,9 +90,6 @@ int RedisParser::readInteger(char prefix, int &retval) {
     if(rlen <= 0) return rlen;
 
     current_integer.append(prev);
-
-    qdb_debug("Received byte: " << prev << "'" << " " << (int)prev[0]);
-    qdb_debug("current_integer: " << quotes(current_integer));
   }
 
   if(current_integer[0] != prefix) {
@@ -121,13 +116,18 @@ int RedisParser::readInteger(char prefix, int &retval) {
   return 1; // success
 }
 
-int RedisParser::readElement(PinnedBuffer &str) {
-  qdb_debug("Element size: " << element_size);
+int RedisParser::readElement(PinnedBuffer &str, bool authenticated) {
   if(element_size == -1) {
     int retcode = readInteger('$', element_size);
     if(retcode <= 0) return retcode;
     if(element_size == 0) encounteredZeroSize = true;
   }
+
+  if(!authenticated && element_size >= 1048576) {
+    qdb_warn("Unauthenticated client attempted to send request containing element with " << element_size << " bytes - shutting the connection down");
+    return -2;
+  }
+
   return readString(element_size, str);
 }
 
@@ -146,6 +146,5 @@ int RedisParser::readString(int nbytes, PinnedBuffer &str) {
   }
 
   str.remove_suffix(2);
-  qdb_debug("Got string: " << str.sv());
   return rlen;
 }

src/RedisParser.hh

@@ -44,7 +44,7 @@ public:
   // returns negative on error
   //----------------------------------------------------------------------------
 
-  LinkStatus fetch(RedisRequest &req, bool allowZeroSizedStrings = true);
+  LinkStatus fetch(RedisRequest &req, bool authenticated);
 
   //----------------------------------------------------------------------------
   // Purge any and all incoming data.
@@ -86,7 +86,7 @@ private:
   //----------------------------------------------------------------------------
 
   int readInteger(char prefix, int& retval);
-  int readElement(PinnedBuffer &buff);
+  int readElement(PinnedBuffer &buff, bool authenticated);
   int readString(int nbytes, PinnedBuffer &buff);
 
 };

test/e2e.cc

@@ -2447,3 +2447,43 @@ TEST_F(Raft_e2e, SharedHash) {
   ASSERT_TRUE(hash2.get("k1", tmp));
   ASSERT_EQ(tmp, "v1");
 }
+
+TEST_F(Raft_e2e, NoAuth) {
+  spinup(0); spinup(1); spinup(2);
+  RETRY_ASSERT_TRUE(checkStateConsensus(0, 1, 2));
+  int leaderID = getLeaderID();
+
+  // de-auth
+  ASSERT_REPLY_DESCRIBE(tunnel(leaderID)->exec("auth", "aaaa").get(),
+    "(error) ERR invalid password");
+
+  // try command with too many arguments for un-authenticated clients
+  ASSERT_EQ(tunnel(leaderID)->exec("1", "2", "3", "4", "5", "6", "7", "8", "9", "10").get(),
+    nullptr);
+
+  // re-connect, re-auth
+  ASSERT_REPLY_DESCRIBE(tunnel(leaderID)->exec("ping").get(),
+    "PONG");
+
+  // try command again
+  ASSERT_REPLY_DESCRIBE(tunnel(leaderID)->exec("1", "2", "3", "4", "5", "6", "7", "8", "9", "10").get(),
+    "(error) ERR unknown command '1'");
+
+  // de-auth
+  ASSERT_REPLY_DESCRIBE(tunnel(leaderID)->exec("auth", "aaaa").get(),
+    "(error) ERR invalid password");
+
+  // try command with huge payload, 1 MB
+  std::string payload(1048577, 'a');
+
+  ASSERT_EQ(tunnel(leaderID)->exec("SET", "key", payload).get(),
+    nullptr);
+
+  // re-connect, re-auth
+  ASSERT_REPLY_DESCRIBE(tunnel(leaderID)->exec("ping").get(),
+    "PONG");
+
+  // ensure huge payload now succeeds
+  ASSERT_REPLY_DESCRIBE(tunnel(leaderID)->exec("SET", "key", payload).get(),
+    "OK");
+}

test/qclient.cc

@@ -198,17 +198,17 @@ TEST(QClient, T3) {
 
     RedisRequest incoming;
     for(size_t i = 0; i < 10; i++) {
-      RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming), 1);
+      RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming, true), 1);
       ASSERT_EQ(incoming, make_req("PING", std::to_string(i)));
       link.Send(SSTR("+" << i << "\r\n"));
     }
 
-    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming), 1);
+    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming, true), 1);
     ASSERT_EQ(incoming, req1);
     link.Send("+OK\r\n");
     ASSERT_REPLY(fut1, "OK");
 
-    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming), 1);
+    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming, true), 1);
     ASSERT_EQ(incoming, req2);
     link.Send("+ZZZ\r\n");
     ASSERT_REPLY(fut2, "ZZZ");
@@ -239,16 +239,16 @@ TEST(QClient, AuthHandshake) {
     std::future<redisReplyPtr> fut2 = tunnel.execute(req2);
 
     RedisRequest incoming;
-    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming), 1);
+    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming, true), 1);
     ASSERT_EQ(incoming, make_req("AUTH", "hunter2"));
     link.Send("+OK\r\n");
 
-    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming), 1);
+    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming, true), 1);
     ASSERT_EQ(incoming, req1);
     link.Send("+OK\r\n");
     ASSERT_REPLY(fut1, "OK");
 
-    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming), 1);
+    RETRY_ASSERT_EQ_SPIN(parser.fetch(incoming, true), 1);
     ASSERT_EQ(incoming, req2);
     link.Send("+ZZZ\r\n");
     ASSERT_REPLY(fut2, "ZZZ");

test/raft-talker.cc

@@ -51,7 +51,7 @@ TEST(RaftTalker, T1) {
   RedisRequest req;
 
   int rc;
-  while( (rc = parser.fetch(req)) == 0) ;
+  while( (rc = parser.fetch(req, true)) == 0) ;
   ASSERT_EQ(rc, 1);
 
   RedisRequest tmp = {"RAFT_HANDSHAKE", VERSION_FULL_STRING, clusterID, timeouts.toString()};
@@ -72,14 +72,14 @@ TEST(RaftTalker, T1) {
                3, // commit index
                entries // payload
              );
-  while( (rc = parser.fetch(req)) == 0) ;
+  while( (rc = parser.fetch(req, true)) == 0) ;
   ASSERT_EQ(rc, 1);
 
   tmp = {"CLIENT", "SETNAME", "some-client-name"};
   ASSERT_EQ(req, tmp);
   link.Send("+OK\r\n");
 
-  while( (rc = parser.fetch(req)) == 0) ;
+  while( (rc = parser.fetch(req, true)) == 0) ;
   ASSERT_EQ(rc, 1);
 
   tmp = {"QUARKDB_VERSION"};
@@ -88,7 +88,7 @@ TEST(RaftTalker, T1) {
 
   while( talker.getNodeVersion() != "1.33.7" ) ;
 
-  while( (rc = parser.fetch(req)) == 0) ;
+  while( (rc = parser.fetch(req, true)) == 0) ;
   ASSERT_EQ(rc, 1);
   tmp = {"RAFT_APPEND_ENTRIES", "its_me_ur_leader:1337",
          intToBinaryString(12) + intToBinaryString(7) + intToBinaryString(11) +

test/redis-parser.cc

@@ -41,25 +41,25 @@ protected:
   void simulateBadConnection(const std::string &str, size_t bytes) {
     size_t i = 0;
     while(i + bytes < str.size()) {
-      ASSERT_EQ(parser->fetch(request), 0);
+      ASSERT_EQ(parser->fetch(request, true), 0);
       link.Send(str.c_str()+i, bytes);
 
       i += bytes;
     }
 
     // send any left-over bytes
-    ASSERT_EQ(parser->fetch(request), 0);
+    ASSERT_EQ(parser->fetch(request, true), 0);
     link.Send(str.c_str()+i, str.size()-i);
   }
 
   void simulateMany(const std::string &str, RedisRequest &valid, size_t bytes) {
     for(size_t i = 1; i < bytes; i++) {
-      ASSERT_EQ(parser->fetch(request), 0);
+      ASSERT_EQ(parser->fetch(request, true), 0);
       simulateBadConnection(str, i);
-      ASSERT_EQ(parser->fetch(request), 1);
+      ASSERT_EQ(parser->fetch(request, true), 1);
       ASSERT_EQ(request, valid);
     }
-    ASSERT_EQ(parser->fetch(request), 0);
+    ASSERT_EQ(parser->fetch(request, true), 0);
   }
 
   Link link;
@@ -69,16 +69,16 @@ protected:
 
 
 TEST_F(Redis_Parser, T1) {
-  ASSERT_EQ(parser->fetch(request), 0);
+  ASSERT_EQ(parser->fetch(request, true), 0);
   link.Send("*2\r\n$3\r\nget\r\n$3\r\nabc\r\n*3\r\n$3\r\nset\r\n$3\r\nabc\r\n$5\r\nhello\r\n");
-  ASSERT_EQ(parser->fetch(request), 1);
+  ASSERT_EQ(parser->fetch(request, true), 1);
   ASSERT_EQ(request.size(), (size_t) 2);
   ASSERT_EQ(request[0], "get");
   ASSERT_EQ(request[1], "abc");
 
   request.clear();
 
-  ASSERT_EQ(parser->fetch(request), 1);
+  ASSERT_EQ(parser->fetch(request, true), 1);
   ASSERT_EQ(request.size(), (size_t) 3);
   ASSERT_EQ(request[0], "set");
   ASSERT_EQ(request[1], "abc");
@@ -86,9 +86,9 @@ TEST_F(Redis_Parser, T1) {
 }
 
 TEST_F(Redis_Parser, ZeroSizedString) {
-  ASSERT_EQ(parser->fetch(request), 0);
+  ASSERT_EQ(parser->fetch(request, true), 0);
   link.Send("*3\r\n$3\r\nset\r\n$0\r\n\r\n$3\r\nabc\r\n");
-  ASSERT_EQ(parser->fetch(request), -2);
+  ASSERT_EQ(parser->fetch(request, true), -2);
 }
 
 TEST_F(Redis_Parser, T2) {
@@ -123,34 +123,34 @@ TEST(RedisRequest, TransactionToPrintableString) {
 TEST_F(Redis_Parser, T3) {
   // test bogus data
   link.Send("hello there\r\n");
-  ASSERT_LT(parser->fetch(request), 0);
+  ASSERT_LT(parser->fetch(request, true), 0);
 }
 
 TEST_F(Redis_Parser, T4) {
   // bad integer
   link.Send("*lol\r\n");
-  ASSERT_LT(parser->fetch(request), 0);
+  ASSERT_LT(parser->fetch(request, true), 0);
 }
 
 TEST_F(Redis_Parser, T5) {
   // wrong string size
   link.Send("*1\r\n$5\r\naaa\r\n");
-  ASSERT_LE(parser->fetch(request), 0);
+  ASSERT_LE(parser->fetch(request, true), 0);
 }
 
 TEST_F(Redis_Parser, T6) {
   // bad string length integer
   link.Send("*1\r\n$asdf\r\n");
-  ASSERT_LT(parser->fetch(request), 0);
+  ASSERT_LT(parser->fetch(request, true), 0);
 }
 
 TEST_F(Redis_Parser, T7) {
   // corrupted \r\n
   link.Send("*1\r\n$3abc\n\n");
-  ASSERT_LT(parser->fetch(request), 0);
+  ASSERT_LT(parser->fetch(request, true), 0);
 }
 
 TEST_F(Redis_Parser, T8) {
   link.Send("*1\n\nabc");
-  ASSERT_LT(parser->fetch(request), 0);
+  ASSERT_LT(parser->fetch(request, true), 0);
 }