return an error and deny access if the POST content unexpectedly terminates (#23587)

KIKIMR-239917

4 files changed, 180 insertions, 4 deletions

diff --git a/ydb/core/http_proxy/http_req.cpp b/ydb/core/http_proxy/http_req.cpp
index 1bf29548159..49c9ffde97e 100644
--- a/ydb/core/http_proxy/http_req.cpp
+++ b/ydb/core/http_proxy/http_req.cpp
@@ -1154,7 +1154,7 @@ namespace NKikimr::NHttpProxy {
                 Request->URL + " " +
                 Request->Protocol + "/" + Request->Version + "\r\n" +
                 Request->Headers +
-                Request->Content;
+                Request->Body;
             signature = MakeHolder<NKikimr::NSQS::TAwsRequestSignV4>(fullRequest);
         }
 
diff --git a/ydb/core/http_proxy/ut/http_ut.cpp b/ydb/core/http_proxy/ut/http_ut.cpp
new file mode 100644
index 00000000000..2f445c70d48
--- /dev/null
+++ b/ydb/core/http_proxy/ut/http_ut.cpp
@@ -0,0 +1,167 @@
+#include <ydb/core/http_proxy/http_req.h>
+#include <ydb/core/testlib/test_client.h>
+
+#include <ydb/library/testlib/service_mocks/access_service_mock.h>
+#include <ydb/library/testlib/service_mocks/iam_token_service_mock.h>
+
+#include <library/cpp/testing/unittest/registar.h>
+#include <library/cpp/http/misc/httpcodes.h>
+
+using namespace NKikimr::NHttpProxy;
+using namespace NKikimr::Tests;
+using namespace NActors;
+
+#include "datastreams_fixture.h"
+
+Y_UNIT_TEST_SUITE(TestMalformedRequest) {
+
+    void TestContentLengthDiff(THttpProxyTestMock& mock, const std::optional<int> contentLengthDiff, TStringBuf compressed) {
+        constexpr TStringBuf headers[]{
+            "POST /Root HTTP/1.1",
+            "Host:example.amazonaws.com",
+            "X-Amz-Target:AmazonSQS.CreateQueue",
+            "X-Amz-Date:20150830T123600Z",
+            "Authorization: AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/20150830/ru-central1/service/aws4_request, SignedHeaders=host;x-amz-date, Signature=5da7c1a2acd57cee7505fc6676e4e544621c30862966e37dddb68e92efbe5d6b)__",
+            "Content-Type:application/json",
+        };
+
+        constexpr TStringBuf jsonStr = R"-({"QueueName": "Example"})-";
+        constexpr TStringBuf deflateJsonStr = "\x78\x01\xab\x56\x0a\x2c\x4d\x2d\x4d\xf5\x4b\xcc\x4d\x55\xb2\x52\x50\x72\xad\x48\xcc\x2d\xc8\x49\x55\xaa\x05\x00\x66\x69\x08\x2d"sv;
+        constexpr TStringBuf gzipJsonStr = "\x1f\x8b\x08\x00\xd8\x6e\xae\x68\x02\xff\xab\x56\x0a\x2c\x4d\x2d\x4d\xf5\x4b\xcc\x4d\x55\xb2\x52\x50\x72\xad\x48\xcc\x2d\xc8\x49\x55\xaa\x05\x00\x32\xf8\x6a\x11\x18\x00\x00\x00"sv;
+        static_assert(jsonStr.size() != deflateJsonStr.size());
+        static_assert(jsonStr.size() != gzipJsonStr.size());
+        TStringBuf payload = jsonStr;
+        if (compressed == "deflate") {
+            payload = deflateJsonStr;
+        } else if (compressed == "gzip") {
+            payload = gzipJsonStr;
+        } else {
+            UNIT_ASSERT(compressed.empty());
+        }
+        std::optional<ssize_t> contentLength = std::nullopt;
+        if (contentLengthDiff.has_value()) {
+            ssize_t sz = payload.size();
+            sz += contentLengthDiff.value();
+            sz = Max<ssize_t>(1, sz);
+            contentLength = sz;
+        }
+
+        TString request;
+        {
+            TStringOutput ss{request};
+            for (const auto h : headers) {
+                ss << h << "\r\n";
+            }
+            if (compressed) {
+                ss << "Content-Encoding: " << compressed << "\r\n";
+            }
+            if (contentLength) {
+                ss << "Content-Length: " << *contentLength << "\r\n";
+            }
+            ss << "\r\n";
+            ss << payload;
+            ss.Finish();
+        }
+
+        auto post = [&request, &mock]() -> THttpResult {
+            TNetworkAddress addr("::", mock.HttpServicePort);
+            TSocket sock(addr);
+            {
+                TSocketOutput so(sock);
+                so.Write(request);
+                so.Finish();
+           }
+
+            TSocketInput si(sock);
+            THttpInput input(&si);
+
+            bool gotRequestId{false};
+            for (auto& header : input.Headers()) {
+                gotRequestId |= header.Name() == "x-amzn-requestid";
+            }
+            Y_ABORT_UNLESS(gotRequestId);
+            ui32 httpCode = ParseHttpRetCode(input.FirstLine());
+            TString description(StripString(TStringBuf(input.FirstLine()).After(' ').After(' ')));
+            TString responseBody = input.ReadAll();
+            Cerr << "Http output full " << responseBody << Endl;
+            return {httpCode, description, responseBody};
+        };
+
+        const bool expectCorrect = contentLengthDiff == 0;  // Content-Length is mandatory
+
+        TMaybe<THttpResult> res;
+        try {
+            res = post();
+        } catch (const THttpReadException&) {
+            if (expectCorrect) {
+                throw;
+            }
+        }
+
+        if (expectCorrect) {
+            UNIT_ASSERT(res.Defined());
+            UNIT_ASSERT_VALUES_EQUAL_C(res->HttpCode, 200, LabeledOutput(res->HttpCode, res->Description, res->Body));
+            NJson::TJsonMap json;
+            UNIT_ASSERT(NJson::ReadJsonTree(res->Body, &json, true));
+        } else {
+            if (res) {
+                // Option A: Server returns error code
+                UNIT_ASSERT_C(IsUserError(res->HttpCode), LabeledOutput(res->HttpCode, res->Description, res->Body));
+            } else {
+                // Option B: Server may ignore malformed request and close connection
+            }
+        }
+    }
+
+    Y_UNIT_TEST_F(ContentLengthNone, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, std::nullopt, "");
+    }
+
+    Y_UNIT_TEST_F(ContentLengthCorrect, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, 0, "");
+    }
+
+    Y_UNIT_TEST_F(ContentLengthLower, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, -3, "");
+    }
+
+    Y_UNIT_TEST_F(ContentLengthHigher, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, +3000, "");
+    }
+
+    Y_UNIT_TEST_F(CompressedDeflateContentLengthNone, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, std::nullopt, "deflate");
+    }
+
+    Y_UNIT_TEST_F(CompressedDeflateContentLengthCorrect, THttpProxyTestMock) {
+        UNIT_ASSERT_TEST_FAILS_C(TestContentLengthDiff(*this, 0, "deflate"), "XFail: deflate decode is broken: POST body decompressed twice");
+    }
+
+    Y_UNIT_TEST_F(CompressedDeflateContentLengthLower, THttpProxyTestMock) {
+        if ("the deflate fails if the sream is terminated prematurely") {
+            return;
+        }
+        TestContentLengthDiff(*this, -3000, "deflate");
+    }
+
+    Y_UNIT_TEST_F(CompressedDeflateContentLengthHigher, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, +3000, "deflate");
+    }
+
+    Y_UNIT_TEST_F(CompressedGzipContentLengthNone, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, std::nullopt, "gzip");
+    }
+
+    Y_UNIT_TEST_F(CompressedGzipContentLengthCorrect, THttpProxyTestMock) {
+        UNIT_ASSERT_TEST_FAILS_C(TestContentLengthDiff(*this, 0, "gzip"), "X-Fail: gzip body doesn't decoded befor json parsing");
+    }
+
+    Y_UNIT_TEST_F(CompressedGzipContentLengthLower, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, -3000, "gzip");
+    }
+
+    Y_UNIT_TEST_F(CompressedGzipContentLengthHigher, THttpProxyTestMock) {
+        TestContentLengthDiff(*this, +3000, "gzip");
+    }
+
+} // Y_UNIT_TEST_SUITE(TestMalformedRequest)
diff --git a/ydb/core/http_proxy/ut/ya.make b/ydb/core/http_proxy/ut/ya.make
index a689f09e0e9..4624681bb00 100644
--- a/ydb/core/http_proxy/ut/ya.make
+++ b/ydb/core/http_proxy/ut/ya.make
@@ -7,6 +7,7 @@ FORK_SUBTESTS()
 PEERDIR(
     contrib/restricted/nlohmann_json
     library/cpp/resource
+    library/cpp/http/misc
     ydb/core/base
     ydb/core/http_proxy
     ydb/core/testlib/default
@@ -31,6 +32,7 @@ PEERDIR(
 SRCS(
     json_proto_conversion_ut.cpp
     datastreams_fixture.h
+    http_ut.cpp
 )
 
 RESOURCE(
diff --git a/ydb/library/http_proxy/authorization/signature.cpp b/ydb/library/http_proxy/authorization/signature.cpp
index cb400927a6c..e6c7c6fab73 100644
--- a/ydb/library/http_proxy/authorization/signature.cpp
+++ b/ydb/library/http_proxy/authorization/signature.cpp
@@ -1,6 +1,8 @@
 #include "auth_helpers.h"
 #include "signature.h"
 
+#include <ydb/library/http_proxy/error/error.h>
+
 #include <library/cpp/http/io/stream.h>
 #include <library/cpp/http/misc/parsed_request.h>
 
@@ -11,6 +13,7 @@
 #include <util/generic/algorithm.h>
 #include <util/generic/map.h>
 #include <util/generic/vector.h>
+#include <util/stream/buffer.h>
 #include <util/stream/str.h>
 #include <util/string/builder.h>
 #include <library/cpp/cgiparam/cgiparam.h>
@@ -67,9 +70,13 @@ TAwsRequestSignV4::TAwsRequestSignV4(const TString& request) {
     if (parsed.Method == "POST") {
         if (input.GetContentLength(contentLength)) {
             inputData.ConstructInPlace();
-            inputData->Resize(contentLength);
-            if (input.Load(inputData->Data(), (size_t)contentLength) != contentLength) {
-                Y_ABORT_UNLESS(false);
+            inputData->Reserve(contentLength);
+            TBufferOutput bufOut{*inputData};
+            try {
+                TransferData(&input, &bufOut);
+            } catch (const std::exception& e) {
+                throw NKikimr::NSQS::TSQSException(NKikimr::NSQS::NErrors::INTERNAL_FAILURE)
+                    << "Failed to decode POST body";
             }
         }
     }