Dns resolve fuzz bug by chaoqin-li1123 · Pull Request #17107 · envoyproxy/envoy

chaoqin-li1123 · 2021-06-23T15:41:19Z

Commit Message: The DNS resolver takes reference to a local variable as its member dns_resolver_options_, and may access invalid data during a DNS refresh, fix by copy the parameter in ctor of DnsResolverImpl.(https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35272)
Additional Description:NA
Risk Level:low
Testing:add fuzz corpus
Docs Changes:NA
Release Notes:NA
Platform Specific Features:NA
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

asraa · 2021-06-23T17:44:58Z

Could you please add the link to the bug in Fixes description?

asraa

Is there an easy way to test the refresh?

chaoqin-li1123 · 2021-06-23T20:06:24Z

I don't know how to reproduce dns refresh in an easier way, we do have a few related fuzz bugs(35136, 34907) caused by this issue tho.

chaoqin-li1123 · 2021-07-09T16:17:29Z

/retest

repokitteh-read-only · 2021-07-09T16:17:33Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17107 (comment) was created by @chaoqin-li1123.

see: more, trace.

github-actions · 2021-08-08T20:01:23Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

chaoqin-li1123 · 2021-08-11T02:35:38Z

/retest

repokitteh-read-only · 2021-08-11T02:35:41Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17107 (comment) was created by @chaoqin-li1123.

see: more, trace.

…esolve_fuzz_bug

asraa

@ggreenway can you please take a final pass? (or if you have any ideas for testing/or if this makes sense to you)

ggreenway

I think that an integration test with a dns cluster, pointed at a non-existent tcp resolver, will reproduce the issue in an ASAN build. It looks like currently the options are usually on the stack of the caller. They are used the first time in DnsResolverImpl::DnsResolverImpl while the stack-based config is still in scope, but then they can be used again if (status == ARES_ECONNREFUSED). Can you try to add an integration test that fails without your fix?

/wait

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 · 2021-08-11T20:21:35Z

I think that an integration test with a dns cluster, pointed at a non-existent tcp resolver, will reproduce the issue in an ASAN build. It looks like currently the options are usually on the stack of the caller. They are used the first time in DnsResolverImpl::DnsResolverImpl while the stack-based config is still in scope, but then they can be used again if (status == ARES_ECONNREFUSED). Can you try to add an integration test that fails without your fix?

/wait

we can make the dns_resolver_options in DnsImplTest a variable on stack to create a regression asan test.

ggreenway

/wait

ggreenway · 2021-08-12T13:32:49Z

test/common/network/dns_impl_test.cc

        Network::Test::getCanonicalLoopbackAddress(GetParam()));
    listener_ = dispatcher_->createListener(socket_, *server_, true);
    updateDnsResolverOptions();
+    envoy::config::core::v3::DnsResolverOptions dns_resolver_options = dns_resolver_options_;


Can you add a comment here explaining what we're testing by putting this on the stack?

makes sense.

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

ggreenway

Thanks!

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

* main: Fix for fuzz tests failing due to invalid corpus paths (envoyproxy#17767) kafka: fix integration test (envoyproxy#17764) Fix typo in cluster.proto (envoyproxy#17755) cluster manager: add drainConnections() API (envoyproxy#17747) kafka broker filter: move to contrib (envoyproxy#17750) quiche: switch external dependency to github (envoyproxy#17732) quiche: implement stream idle timeout in codec (envoyproxy#17674) Update c-ares to 1.17.2 (envoyproxy#17704) Fix dns resolve fuzz bug (envoyproxy#17107) Remove members that shadow members of the base class (envoyproxy#17713) thrift proxy: missing parts from the previous PR (envoyproxy#17668) thrift-proxy: cleanup ConnectionManager::ActiveRpc (envoyproxy#17734) listener: extra warning for deprecated use_proxy_proto field (envoyproxy#17736) kafka: add support for metadata request in mesh-filter (envoyproxy#17597) upstream: add all host map to priority set for fast host searching (envoyproxy#17290) Remove the support for `hidden_envoy_deprecated_per_filter_config` (envoyproxy#17725) tls: SDS support for private key providers (envoyproxy#16737) bazel: update rules_foreign_cc (envoyproxy#17445) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

chaoqin-li1123 added 4 commits June 23, 2021 03:54

add test corpus

e758737

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

copy param to member

f2ec599

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

clean up corpus

6a12107

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

clean up corpus

01d9797

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

alyssawilk assigned asraa Jun 23, 2021

asraa reviewed Jun 23, 2021

View reviewed changes

lizan added the waiting label Jun 30, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 8, 2021

Merge branch 'main' of https://github.com/envoyproxy/envoy into dns_r…

93f6524

…esolve_fuzz_bug

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 11, 2021

repokitteh-read-only bot removed the waiting label Aug 11, 2021

asraa previously approved these changes Aug 11, 2021

View reviewed changes

asraa reviewed Aug 11, 2021

View reviewed changes

ggreenway requested changes Aug 11, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 11, 2021

ggreenway self-assigned this Aug 11, 2021

add regression test for asan build

f069f85

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 dismissed asraa’s stale review via f069f85 August 11, 2021 20:19

repokitteh-read-only bot removed the waiting label Aug 11, 2021

ggreenway requested changes Aug 12, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 12, 2021

add comment

a0691e0

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

repokitteh-read-only bot removed the waiting label Aug 12, 2021

ggreenway approved these changes Aug 12, 2021

View reviewed changes

ggreenway merged commit a7de9b8 into envoyproxy:main Aug 17, 2021

yanjunxiang-google added a commit to yanjunxiang-google/envoy that referenced this pull request Aug 17, 2021

merge conflicts with commits: Dns resolve fuzz bug envoyproxy#17107

37380ce

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

yanjunxiang-google mentioned this pull request Aug 17, 2021

Refactoring Envoy DNS resolution as extension #17479

Merged

suniltheta mentioned this pull request Sep 8, 2021

dns: renumber dns filter fields for wire compatibility (#17943) #18018

Merged

Conversation

chaoqin-li1123 commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asraa commented Jun 23, 2021

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

chaoqin-li1123 commented Jun 23, 2021

Uh oh!

chaoqin-li1123 commented Jul 9, 2021

Uh oh!

repokitteh-read-only bot commented Jul 9, 2021

Uh oh!

github-actions bot commented Aug 8, 2021

Uh oh!

chaoqin-li1123 commented Aug 11, 2021

Uh oh!

repokitteh-read-only bot commented Aug 11, 2021

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

chaoqin-li1123 commented Aug 11, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

chaoqin-li1123 Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chaoqin-li1123 commented Jun 23, 2021 •

edited

Loading