admin: Fix crashes on invalid FieldMasks by paul-r-gall · Pull Request #16979 · envoyproxy/envoy

paul-r-gall · 2021-06-14T21:11:29Z

Signed-off-by: Paul Gallagher pgal@google.com

Note: Though this is an unexpected crash, this change does not require the security process, as the admin interface already has documented methods of triggering a crash (/quitquitquit), and should always be protected anyway.

Commit Message: admin: Fix crashes on invalid FieldMasks
Additional Description: When a field mask is invalid, TrimMessage will LOG(FATAL). Since explicitly checking validity of a field mask via FieldMaskUtil::IsValidFieldMask for a message requires compile-time knowledge of the message type, the best we can do is catch the exception thrown by TrimMessage and return an error.
Risk Level: Low
Testing: Unit tests.
Docs Changes: None, this aligns actual behavior with documented behavior.

Signed-off-by: Paul Gallagher <pgal@google.com>

jmarantz · 2021-06-15T12:46:51Z

Heh, /quitquitquit should exit cleanly and not "crash" :)

Anyway I'll take a look at this.

jmarantz

At a high level this looks good, with a couple of small issues and one question.

Would it make sense to do the exception trapping in config_dump_Handler.cc -- just wrap the try around addAllConfigDump? I think that might be consistent with the way we filter bad regexes in the /stats handler, but I'll leave it to you to check :)

jmarantz · 2021-06-15T12:52:27Z

source/server/admin/config_dump_handler.cc

+        // trimResourceMessage will LOG(FATAL) if `field_mask` is malformed.
+        TRY_ASSERT_MAIN_THREAD { trimResourceMessage(field_mask, msg); }
+        END_TRY
+        catch (...) {


can you just catch a specific exception here? Looking at source/common/protobuf/utility.cc; I think you can use EnvoyException, which is the usual pattern.

Then you can also give the text from the exception (e.what() I think).

Actually, it seems as though this isn't an EnvoyException but instead a std::exception, and calling e.what() just returns "std::exception", which isn't particularly helpful.

Can you debug who is throwing this std::exception?

In theory no one in Envoy should be throwing an exception that is not derived from EnvoyException. However, if this comes from the STL then that won't be true.

This is coming from a LOG(FATAL) here.

Can we see what's going on up stack and make it not call LOG(FATAL)?

Can you use this utility inside trimResourceMessage after the unpack?

I meant protobuf. Thanks.

Unfortunately I don't think so; all messages are dynamically constructed from DynamicMessageFactory inside trimResourceMessage, and IsValidFieldMask is templated and requires compile-time knowledge of the proto type, e.g. IsValidFieldMask<Cluster>(mask).

Is there anything we can do to pre-validate the field_mask to avoid crashing in protobuf? Catching the throw that emanates from a LOG(FATAL) seems non-robust.

I added a method which essentially does the same thing as IsValidFieldMask but doesn't require compile-time knowledge of types. LMK if this works for you!

jmarantz · 2021-06-15T12:56:49Z

source/server/admin/config_dump_handler.cc

+        ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, message.get());
+      }
+      END_TRY
+      catch (...) {


catch (EnvoyException e)

jmarantz · 2021-06-15T12:58:55Z

test/server/admin/config_dump_handler_test.cc

+                header_map, response));
+  EXPECT_EQ("FieldMask paths: \"cluster.transport_socket_matches.name\"\n could not be "
+            "successfully used.",
+            response.toString());


can you also make sure that headerMap.ContentTtype == "text/plain" here? If that was somehow 'text/html' then the error message could contain an XSS.

I think there's a ContentTypeValues.XXX value for that.

jmarantz · 2021-06-15T12:59:26Z

test/server/admin/config_dump_handler_test.cc

+            getCallback("/config_dump?mask=bootstrap.node.extensions.name", header_map, response));
+  EXPECT_EQ("FieldMask paths: \"bootstrap.node.extensions.name\"\n could not be "
+            "successfully used.",
+            response.toString());


ditto: check for content-type: text/plain

jmarantz

/wait

Signed-off-by: Paul Gallagher <pgal@google.com>

jmarantz · 2021-06-21T19:09:47Z

source/server/admin/config_dump_handler.cc

-    addAllConfigToDump(dump, mask, include_eds);
+    auto err = addAllConfigToDump(dump, mask, include_eds);
+    if (err.has_value()) {
+      response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Text);


sorry can you also add

response_headers.addReference( Http::Headers::get().XContentTypeOptions, Http::Headers::get().XContentTypeOptionValues.Nosniff);

Just a little paranoia to make sure browsers don't flip into HTML mode if some text looks like HTML. Just a bit of paranoia based on past browser behavior :)

Signed-off-by: Paul Gallagher <pgal@google.com>

jmarantz

basically LGTM with a few minor nits you can take care of, but I'll turn it over to senior maintainers. Thanks for doing this!

/assign-from @envoyproxy/senior-maintainers

source/server/admin/config_dump_handler.cc

test/server/admin/config_dump_handler_test.cc

repokitteh-read-only · 2021-06-21T20:24:06Z

@envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #16979 (review) was submitted by @jmarantz.

see: more, trace.

Signed-off-by: Paul Gallagher <pgal@google.com>

zuercher

I think this looks good, modulo one nit and the clang-tidy errors. Those are

CheckFieldMaskAndTrimMessage vs checkFieldMaskAndTrimMessage
requiring braces around all control statements bodies (the if at line config_dump_handler.cc:110)

zuercher · 2021-06-22T16:24:57Z

source/server/admin/config_dump_handler.cc

 // below.
 //
+//
+//


nit: rm extra blank comment lines

Signed-off-by: Paul Gallagher <pgal@google.com>

zuercher

Thanks!

When a field mask is invalid, TrimMessage will LOG(FATAL). Since explicitly checking validity of a field mask via FieldMaskUtil::IsValidFieldMask for a message requires compile-time knowledge of the message type, the best we can do is catch the exception thrown by TrimMessage and return an error. Risk Level: Low Testing: Unit tests. Docs Changes: None, this aligns actual behavior with documented behavior. Release Notes: n/a Signed-off-by: Paul Gallagher <pgal@google.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

When a field mask is invalid, TrimMessage will LOG(FATAL). Since explicitly checking validity of a field mask via FieldMaskUtil::IsValidFieldMask for a message requires compile-time knowledge of the message type, the best we can do is catch the exception thrown by TrimMessage and return an error. Risk Level: Low Testing: Unit tests. Docs Changes: None, this aligns actual behavior with documented behavior. Release Notes: n/a Signed-off-by: Paul Gallagher <pgal@google.com>

Fix crash on invalid FieldMask

87af1e7

Signed-off-by: Paul Gallagher <pgal@google.com>

jmarantz self-assigned this Jun 15, 2021

jmarantz reviewed Jun 15, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 15, 2021

paul-r-gall added 2 commits June 16, 2021 12:20

check content type

316a5b2

Signed-off-by: Paul Gallagher <pgal@google.com>

prevalidate FieldMask and update headers

491d696

Signed-off-by: Paul Gallagher <pgal@google.com>

repokitteh-read-only bot removed the waiting label Jun 21, 2021

jmarantz reviewed Jun 21, 2021

View reviewed changes

add nosniff header

6761dd2

Signed-off-by: Paul Gallagher <pgal@google.com>

jmarantz previously approved these changes Jun 21, 2021

View reviewed changes

repokitteh-read-only bot assigned zuercher Jun 21, 2021

apply fixes

6101d83

Signed-off-by: Paul Gallagher <pgal@google.com>

paul-r-gall dismissed jmarantz’s stale review via 6101d83 June 22, 2021 13:18

zuercher reviewed Jun 22, 2021

View reviewed changes

source/server/admin/config_dump_handler.cc Outdated

// below.

//

//

//

Copy link
Copy Markdown

Member

zuercher Jun 22, 2021

Choose a reason for hiding this comment

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

nit: rm extra blank comment lines

fix formatting

4b95053

Signed-off-by: Paul Gallagher <pgal@google.com>

zuercher approved these changes Jun 23, 2021

View reviewed changes

zuercher merged commit fd39c79 into envoyproxy:main Jun 23, 2021

paul-r-gall deleted the FixConfigDumpCrash branch June 23, 2021 19:38

paul-r-gall mentioned this pull request Jul 1, 2021

admin: fix field mask parameter #17214

Merged

bartebor mentioned this pull request Jul 7, 2021

Segfault config_dump with invalid mask #17257

Closed

Conversation

paul-r-gall commented Jun 14, 2021

Uh oh!

jmarantz commented Jun 15, 2021

Uh oh!

jmarantz left a comment

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!

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!

jmarantz left a comment

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!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 21, 2021

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants