security hardening

lib/src/Cookie.cc

@@ -19,12 +19,26 @@ using namespace drogon;
 
 std::string Cookie::cookieString() const
 {
+    // Strip CR and LF from attacker-influenceable fields so that a value
+    // containing "\r\n" cannot inject additional headers / split the
+    // response via the Set-Cookie line.
+    auto sanitize = [](const std::string &in) {
+        std::string out;
+        out.reserve(in.size());
+        for (char c : in)
+            if (c != '\r' && c != '\n')
+                out.push_back(c);
+        return out;
+    };
+    const std::string key = sanitize(key_);
+    const std::string value = sanitize(value_);
+
     constexpr std::string_view prefix = "Set-Cookie: ";
     std::string ret;
     // reserve space to reduce frequency allocation
-    ret.reserve(prefix.size() + key_.size() + value_.size() + 30);
+    ret.reserve(prefix.size() + key.size() + value.size() + 30);
     ret = prefix;
-    ret.append(key_).append("=").append(value_).append("; ");
+    ret.append(key).append("=").append(value).append("; ");
     if (expiresDate_.microSecondsSinceEpoch() !=
             (std::numeric_limits<int64_t>::max)() &&
         expiresDate_.microSecondsSinceEpoch() >= 0)
@@ -41,11 +55,11 @@ std::string Cookie::cookieString() const
     }
     if (!domain_.empty())
     {
-        ret.append("Domain=").append(domain_).append("; ");
+        ret.append("Domain=").append(sanitize(domain_)).append("; ");
     }
     if (!path_.empty())
     {
-        ret.append("Path=").append(path_).append("; ");
+        ret.append("Path=").append(sanitize(path_)).append("; ");
     }
     if (sameSite_ != SameSite::kNull)
     {

lib/src/HttpRequestImpl.cc

@@ -503,6 +503,22 @@ void HttpRequestImpl::addHeader(const char *start,
             }
             break;
 
+            case 14:
+                if (field == "content-length")
+                {
+                    // RFC 9110 8.6: a message with multiple Content-Length
+                    // fields carrying differing values has ambiguous framing
+                    // and must be rejected (request smuggling vector). The
+                    // header map keeps the first value, so record the conflict
+                    // here before it is collapsed.
+                    auto it = headers_.find(field);
+                    if (it != headers_.end() && it->second != value)
+                    {
+                        multipleContentLength_ = true;
+                    }
+                }
+                break;
+
             default:
                 break;
         }

lib/src/HttpRequestImpl.h

@@ -75,6 +75,7 @@ class HttpRequestImpl : public HttpRequest
         cookies_.clear();
         contentLengthHeaderValue_.reset();
         realContentLength_ = 0;
+        multipleContentLength_ = false;
         flagForParsingParameters_ = false;
         path_.clear();
         originalPath_.clear();
@@ -709,6 +710,10 @@ class HttpRequestImpl : public HttpRequest
     SafeStringMap<std::string> cookies_;
     std::optional<size_t> contentLengthHeaderValue_;
     size_t realContentLength_{0};
+    // Set when more than one Content-Length header with differing values is
+    // received. Such a request has ambiguous framing (request smuggling
+    // vector) and is rejected by the parser.
+    bool multipleContentLength_{false};
     mutable SafeStringMap<std::string> parameters_;
     mutable std::shared_ptr<Json::Value> jsonPtr_;
     SessionPtr sessionPtr_;

lib/src/HttpRequestParser.cc

@@ -221,9 +221,35 @@ int HttpRequestParser::parseRequest(MsgBuffer *buf)
                 // and maintainability.
 
                 // process header information
+                const std::string &encode =
+                    request_->getHeaderBy("transfer-encoding");
                 auto &len = request_->getHeaderBy("content-length");
