dns_filter: disable malformed DNS messages to dns filter by botengyao · Pull Request #20744 · envoyproxy/envoy

botengyao · 2022-04-08T20:49:53Z

Signed-off-by: Boteng Yao boteng@google.com

Commit Message: Disable malformed DNS messages to dns parser.
Additional Description: Only allow query message without any Answer, Authority, and Additional RRs to pass the parser check.
Risk Level: Low
Testing: unit test
Docs Changes:
Release Notes: Previously, the parser will return true when client sends a malformed message with question count == 1 and answer count == 1, which is not expected. The new behavior will only accept queries with Question count == 1, and all other Answer RRs count == 0. Otherwise, a format error code will be sent back.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Boteng Yao <boteng@google.com>

repokitteh-read-only · 2022-04-08T20:49:57Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #20744 was opened by botengyao.

see: more, trace.

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao · 2022-04-09T03:35:49Z

/retest

repokitteh-read-only · 2022-04-09T03:35:53Z

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

🐱

Caused by: a #20744 (comment) was created by @botengyao.

see: more, trace.

yanjunxiang-google · 2022-04-11T19:24:09Z

/assign @yanjunxiang-google

pradeepcrao

Thanks so much for taking care of this Boteng!

yanjunxiang-google · 2022-04-12T23:22:02Z

/assign @abaptiste

yanjunxiang-google · 2022-04-13T12:58:53Z

-  // resource records, where the number of records is specified in the corresponding count field in
-  // the header.
-
-  // Parse Answer Records and Additional Resource Records. This is primarily used for tests


It's understandable that the DNS client request message should not contain answer records. Just brainstorming, what's the right behavior if it does contain that? Should we simply ignore it? Reading through RFC1035, I don't see any text to describe this situation.

Good point. RFC1035 is fuzzy and there is no clear statement for this scenario. But I found the RFC1034 6.2.1 query example, and RFC1035 7.3, stating that - Check the header for reasonableness. Discard datagrams which are queries when responses are expected. for response processing. For query we can have the similar process based on the query type. Note, this is not for inverse query. The inverse query type can include the answers with empty questions.

For our dns filter, the parsed answers are not used for the main query logic but only for testing.

yanjunxiang-google · 2022-04-13T13:01:56Z

@@ -193,8 +193,11 @@ bool DnsMessageParser::parseDnsObject(DnsQueryContextPtr& context,
    }


If as below, that we are simply ignoring parsing the Answers/authority/additional RRs in a DNS client request, should we still set these states? Or after parsing questions section, then directly return?

If as below, that we are simply ignoring parsing the Answers/authority/additional RRs in a DNS client request, should we still set these states? Or after parsing questions section, then directly return?

We should still parse them I believe, because this state machine is to parse header to get the meta data like the number of questions/answers/authority/additional RRs. We can check if the request is expected based the header data, and then data are parsed from buffer like a byte stream.

yanjunxiang-google · 2022-04-14T20:14:30Z

