[ext_proc] Filter allows external processor to set 1XX status codes

[ikepolinsky]

Received permission to make this crash public from @htuch and @gbrail

Title: Filter allows external processor to set 1XX status codes

Description:
The ext_proc filter does not validate changes to the response status code. This enables a processor to set the status code to 1XX and violate the FilterManager::encodeHeaders precondition.

Repro steps:
Adding the following test case to the integration test reproduces the crash.

// Test that the filter prevents an external process from changing the status code
// to 1XX. FilterManager::encodeHeaders() expects the status code is not 1XX and
// if this precondition is not held, envoy will crash.
TEST_P(ExtProcIntegrationTest, ResponseStatus100) {
  initializeConfig();
  HttpIntegrationTest::initialize();
  auto response = sendDownstreamRequest(absl::nullopt);

  processAndRespondImmediately(true, [](ImmediateResponse& immediate) {
    immediate.mutable_status()->set_code(envoy::type::v3::StatusCode::Continue);
  });

  // TODO downstream response status should not be 100
}

Call Stack:

[2021-08-03 00:58:24.424][1717][debug][filter] [source/extensions/filters/http/ext_proc/ext_proc.cc:393] Received immediate response response
[2021-08-03 00:58:24.424][1717][debug][http] [source/common/http/filter_manager.cc:909] [C739] [S17764435551718391193] Sending local reply with details
[2021-08-03 00:58:24.424][1717][critical][assert] [source/common/http/filter_manager.cc:1041] assert failure: !CodeUtility::is1xx(Utility::getResponseStatus(headers)) || Utility::getResponseStatus(headers) == enumToInt(Http::Code::SwitchingProtocols).
[2021-08-03 00:58:24.424][1717][critical][backtrace] [./source/server/backtrace.h:104] Caught Aborted, suspect faulting address 0xd40010000000c

[gbrail]

We already have code that rejects attempts to delete system headers like :path and :status -- in that case, the filter ignores the response and logs a "debug" message. We'll do the same here for status codes < 200.

Open question -- are there other status codes that will cause other problems for Envoy? Should we also ignore attempts to set status codes >= 600 since those aren't a part of the HTTP specs?

[ikepolinsky]

I don't see any indication that other status codes will cause problems, but that might be hidden somewhere deeper than encodeHeaders.

Somewhat related to this status code issue is this check in encodeHeaders and it may be possible for the external processor to create a condition where this assert fails.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.