+
+                // RFC 9112 6.1 / RFC 9110 8.6: a request carrying both a
+                // Content-Length and a Transfer-Encoding, or conflicting
+                // Content-Length values, has ambiguous framing and is a
+                // request smuggling vector. Reject it outright rather than
+                // silently favoring one header over the other.
+                if (!encode.empty() && !len.empty())
+                {
+                    return -k400BadRequest;
+                }
+                if (request_->multipleContentLength_)
+                {
+                    return -k400BadRequest;
+                }
+
                 if (!len.empty())
                 {
+                    // Content-Length must be a run of digits only; reject a
+                    // leading sign, whitespace or trailing garbage that
+                    // std::stoull would otherwise silently accept (e.g. "-1"
+                    // wrapping to a huge value, or "5abc").
+                    if (len.find_first_not_of("0123456789") !=
+                        std::string::npos)
+                    {
+                        return -k400BadRequest;
+                    }
                     try
                     {
                         remainContentLength_ =
@@ -246,8 +272,6 @@ int HttpRequestParser::parseRequest(MsgBuffer *buf)
                 }
                 else
                 {
-                    const std::string &encode =
-                        request_->getHeaderBy("transfer-encoding");
                     if (encode.empty())
                     {
                         // no content-length and no transfer-encoding,
@@ -367,18 +391,34 @@ int HttpRequestParser::parseRequest(MsgBuffer *buf)
                 // chunk length line
                 std::string len(buf->peek(), crlf - buf->peek());
                 char *end;
-                currentChunkLength_ = strtol(len.c_str(), &end, 16);
-                if (currentChunkLength_ != 0)
+                // Parse as unsigned; a signed strtol would turn a value such
+                // as "-1" into a huge size_t after the cast.
+                unsigned long long chunkLen = strtoull(len.c_str(), &end, 16);
+                // The field must begin with a valid hex digit and may only be
+                // followed by a chunk extension (";...") or trailing
+                // whitespace; anything else is a malformed chunk size.
+                if (end == len.c_str() || (*end != '\0' && *end != ';' &&
+                                           *end != ' ' && *end != '\t'))
+                {
+                    return -k400BadRequest;
+                }
+                if (chunkLen != 0)
                 {
-                    if (currentChunkLength_ + remainContentLength_ >
-                        HttpAppFrameworkImpl::instance().getClientMaxBodySize())
+                    auto maxBodySize =
+                        HttpAppFrameworkImpl::instance().getClientMaxBodySize();
+                    // Compare on the wide unsigned value and subtract from the
+                    // limit to avoid the size_t overflow that "chunkLen +
+                    // remainContentLength_" could wrap past.
+                    if (chunkLen > maxBodySize - remainContentLength_)
                     {
                         return -k413RequestEntityTooLarge;
                     }
+                    currentChunkLength_ = static_cast<size_t>(chunkLen);
                     status_ = HttpRequestParseStatus::kExpectChunkBody;
                 }
                 else
                 {
+                    currentChunkLength_ = 0;
                     status_ = HttpRequestParseStatus::kExpectLastEmptyChunk;
                 }
                 buf->retrieveUntil(crlf + CRLF_LEN);

lib/src/HttpResponseImpl.h

@@ -22,6 +22,7 @@
 #include <trantor/net/InetAddress.h>
 #include <trantor/utils/Date.h>
 #include <trantor/utils/MsgBuffer.h>
+#include <algorithm>
 #include <memory>
 #include <mutex>
 #include <string>
@@ -155,14 +156,30 @@ class DROGON_EXPORT HttpResponseImpl : public HttpResponse
         headers_.erase(lowerKey);
     }
 
+    // Strip CR and LF from a header name or value. Untrusted data placed
+    // into a response header would otherwise allow HTTP response splitting /
+    // header injection by embedding a "\r\n" sequence.
+    static std::string sanitizeHeaderField(std::string field)
+    {
+        field.erase(std::remove_if(field.begin(),
+                                   field.end(),
+                                   [](char c) {
+                                       return c == '\r' || c == '\n';
+                                   }),
+                    field.end());
+        return field;
+    }
+
     void addHeader(std::string field, const std::string &value) override
     {
         fullHeaderString_.reset();
         transform(field.begin(),
                   field.end(),
                   field.begin(),
                   [](unsigned char c) { return tolower(c); });
-        headers_[std::move(field)] = value;
+        auto sanitizedValue = sanitizeHeaderField(value);
+        headers_[sanitizeHeaderField(std::move(field))] =
+            std::move(sanitizedValue);
     }
 
     void addHeader(std::string field, std::string &&value) override
@@ -172,7 +189,9 @@ class DROGON_EXPORT HttpResponseImpl : public HttpResponse
                   field.end(),
                   field.begin(),
                   [](unsigned char c) { return tolower(c); });
-        headers_[std::move(field)] = std::move(value);
+        auto sanitizedValue = sanitizeHeaderField(std::move(value));
+        headers_[sanitizeHeaderField(std::move(field))] =
+            std::move(sanitizedValue);
     }
 
     void addHeader(const char *start, const char *colon, const char *end);
@@ -233,7 +252,9 @@ class DROGON_EXPORT HttpResponseImpl : public HttpResponse
 
     void redirect(const std::string &url)
     {
-        headers_["location"] = url;
+        // Sanitize to prevent header injection when the target is derived
+        // from untrusted input.
+        headers_["location"] = sanitizeHeaderField(url);
     }
 
     std::shared_ptr<trantor::MsgBuffer> renderToBuffer();