+  return EMPTY_STRING;
+}
+
+DnsQueryContextPtr DnsResponseValidator::createResponseContext(Network::UdpRecvData& client_request,


Is this function is added for test only? And is it to test some testing code, not production code?
I am thinking we should just test createQueryContext().

This function is to parse the DNS response message, and it is used to verify the response generated by DNS filter, which we want to test. Basically, the query and response are sharing the same message format, this is only to parse and verify the response.

I understand what you mean now. So, the test is to send a request, get the response and parse the response and verify it is expected. The original code is using createQueryContext() to parse the response since that logic is embedded there. With the new code, createResponseContext is added to specially handle the response. Also, in the real world, Envoy won't parse the response. Only the DNS client who sends the request will parse the response. Thus createResponseContext() is only called in Envoy test code.

With that, I think the only thing I recommend is to add a few new test cases to verify the new logic --- bail out early and return false if answer is detected in query, which you added in createQueryContext()->parseDnsObject().

More generally, I'm wondering if there is some way to actually replace this custom code with c-ares? I bet there is some way we could pipe the response into c-ares and have it parse it. IMO That would be better than maintaining this code just for testing purposes. Maybe add a TODO around this also?

Good point, DNS filter only supports A, AAAA, SRV records, so we could use c-ares methods like ares_parse_a_reply, ares_parse_aaaa_reply, and ares_parse_srv_reply.

For now, the answer RRs are parsed by c-ares not the overall message format. This is only for test and validation, IMO it is okay to keep as it is, but will add a TODO.

yanjunxiang-google · 2022-04-14T20:15:39Z

  requestResponseWithListenerAddress(*listener_address, query, response);

-  query_ctx_ = response_parser_->createQueryContext(response, counters_);
+  query_ctx_ = response_parser_->createResponseContext(response, counters_);


I think the test code should test the production code, which is createQueryContext(). The test code should not test the testing specific code logic. Same as all below.

Right, the response is already got from the DNS filter, which is from requestResponseWithListenerAddress(*listener_address, query, response). This line is used to verify the response generated by the DNS filter.

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao · 2022-04-16T11:16:53Z

/retest

repokitteh-read-only · 2022-04-16T11:16:57Z

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

🐱

Caused by: a #20744 (comment) was created by @botengyao.

see: more, trace.

botengyao · 2022-04-19T02:23:48Z

Gentle ping for any comments.

Signed-off-by: Boteng Yao <boteng@google.com>

yanjunxiang-google · 2022-04-19T19:49:03Z

LGTM

botengyao · 2022-04-20T15:58:37Z

Hi @mattklein123, can someone take a further look and merge this PR?

mattklein123

Thanks for the cleanup. LGTM with small comments.

/wait

mattklein123 · 2022-04-20T16:59:41Z

+  return EMPTY_STRING;
+}
+
+DnsQueryContextPtr DnsResponseValidator::createResponseContext(Network::UdpRecvData& client_request,


More generally, I'm wondering if there is some way to actually replace this custom code with c-ares? I bet there is some way we could pipe the response into c-ares and have it parse it. IMO That would be better than maintaining this code just for testing purposes. Maybe add a TODO around this also?

Signed-off-by: Boteng Yao <boteng@google.com>

mattklein123 · 2022-04-21T16:20:54Z

+    ENVOY_LOG(
+        debug,
+        "One or more of Answers [{}], Authority [{}], and Additional [{}] RRs present in the query."
+        "Inverse query is not supported",
+        static_cast<int>(context->header_.answers),
+        static_cast<int>(context->header_.authority_rrs),
+        static_cast<int>(context->header_.additional_rrs));


This is not what I meant (I meant adding a filter stat/metric for this failure case that can be monitored), but if you want to keep that that is fine.

botengyao · 2022-04-21T16:35:06Z

Thank you for approving!

…20744) Signed-off-by: Boteng Yao <boteng@google.com>

Commit Message: Additional Description: I believe it is stable now since we conduct a security review and improvement for dns filter like #18651 #20744 #22861 #17479 #34409 #34456 #34490 and so on Risk Level: low Testing: Docs Changes: Release Notes: Signed-off-by: Boteng Yao <boteng@google.com>

botengyao added 4 commits April 8, 2022 15:41

Disable malformed DNS messages to filter

2713531

Signed-off-by: Boteng Yao <boteng@google.com>

Change comments and delete unused code

61e9448

Signed-off-by: Boteng Yao <boteng@google.com>

Add two unit tests for query record parser

efaa84d

Signed-off-by: Boteng Yao <boteng@google.com>

Checked some minor typos

340843f

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao marked this pull request as ready for review April 8, 2022 20:51

botengyao requested a review from mattklein123 as a code owner April 8, 2022 20:51

botengyao changed the title ~~dns_filter: disable malformed DNS messages to dns parser~~ dns_filter: disable malformed DNS messages to dns filter Apr 8, 2022

