Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert(valid) when ext_proc filter apply header mutations #27547

Merged

Conversation

yanjunxiang-google
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google commented May 22, 2023

When ext_proc server sends back some header mutation settings which contain NULL characters, like /n, /r or /0, this will cause ext_proc filter ASSERT(valid) being fired up when Envoy is built in debug mode.
In release mode, the ASSERT is no-op, however, we should add an explicit check to avoid such kind of headers get injected into Envoy.

Assert(valid) when ext_proc filter apply header mutations.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google yanjunxiang-google requested a review from snowp as a code owner May 22, 2023 17:39
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

/assign @yanavlasov @htuch @mpwarres @stevenzzzz

@yanjunxiang-google
Copy link
Contributor Author

yanjunxiang-google commented May 22, 2023

This is a fuzzer issue:

OSS fuzzer issue link: https://oss-fuzz.com/testcase-detail/5309367839490048

Accepting input from '[STDIN]'

  | Usage for fuzzing: honggfuzz -P [flags] -- /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds-honggfuzz_envoy_13d8ff3fd8b6e12ff5bbd32d951c40c9e1c6513f/revisions/ext_proc_unit_test_fuzz
  | [2023-05-22 14:48:58.839][2428][critical][assert] [./envoy/http/header_map.h:71] assert failure: valid().
  | AddressSanitizer:DEADLYSIGNAL
  | =================================================================
  | ==2428==ERROR: AddressSanitizer: ABRT on unknown address 0x05390000097c (pc 0x7a908660600b bp 0x7ffdaf9e1c90 sp 0x7ffdaf9e19b0 T0)
  | SCARINESS: 10 (signal)
  | #0 0x7a908660600b in raise /build/glibc-SzIz7B/glibc-2.31/sysdeps/unix/sysv/linux/raise.c:51:1
  | #1 0x7a90865e5858 in abort /build/glibc-SzIz7B/glibc-2.31/stdlib/abort.c:79:7
  | #2 0x1557b8f in Envoy::Http::LowerCaseString::LowerCaseString(std::__1::basic_string_view<char, std::__1::char_traits >) envoy/http/header_map.h:71:5
  | #3 0x1685664 in Envoy::Extensions::HttpFilters::ExternalProcessing::MutationUtils::applyHeaderMutations(envoy::service::ext_proc::v3::HeaderMutation const&, Envoy::Http::HeaderMap&, bool, Envoy::Extensions::Filters::Common::MutationRules::Checker const&, Envoy::Stats::Counter&) /proc/self/cwd/source/extensions/filters/http/ext_proc/mutation_utils.cc:43:27
  | #4 0x165dc14 in Envoy::Extensions::HttpFilters::ExternalProcessing::ProcessorState::handleHeadersResponse(envoy::service::ext_proc::v3::HeadersResponse const&) /proc/self/cwd/source/extensions/filters/http/ext_proc/processor_state.cc:87:31
  | #5 0x163ae9e in Envoy::Extensions::HttpFilters::ExternalProcessing::Filter::onReceiveMessage(std::__1::unique_ptr<envoy::service::ext_proc::v3::ProcessingResponse, std::__1::default_deleteenvoy::service::ext_proc::v3::ProcessingResponse >&&) /proc/self/cwd/source/extensions/filters/http/ext_proc/ext_proc.cc:609:41
  | #6 0x14c2fbf in operator() /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:86:31
  | #7 0x14c2fbf in __invoke<(lambda at test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:82:28) &, envoy::service::ext_proc::v3::ProcessingRequest, bool> /usr/local/include/c++/v1/type_traits:3592:23
  | #8 0x14c2fbf in __call<(lambda at test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:82:28) &, envoy::service::ext_proc::v3::ProcessingRequest, bool> /usr/local/include/c++/v1/__functional/invoke.h:61:9
  | #9 0x14c2fbf in operator() /usr/local/include/c++/v1/__functional/function.h:181:16
  | #10 0x14c2fbf in std::__1::__function::__func<Envoy::Extensions::HttpFilters::ExtProc::UnitTestFuzz::TestOneProtoInput(envoy::extensions::filters::http::ext_proc::unit_test_fuzz::ExtProcUnitTestCase const&)::$_0::operator()(Envoy::Extensions::HttpFilters::ExternalProcessing::ExternalProcessorCallbacks&, envoy::config::core::v3::GrpcService const&, Envoy::StreamInfo::StreamInfo const&) const::'lambda'(envoy::service::ext_proc::v3::ProcessingRequest&&, bool), std::__1::allocator<Envoy::Extensions::HttpFilters::ExtProc::UnitTestFuzz::TestOneProtoInput(envoy::extensions::filters::http::ext_proc::unit_test_fuzz::ExtProcUnitTestCase const&)::$_0::operator()(Envoy::Extensions::HttpFilters::ExternalProcessing::ExternalProcessorCallbacks&, envoy::config::core::v3::GrpcService const&, Envoy::StreamInfo::StreamInfo const&) const::'lambda'(envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>, void (envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>::operator()(envoy::service::ext_proc::v3::ProcessingRequest&&, bool&&) /usr/local/include/c++/v1/__functional/function.h:355:12
  | #11 0x1586861 in operator() /usr/local/include/c++/v1/__functional/function.h:508:16
  | #12 0x1586861 in operator() /usr/local/include/c++/v1/__functional/function.h:1185:12
  | #13 0x1586861 in ApplyImpl<const std::__1::function<void (envoy::service::ext_proc::v3::ProcessingRequest &&, bool)> &, std::__1::tuple<envoy::service::ext_proc::v3::ProcessingRequest &&, bool>, 0UL, 1UL> /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:420:10
  | #14 0x1586861 in Apply<const std::__1::function<void (envoy::service::ext_proc::v3::ProcessingRequest &&, bool)> &, std::__1::tuple<envoy::service::ext_proc::v3::ProcessingRequest &&, bool> > /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:429:10
  | #15 0x1586861 in Perform /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-actions.h:497:12
  | #16 0x1586861 in PerformAction<void (envoy::service::ext_proc::v3::ProcessingRequest &&, bool)> /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-spec-builders.h:1441:12
  | #17 0x1586861 in testing::internal::FunctionMocker<void (envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>::UntypedPerformAction(void const*, void*) const /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-spec-builders.h:1556:12
  | #18 0x50c8339 in testing::internal::UntypedFunctionMockerBase::UntypedInvokeWith(void*) /proc/self/cwd/external/com_google_googletest/googlemock/src/gmock-spec-builders.cc:452:24
  | #19 0x1590021 in testing::internal::FunctionMocker<void (envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>::Invoke(envoy::service::ext_proc::v3::ProcessingRequest&&, bool) /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-spec-builders.h:1593:15
  | #20 0x1585db3 in Envoy::Extensions::HttpFilters::ExtProc::UnitTestFuzz::MockStream::send(envoy::service::ext_proc::v3::ProcessingRequest&&, bool) /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/mocks.h:20:3
  | #21 0x162023f in Envoy::Extensions::HttpFilters::ExternalProcessing::Filter::onHeaders(Envoy::Extensions::HttpFilters::ExternalProcessing::ProcessorState&, Envoy::Http::RequestOrResponseHeaderMap&, bool) /proc/self/cwd/source/extensions/filters/http/ext_proc/ext_proc.cc:177:12
  | #22 0x16225a6 in Envoy::Extensions::HttpFilters::ExternalProcessing::Filter::decodeHeaders(Envoy::Http::RequestHeaderMap&, bool) /proc/self/cwd/source/extensions/filters/http/ext_proc/ext_proc.cc:195:23
  | #23 0x15c247b in Envoy::Http::FilterHeadersStatus Envoy::Extensions::HttpFilters::HttpFilterFuzzer::sendHeadersEnvoy::Http::StreamDecoderFilter(Envoy::Http::StreamDecoderFilter*, test::fuzz::HttpData const&, bool) /proc/self/cwd/test/extensions/filters/http/common/fuzz/http_filter_fuzzer.h:145:46
  | #24 0x14d648f in void Envoy::Extensions::HttpFilters::HttpFilterFuzzer::runDataEnvoy::Http::StreamDecoderFilter(Envoy::Http::StreamDecoderFilter*, test::fuzz::HttpData const&) /proc/self/cwd/test/extensions/filters/http/common/fuzz/http_filter_fuzzer.h:102:31
  | #25 0x14bf07a in TestOneProtoInput /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:93:10
  | #26 0x14bf07a in LLVMFuzzerTestOneInput /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:38:1
  | #27 0x519eedb in main
  | #28 0x7a90865e7082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
  | #29 0x140142d in _start
  |  
  | AddressSanitizer can not provide additional info.
  | SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
  | ==2428==ABORTING