lib/src/StaticFileRouter.cc

@@ -76,7 +76,15 @@ void StaticFileRouter::route(
     const std::string &path = req->path();
     if (path.find("..") != std::string::npos)
     {
-        auto directories = utils::splitString(path, "/");
+        // Treat the backslash as a path separator as well. On Windows the
+        // filesystem accepts '\\' as a directory separator, so a request
+        // target such as "..%5c..%5c" (which is url-decoded to "..\..\")
+        // must not be able to slip past a traversal check that only splits
+        // on '/'. Normalize a copy of the path used solely for this check;
+        // the path actually used to locate the file is left untouched.
+        std::string normalizedPath = path;
+        std::replace(normalizedPath.begin(), normalizedPath.end(), '\\', '/');
+        auto directories = utils::splitString(normalizedPath, "/");
         int traversalDepth = 0;
         for (const auto &dir : directories)
         {

orm_lib/inc/drogon/orm/Criteria.h

@@ -333,6 +333,13 @@ class DROGON_EXPORT Criteria
      * ["id","!=",null] means 'id is not null'
      * ["user_name","in",["Tom","Bob"]] means 'user_name in ('Tom', 'Bob')'
      * ["price","<",1000] means 'price < 1000'
+     *
+     * @note The value (third item) is always bound as a query parameter and is
+     * therefore safe. The comparison operator is validated against a fixed
+     * allowlist and throws if unrecognized. The field name (first item),
+     * however, is concatenated directly into the SQL statement and cannot be
+     * parameterized; do not build a Criteria from an untrusted field name
+     * without validating it against your own allowlist of column names.
      */
     explicit Criteria(const Json::Value &json) noexcept(false);
     Criteria() = default;

orm_lib/src/Criteria.cc

@@ -14,11 +14,69 @@
 
 #include <drogon/orm/Criteria.h>
 #include <json/json.h>
+#include <cctype>
+#include <stdexcept>
+#include <string>
+#include <unordered_map>
 
 namespace drogon
 {
 namespace orm
 {
+namespace
+{
+// Normalize an operator token: trim surrounding whitespace, lowercase, and
+// collapse internal runs of whitespace to a single space, so that both "> ="
+// style spacing and case variations map to a single canonical form.
+std::string normalizeOperator(const std::string &raw)
+{
+    std::string out;
+    out.reserve(raw.size());
+    bool pendingSpace = false;
+    for (char c : raw)
+    {
+        auto uc = static_cast<unsigned char>(c);
+        if (std::isspace(uc))
+        {
+            pendingSpace = true;
+            continue;
+        }
+        if (pendingSpace && !out.empty())
+            out.push_back(' ');
+        pendingSpace = false;
+        out.push_back(static_cast<char>(std::tolower(uc)));
+    }
+    return out;
+}
+
+// Map a caller-supplied comparison operator to a fixed, safe SQL fragment.
+// In the JSON form of Criteria the operator is concatenated straight into the
+// query string (it cannot be bound as a parameter), so it must be restricted
+// to a known set to avoid SQL injection through an attacker-controlled filter.
+const std::string &comparisonOperatorSql(const std::string &raw)
+{
+    static const std::unordered_map<std::string, std::string> allowed = {
+        {"=", " = "},
+        {"!=", " != "},
+        {"<>", " <> "},
+        {">", " > "},
+        {">=", " >= "},
+        {"<", " < "},
+        {"<=", " <= "},
+        {"like", " like "},
+        {"not like", " not like "},
+        {"ilike", " ilike "},
+        {"not ilike", " not ilike "},
+    };
+    auto it = allowed.find(normalizeOperator(raw));
+    if (it == allowed.end())
+    {
+        throw std::runtime_error("Invalid comparison operator in Criteria");
+    }
+    return it->second;
+}
+}  // namespace
+
 const Criteria operator&&(Criteria cond1, Criteria cond2)
 {
     bool cond1valid = (bool)cond1, cond2valid = (bool)cond2;
@@ -104,7 +162,7 @@ Criteria::Criteria(const Json::Value &json) noexcept(false)
         {
             throw std::runtime_error("Json format error");
         }
-        conditionString_.append(json[1].asString());
+        conditionString_.append(comparisonOperatorSql(json[1].asString()));
         conditionString_.append("$?");
         outputArgumentsFunc_ =
             [arg = json[2].asString()](internal::SqlBinder &binder) {