fix format check issues

3bc1415

Signed-off-by: Boteng Yao <boteng@google.com>

yanavlasov assigned pradeepcrao Apr 11, 2022

repokitteh-read-only Bot assigned yanjunxiang-google Apr 11, 2022

pradeepcrao reviewed Apr 12, 2022

View reviewed changes

Comment thread test/extensions/filters/udp/dns_filter/dns_filter_test.cc Outdated

repokitteh-read-only Bot assigned abaptiste Apr 12, 2022

yanjunxiang-google reviewed Apr 13, 2022

View reviewed changes

yanjunxiang-google reviewed Apr 14, 2022

View reviewed changes

botengyao added 4 commits April 15, 2022 13:56

Merge remote-tracking branch 'upstream/main' into dnsparserfix

259d70c

Signed-off-by: Boteng Yao <boteng@google.com>

fix typos and change to static methods

91ec522

Signed-off-by: Boteng Yao <boteng@google.com>

remove unused defination

04694f7

Signed-off-by: Boteng Yao <boteng@google.com>

Merge remote-tracking branch 'upstream/main' into dnsparserfix

28ca0f1

Signed-off-by: Boteng Yao <boteng@google.com>

yanjunxiang-google reviewed Apr 19, 2022

View reviewed changes

Comment thread source/extensions/filters/udp/dns_filter/dns_parser.cc

Add more test cases and change comments

42de2f1

Signed-off-by: Boteng Yao <boteng@google.com>

yanjunxiang-google previously approved these changes Apr 19, 2022

View reviewed changes

mattklein123 self-assigned this Apr 20, 2022

mattklein123 requested changes Apr 20, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 20, 2022

Refine log message and add todos

8d35793

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao dismissed yanjunxiang-google’s stale review via 8d35793 April 20, 2022 18:48

repokitteh-read-only Bot removed the waiting label Apr 20, 2022

botengyao added 3 commits April 20, 2022 18:49

Merge remote-tracking branch 'upstream/main' into dnsparserfix

1519d91

Signed-off-by: Boteng Yao <boteng@google.com>

Fix typos in log

564fcbe

Signed-off-by: Boteng Yao <boteng@google.com>

Fix typos in comments debug

6c5b660

Signed-off-by: Boteng Yao <boteng@google.com>

mattklein123 reviewed Apr 21, 2022

View reviewed changes

mattklein123 approved these changes Apr 21, 2022

View reviewed changes

mattklein123 merged commit 02f28f4 into envoyproxy:main Apr 21, 2022

botengyao mentioned this pull request May 5, 2022

dns_filter: simpilfy dns filter fuzz tests #21166

Merged

ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022

dns_filter: disable malformed DNS messages to dns filter (envoyproxy#…

2e7cddc

…20744) Signed-off-by: Boteng Yao <boteng@google.com>

lobkovilya mentioned this pull request Aug 12, 2022

DNS filter rejects requests that have QUERY + ADDITIONAL #22676

Closed

botengyao mentioned this pull request Dec 11, 2024

dns_filter: change dns filter to stable #37618

Merged

		@@ -193,8 +193,11 @@ bool DnsMessageParser::parseDnsObject(DnsQueryContextPtr& context,
		}

Conversation

botengyao commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Apr 8, 2022

Uh oh!

botengyao commented Apr 9, 2022

Uh oh!

repokitteh-read-only Bot commented Apr 9, 2022

Uh oh!

yanjunxiang-google commented Apr 11, 2022

Uh oh!

pradeepcrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao commented Apr 16, 2022

Uh oh!

repokitteh-read-only Bot commented Apr 16, 2022

Uh oh!

botengyao commented Apr 19, 2022

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 19, 2022

Uh oh!

botengyao commented Apr 20, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao commented Apr 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

botengyao commented Apr 8, 2022 •

edited

Loading

botengyao Apr 13, 2022 •

edited

Loading

botengyao Apr 20, 2022 •

edited

Loading