Accepting input from '[STDIN]'
Usage for fuzzing: honggfuzz -P [flags] -- /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds-honggfuzz_envoy_13d8ff3fd8b6e12ff5bbd32d951c40c9e1c6513f/revisions/ext_proc_unit_test_fuzz
[2023-05-22 14:48:58.839][2428][critical][assert] [./envoy/http/header_map.h:71] assert failure: valid().
AddressSanitizer:DEADLYSIGNAL

==2428==ERROR: AddressSanitizer: ABRT on unknown address 0x05390000097c (pc 0x7a908660600b bp 0x7ffdaf9e1c90 sp 0x7ffdaf9e19b0 T0)
SCARINESS: 10 (signal)
#0 0x7a908660600b in raise /build/glibc-SzIz7B/glibc-2.31/sysdeps/unix/sysv/linux/raise.c:51:1
#1 0x7a90865e5858 in abort /build/glibc-SzIz7B/glibc-2.31/stdlib/abort.c:79:7
#2 0x1557b8f in Envoy::Http::LowerCaseString::LowerCaseString(std::__1::basic_string_view<char, std::__1::char_traits >) envoy/http/header_map.h:71:5
#3 0x1685664 in Envoy::Extensions::HttpFilters::ExternalProcessing::MutationUtils::applyHeaderMutations(envoy::service::ext_proc::v3::HeaderMutation const&, Envoy::Http::HeaderMap&, bool, Envoy::Extensions::Filters::Common::MutationRules::Checker const&, Envoy::Stats::Counter&) /proc/self/cwd/source/extensions/filters/http/ext_proc/mutation_utils.cc:43:27
#4 0x165dc14 in Envoy::Extensions::HttpFilters::ExternalProcessing::ProcessorState::handleHeadersResponse(envoy::service::ext_proc::v3::HeadersResponse const&) /proc/self/cwd/source/extensions/filters/http/ext_proc/processor_state.cc:87:31
#5 0x163ae9e in Envoy::Extensions::HttpFilters::ExternalProcessing::Filter::onReceiveMessage(std::__1::unique_ptr<envoy::service::ext_proc::v3::ProcessingResponse, std::__1::default_deleteenvoy::service::ext_proc::v3::ProcessingResponse >&&) /proc/self/cwd/source/extensions/filters/http/ext_proc/ext_proc.cc:609:41
#6 0x14c2fbf in operator() /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:86:31
#7 0x14c2fbf in __invoke<(lambda at test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:82:28) &, envoy::service::ext_proc::v3::ProcessingRequest, bool> /usr/local/include/c++/v1/type_traits:3592:23
#8 0x14c2fbf in __call<(lambda at test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:82:28) &, envoy::service::ext_proc::v3::ProcessingRequest, bool> /usr/local/include/c++/v1/__functional/invoke.h:61:9
#9 0x14c2fbf in operator() /usr/local/include/c++/v1/__functional/function.h:181:16
#10 0x14c2fbf in std::__1::__function::__func<Envoy::Extensions::HttpFilters::ExtProc::UnitTestFuzz::TestOneProtoInput(envoy::extensions::filters::http::ext_proc::unit_test_fuzz::ExtProcUnitTestCase const&)::$_0::operator()(Envoy::Extensions::HttpFilters::ExternalProcessing::ExternalProcessorCallbacks&, envoy::config::core::v3::GrpcService const&, Envoy::StreamInfo::StreamInfo const&) const::'lambda'(envoy::service::ext_proc::v3::ProcessingRequest&&, bool), std::__1::allocator<Envoy::Extensions::HttpFilters::ExtProc::UnitTestFuzz::TestOneProtoInput(envoy::extensions::filters::http::ext_proc::unit_test_fuzz::ExtProcUnitTestCase const&)::$_0::operator()(Envoy::Extensions::HttpFilters::ExternalProcessing::ExternalProcessorCallbacks&, envoy::config::core::v3::GrpcService const&, Envoy::StreamInfo::StreamInfo const&) const::'lambda'(envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>, void (envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>::operator()(envoy::service::ext_proc::v3::ProcessingRequest&&, bool&&) /usr/local/include/c++/v1/__functional/function.h:355:12
#11 0x1586861 in operator() /usr/local/include/c++/v1/__functional/function.h:508:16
#12 0x1586861 in operator() /usr/local/include/c++/v1/__functional/function.h:1185:12
#13 0x1586861 in ApplyImpl<const std::__1::function<void (envoy::service::ext_proc::v3::ProcessingRequest &&, bool)> &, std::__1::tuple<envoy::service::ext_proc::v3::ProcessingRequest &&, bool>, 0UL, 1UL> /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:420:10
#14 0x1586861 in Apply<const std::__1::function<void (envoy::service::ext_proc::v3::ProcessingRequest &&, bool)> &, std::__1::tuple<envoy::service::ext_proc::v3::ProcessingRequest &&, bool> > /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:429:10
#15 0x1586861 in Perform /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-actions.h:497:12
#16 0x1586861 in PerformAction<void (envoy::service::ext_proc::v3::ProcessingRequest &&, bool)> /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-spec-builders.h:1441:12
#17 0x1586861 in testing::internal::FunctionMocker<void (envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>::UntypedPerformAction(void const*, void*) const /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-spec-builders.h:1556:12
#18 0x50c8339 in testing::internal::UntypedFunctionMockerBase::UntypedInvokeWith(void*) /proc/self/cwd/external/com_google_googletest/googlemock/src/gmock-spec-builders.cc:452:24
#19 0x1590021 in testing::internal::FunctionMocker<void (envoy::service::ext_proc::v3::ProcessingRequest&&, bool)>::Invoke(envoy::service::ext_proc::v3::ProcessingRequest&&, bool) /proc/self/cwd/external/com_google_googletest/googlemock/include/gmock/gmock-spec-builders.h:1593:15
#20 0x1585db3 in Envoy::Extensions::HttpFilters::ExtProc::UnitTestFuzz::MockStream::send(envoy::service::ext_proc::v3::ProcessingRequest&&, bool) /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/mocks.h:20:3
#21 0x162023f in Envoy::Extensions::HttpFilters::ExternalProcessing::Filter::onHeaders(Envoy::Extensions::HttpFilters::ExternalProcessing::ProcessorState&, Envoy::Http::RequestOrResponseHeaderMap&, bool) /proc/self/cwd/source/extensions/filters/http/ext_proc/ext_proc.cc:177:12
#22 0x16225a6 in Envoy::Extensions::HttpFilters::ExternalProcessing::Filter::decodeHeaders(Envoy::Http::RequestHeaderMap&, bool) /proc/self/cwd/source/extensions/filters/http/ext_proc/ext_proc.cc:195:23
#23 0x15c247b in Envoy::Http::FilterHeadersStatus Envoy::Extensions::HttpFilters::HttpFilterFuzzer::sendHeadersEnvoy::Http::StreamDecoderFilter(Envoy::Http::StreamDecoderFilter*, test::fuzz::HttpData const&, bool) /proc/self/cwd/test/extensions/filters/http/common/fuzz/http_filter_fuzzer.h:145:46
#24 0x14d648f in void Envoy::Extensions::HttpFilters::HttpFilterFuzzer::runDataEnvoy::Http::StreamDecoderFilter(Envoy::Http::StreamDecoderFilter*, test::fuzz::HttpData const&) /proc/self/cwd/test/extensions/filters/http/common/fuzz/http_filter_fuzzer.h:102:31
#25 0x14bf07a in TestOneProtoInput /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:93:10
#26 0x14bf07a in LLVMFuzzerTestOneInput /proc/self/cwd/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc:38:1
#27 0x519eedb in main
#28 0x7a90865e7082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
#29 0x140142d in _start
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
==2428==ABORTING

@yanjunxiang-google
Copy link
Contributor Author

Test case:

config {
grpc_service {
envoy_grpc {
cluster_name: "{"
}
}
stat_prefix: "sr..&......e.p"
mutation_rules {
allow_envoy {
}
disallow_all {
}
}
}
request {
}
response {
request_headers {
response {
header_mutation {
remove_headers: "\0 002"
remove_headers: "+d"
remove_headers: "\184017"
remove_headers: "+d"
}
}
}
dynamic_metadata {
}
}

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM mostyl modulo comments. Needs a better description in commit message.

source/extensions/filters/http/ext_proc/mutation_utils.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/ext_proc/mutation_utils.cc Outdated Show resolved Hide resolved
@stevenzzzz
Copy link
Contributor

LGTM, thanks for the catch.

Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

/retest

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo nit.

source/extensions/filters/http/ext_proc/ext_proc.cc Outdated Show resolved Hide resolved
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanavlasov yanavlasov merged commit 2b3fbe5 into envoyproxy:main May 25, 2023
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
…#27547)

* ASSERT(valid()) when ext_proc filter apply header mutations.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants