Internally use HMAC in http auth

To at least get constant time compare.
Also fix incorrect parsing of the creds (were incorrectly stripped).
Also add unit tests.

src/HttpServer.cc

@@ -42,6 +42,7 @@
 #include "DlAbortEx.h"
 #include "message.h"
 #include "util.h"
+#include "util_security.h"
 #include "LogFactory.h"
 #include "Logger.h"
 #include "base64.h"
@@ -57,6 +58,8 @@
 
 namespace aria2 {
 
+std::unique_ptr<util::security::HMAC> HttpServer::hmac_;
+
 HttpServer::HttpServer(const std::shared_ptr<SocketCore>& socket)
  : socket_(socket),
    socketRecvBuffer_(new SocketRecvBuffer(socket_)),
@@ -279,7 +282,7 @@ bool HttpServer::sendBufferIsEmpty() const
 
 bool HttpServer::authenticate()
 {
-  if(username_.empty()) {
+  if (!username_) {
     return true;
   }
 
@@ -292,19 +295,37 @@ bool HttpServer::authenticate()
   if(!util::streq(p.first.first, p.first.second, "Basic")) {
     return false;
   }
+
   std::string userpass = base64::decode(p.second.first, p.second.second);
-  auto up = util::divide(std::begin(userpass), std::end(userpass), ':');
+  auto up = util::divide(std::begin(userpass), std::end(userpass), ':', false);
-  return util::streq(up.first.first, up.first.second,
+  std::string username(up.first.first, up.first.second);
-                     username_.begin(), username_.end()) &&
+  std::string password(up.second.first, up.second.second);
-    util::streq(up.second.first, up.second.second,
+  return *username_ == hmac_->getResult(username) &&
-                password_.begin(), password_.end());
+    (!password_ || *password_ == hmac_->getResult(password));
 }
 
 void HttpServer::setUsernamePassword
 (const std::string& username, const std::string& password)
 {
-  username_ = username;
+  using namespace util::security;
-  password_ = password;
+
+  if (!hmac_) {
+    hmac_ = HMAC::createRandom();
+  }
+
+  if (!username.empty()) {
+    username_ = make_unique<HMACResult>(hmac_->getResult(username));
+  }
+  else {
+    username_.reset();
+  }
+
+  if (!password.empty()) {
+    password_ = make_unique<HMACResult>(hmac_->getResult(password));
+  }
+  else {
+    password_.reset();
+  }
 }
 
 int HttpServer::setupResponseRecv()

src/HttpServer.h

@@ -53,6 +53,13 @@ class DownloadEngine;
 class SocketRecvBuffer;
 class DiskWriter;
 
+namespace util {
+  namespace security {
+    class HMAC;
+    class HMACResult;
+  }
+}
+
 enum RequestType {
   RPC_TYPE_NONE,
   RPC_TYPE_XML,
@@ -64,6 +71,8 @@ enum RequestType {
 // intended to be a generic HTTP server.
 class HttpServer {
 private:
+  static std::unique_ptr<util::security::HMAC> hmac_;
+
   std::shared_ptr<SocketCore> socket_;
   std::shared_ptr<SocketRecvBuffer> socketRecvBuffer_;
   SocketBuffer socketBuffer_;
@@ -77,8 +86,8 @@ private:
   std::unique_ptr<DiskWriter> lastBody_;
   bool keepAlive_;
   bool gzip_;
-  std::string username_;
+  std::unique_ptr<util::security::HMACResult> username_;
-  std::string password_;
+  std::unique_ptr<util::security::HMACResult> password_;
   bool acceptsGZip_;
   std::string allowOrigin_;
   bool secure_;

src/util.h

@@ -156,14 +156,20 @@ std::string strip
 template<typename InputIterator>
 std::pair<std::pair<InputIterator, InputIterator>,
           std::pair<InputIterator, InputIterator>>
-divide(InputIterator first, InputIterator last, char delim)
+divide(InputIterator first, InputIterator last, char delim, bool strip=true)
 {
   auto dpos = std::find(first, last, delim);
-  if(dpos == last) {
+  if (dpos == last) {
+    if (strip) {
       return {stripIter(first, last), {last, last}};
-  } else {
-    return {stripIter(first, dpos), stripIter(dpos+1, last)};
     }
+    return {{first, last}, {last, last}};
+  }
+
+  if (strip) {
+    return {stripIter(first, dpos), stripIter(dpos + 1, last)};
+  }
+  return {{first, dpos}, {dpos + 1, last}};
 }
 
 template<typename T>

test/HttpServerTest.cc

@@ -0,0 +1,188 @@
+#include "HttpServer.h"
+
+#include <cppunit/extensions/HelperMacros.h>
+
+#include "SocketCore.h"
+#include "a2functional.h"
+
+namespace aria2 {
+
+class HttpServerTest : public CppUnit::TestFixture
+{
+  CPPUNIT_TEST_SUITE(HttpServerTest);
+  CPPUNIT_TEST(testHttpBasicAuth);
+  CPPUNIT_TEST_SUITE_END();
+public:
+  void testHttpBasicAuth();
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION(HttpServerTest);
+
+namespace {
+  std::unique_ptr<HttpServer> performHttpRequest(SocketCore& server, std::string request)
+  {
+    std::pair<std::string, uint16_t> addr;
+    server.getAddrInfo(addr);
+
+    SocketCore client;
+    client.establishConnection("localhost", addr.second);
+    while (!client.isWritable(0)) {}
+
+    auto inbound = server.acceptConnection();
+    inbound->setBlockingMode();
+    auto rv = make_unique<HttpServer>(inbound);
+
+    client.writeData(request);
+    while (!rv->receiveRequest()) {}
+    return rv;
+  }
+} // namespace
+
+void HttpServerTest::testHttpBasicAuth()
+{
+  SocketCore server;
+  server.bind(0);
+  server.beginListen();
+  server.setBlockingMode();
+
+  {
+    // Default is no auth
+    auto req = performHttpRequest(
+        server, "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\n\r\n");
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Empty user-name and password should come out as no auth.
+    auto req = performHttpRequest(
+        server, "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\n\r\n");
+    req->setUsernamePassword("", "");
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Empty user-name but set password should also come out as no auth.
+    auto req = performHttpRequest(
+        server, "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\n\r\n");
+    req->setUsernamePassword("", "pass");
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Client provided credentials should be ignored when there is no auth.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNz\r\n\r\n");
+    req->setUsernamePassword("", "pass");
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Correct client provided credentials should match.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNz\r\n\r\n");
+    req->setUsernamePassword("user", "pass");
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Correct client provided credentials should match (2).
+    // Embedded nulls
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcgBudWxsOnBhc3MAbnVsbA==\r\n\r\n");
+    req->setUsernamePassword(std::string("user\0null", 9), std::string("pass\0null", 9));
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Correct client provided credentials should match (3).
+    // Embedded, leading nulls
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic AHVzZXI6AHBhc3M=\r\n\r\n");
+    req->setUsernamePassword(std::string("\0user", 5), std::string("\0pass", 5));
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Correct client provided credentials should match (3).
+    // Whitespace
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic IHVzZXIJOgpwYXNzDQ==\r\n\r\n");
+    req->setUsernamePassword(" user\t", "\npass\r");
+    CPPUNIT_ASSERT(req->authenticate());
+  }
+
+  {
+    // Wrong client provided credentials should NOT match.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNz\r\n\r\n");
+    req->setUsernamePassword("user", "pass2");
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+
+  {
+    // Wrong client provided credentials should NOT match (2).
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNz\r\n\r\n");
+    req->setUsernamePassword("user2", "pass");
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+
+  {
+    // Wrong client provided credentials should NOT match (3).
+    // Embedded null in pass config.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNz\r\n\r\n");
+    req->setUsernamePassword("user", std::string("pass\0three", 10));
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+
+  {
+    // Wrong client provided credentials should NOT match (4).
+    // Embedded null in user config.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNz\r\n\r\n");
+    req->setUsernamePassword(std::string("user\0four", 9), "pass");
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+
+  {
+    // Wrong client provided credentials should NOT match (5).
+    // Embedded null in http auth.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic dXNlcjpwYXNzAHRocmVl\r\n\r\n");
+    req->setUsernamePassword("user", "pass");
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+
+  {
+    // Wrong client provided credentials should NOT match (6).
+    // Embedded null in http auth.
+    // Embedded, leading nulls
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\nAuthorization: Basic AHVzZXI6AHBhc3M=\r\n\r\n");
+    req->setUsernamePassword(std::string("\0user5", 6), std::string("\0pass", 5));
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+
+  {
+    // When there is a user and password, the client must actually provide auth.
+    auto req = performHttpRequest(
+        server,
+        "GET / HTTP/1.1\r\nUser-Agent: aria2-test\r\n\r\n");
+    req->setUsernamePassword("user", "pass");
+    CPPUNIT_ASSERT(!req->authenticate());
+  }
+}
+
+} // namespace aria2

test/Makefile.am

@@ -77,6 +77,7 @@ aria2c_SOURCES = AllTest.cc\
 	ValueBaseJsonParserTest.cc\
 	RpcResponseTest.cc\
 	RpcMethodTest.cc\
+	HttpServerTest.cc\
 	BufferedFileTest.cc\
 	GeomStreamPieceSelectorTest.cc\
 	SegListTest.cc\