lua: fix a bug where setting a big response body in lua will result in crash (envoyproxy#41571)

Commit Message: lua: fix a bug where setting a big response body in lua
will result in crash
Additional Description:

At the response phase, when the lua script rewrite the response body to
a huge body, the wartermark callback will be triggered and result a
direct local response. The direct local response headers will override
the original response headers and result in all references of original
response headers be dangling.
But note, because the lua didn't aware the he wartermark callback and
will still try to update the content-length of the original response
headers, so, finally it result in a crash.

Risk Level: low.
Testing: unit, integration.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.

---------

Signed-off-by: WangBaiping <[email protected]>
Signed-off-by: code <[email protected]>
Co-authored-by: phlax <[email protected]>

Author: wbpcode

changelogs/current.yaml

@@ -8,6 +8,10 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: lua
+  change: |
+    Fix a bug where Lua filters may result in Envoy crashes when setting response body to a
+    larger payload (greater than the body buffer limit).
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/http/conn_manager_impl.h

@@ -250,11 +250,15 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
       informational_headers_ = std::move(informational_headers);
     }
     void setResponseHeaders(Http::ResponseHeaderMapPtr&& response_headers) override {
-      // We'll overwrite the headers in the case where we fail the stream after upstream headers
-      // have begun filter processing but before they have been sent downstream.
+      if (response_headers_ != nullptr) {
+        overwritten_headers_.emplace_back(std::move(response_headers_));
+      }
       response_headers_ = std::move(response_headers);
     }
     void setResponseTrailers(Http::ResponseTrailerMapPtr&& response_trailers) override {
+      if (response_trailers_ != nullptr) {
+        overwritten_headers_.emplace_back(std::move(response_trailers_));
+      }
       response_trailers_ = std::move(response_trailers);
     }
     void chargeStats(const ResponseHeaderMap& headers) override;
@@ -448,6 +452,13 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     ResponseHeaderMapSharedPtr response_headers_;
     ResponseTrailerMapSharedPtr response_trailers_;
 
+    // Keep track all the historical headers to avoid potential lifetime issues.
+    // For example,
+    // when Envoy processing a response, if we send a local reply, then the local reply
+    // headers will overwrite the original response and result in the previous response
+    // being dangling. To avoid this, we store the original headers.
+    std::vector<std::shared_ptr<HeaderMap>> overwritten_headers_;
+
     // Note: The FM must outlive the above headers, as they are possibly accessed during filter
     // destruction.
     DownstreamFilterManager filter_manager_;

source/extensions/filters/common/lua/wrappers.cc

@@ -66,9 +66,10 @@ int BufferWrapper::luaGetBytes(lua_State* state) {
 
 int BufferWrapper::luaSetBytes(lua_State* state) {
   data_.drain(data_.length());
+  ASSERT(data_.length() == 0); // Defensive check.
   absl::string_view bytes = getStringViewFromLuaString(state, 2);
+  headers_.setContentLength(bytes.size());
   data_.add(bytes);
-  headers_.setContentLength(data_.length());
   lua_pushnumber(state, data_.length());
   return 1;
 }

test/common/http/conn_manager_impl_test_2.cc

@@ -1820,6 +1820,8 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) {
   // Start the response without processing the request headers through all
   // filters.
   ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}};
+  ResponseHeaderMap* original_response_headers = response_headers.get();
+
   EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false))
       .WillOnce(Return(FilterHeadersStatus::StopIteration));
   decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails("");
@@ -1837,6 +1839,8 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) {
   // The 500 goes directly to the encoder.
   EXPECT_CALL(response_encoder_, encodeHeaders(_, false))
       .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> FilterHeadersStatus {
+        // The new headers should overwrite the original headers.
+        EXPECT_NE(&headers, original_response_headers);
         // Make sure this is a 500
         EXPECT_EQ("500", headers.getStatusValue());
         // Make sure Envoy standard sanitization has been applied.
@@ -1849,6 +1853,10 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) {
   decoder_filters_[0]->callbacks_->encodeData(fake_response, false);
   EXPECT_EQ("Internal Server Error", response_body);
 
+  // The active stream will keep the overwritten headers alive to avoid potential lifetime issues.
+  // Ensure the original headers are still valid.
+  EXPECT_EQ(original_response_headers->getStatusValue(), "200");
+
   EXPECT_EQ(1U, stats_.named_.rs_too_large_.value());
 }
 

test/extensions/filters/http/lua/lua_integration_test.cc

@@ -263,6 +263,23 @@ class LuaIntegrationTest : public UpstreamDownstreamIntegrationTest {
     expectResponseBodyRewrite(code, true, enable_wrap_body);
   }
 
+  IntegrationStreamDecoderPtr initializeAndSendRequest(const std::string& code) {
+    initializeFilter(code);
+    codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http")));
+    Http::TestRequestHeaderMapImpl request_headers{{":method", "POST"},
+                                                   {":path", "/test/long/url"},
+                                                   {":scheme", "http"},
+                                                   {":authority", "foo.lyft.com"},
+                                                   {"x-forwarded-for", "10.0.0.1"}};
+
+    auto encoder_decoder = codec_client_->startRequest(request_headers);
+    Buffer::OwnedImpl request_data("done");
+    encoder_decoder.first.encodeData(request_data, true);
+    waitForNextUpstreamRequest();
+
+    return std::move(encoder_decoder.second);
+  }
+
   void cleanup() {
     codec_client_->close();
     if (fake_lua_connection_ != nullptr) {
@@ -1411,6 +1428,41 @@ name: lua
   testRewriteResponse(FILTER_AND_CODE);
 }
 
+// Rewrite response buffer to a huge body.
+TEST_P(LuaIntegrationTest, RewriteResponseToHugeBody) {
+  const std::string FILTER_AND_CODE =
+      R"EOF(
+name: lua
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
+  default_source_code:
+    inline_string: |
+      function envoy_on_response(response_handle)
+        -- Default HTTP2 body buffer limit is 16MB for now. To set
+        -- a 16MB+ body to ensure both HTTP1 and HTTP2 will hit the limit.
+        local huge_body = string.rep("a", 1024 * 1024 * 16 + 1) -- 16MB + 1
+        local content_length = response_handle:body():setBytes(huge_body)
+        response_handle:logTrace(content_length)
+      end
+)EOF";
+
+  auto response = initializeAndSendRequest(FILTER_AND_CODE);
+
+  Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, {"foo", "bar"}};
+  upstream_request_->encodeHeaders(response_headers, false);
+  Buffer::OwnedImpl response_data1("good");
+  upstream_request_->encodeData(response_data1, false);
+  Buffer::OwnedImpl response_data2("bye");
+  upstream_request_->encodeData(response_data2, true);
+
+  ASSERT_TRUE(response->waitForEndStream());
+  ASSERT_TRUE(response->complete());
+
+  EXPECT_EQ(response->headers().getStatusValue(), "500");
+
+  cleanup();
+}
+
 // Rewrite response buffer, without original upstream response body
 // and always wrap body.
 TEST_P(LuaIntegrationTest, RewriteResponseBufferWithoutUpstreamBody) {