Envoy ASSERT() due to regex rewrite path string contains \r#23448
Envoy ASSERT() due to regex rewrite path string contains \r#23448htuch merged 1 commit intoenvoyproxy:mainfrom
Conversation
…characters. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
yanjunxiang-google
changed the title
|
/assign @adisuissa |
|
We are seeing an Envoy crash in ASSERT() with below trace: ==458==ERROR: AddressSanitizer: ABRT on unknown address 0x0539000001ca (pc 0x7f053ef4f18b bp 0x7ffc30b6cc90 sp 0x7ffc30b6c990 T0) And the configuration to trigger this is: route { |
|
/assign @htuch |
| // to capture group 2. | ||
| string substitution = 2; | ||
| string substitution = 2 | ||
| [(validate.rules).string = {well_known_regex: HTTP_HEADER_VALUE strict: false}]; |
There was a problem hiding this comment.
This is technically a breaking API change. I agree that \r in headers is invalid. Can you audit all the uses of RegexMatchAndSubstitute and figure out if HTTP_HEADER_VALUE is the right choice? I think some of them are paths, hosts and so on. It might turn out HTTP_HEADER_VALUE is sufficiently general to encompass them but I'm not sure, would be good to have a paper trail when making this kind of change.
Also, presumably we're only "crashing" in ASSERT mode, and this isn't a genuine security issue? Please clarify in the commit message.
There was a problem hiding this comment.
Alternatively, the validation can be done when processing the config in RouteEntryImplBase.
There was a problem hiding this comment.
Thanks for the comments!
There are six places using RegexMatchAndSubstitute:
-
3)
5)
6)
It looks to me all of them are trying to using the "substitution" string in the RegexMatchAndSubstitute to replace some parts of the HTTP header. Thus "substitution" should not contain '\r', '\n', '\0', which [(validate.rules).string = {well_known_regex: HTTP_HEADER_VALUE strict: false}] is trying to guard against.
Regarding the alternative to implement the logic in RouteEntryImplBase, I would think leverage the pgv is able to guard all the cases above. Also it's widely used, like:
or
Yeah, the crash only happens in ASSERT mode. Let me change the commit message. Also, it's triggered by incorrect Envoy configuration, which IIUC, can not be exploited by malicious client, thus I would think it's not a genuine security issue.
There was a problem hiding this comment.
Can RegexMatchAndSubstitute be used by the unified matcher?
If so, it may be used as an extension for a non-header regex substitution.
There was a problem hiding this comment.
That's a good question. I don't see it yet. The original commit for RegexMatchAndSubstitute is #10050, which just use it for RouteAction.
If in the future people want to use it as unified matcher, they are aware that NULL characters can not be in the "substitution". Otherwise, they have to add their own proto field.
There was a problem hiding this comment.
I don't think it's possible to use RegexMatchAndSubstitute as a matcher in the unified matching API, since the unified matching API separates matching from actions, and RegexMatchAndSubstitute combines the two. But @snowp can probably tell us for sure.
There was a problem hiding this comment.
cc @snowp can you shed a light on whether RegexMatchAndSubstitute can be used as a matcher?
Generally speaking, is there a list of what types can be used in the unified matcher (both matchers and actions)?
There was a problem hiding this comment.
As Mark said the unified matcher doesn't support any form of rewrite, it performs a stateless matching which results in an action, which then needs to be acted upon. There's certainly possibilities to make it support some kind of mutability or state propagation from the matcher, but it's not there today
Per https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/common/matcher/v3/matcher.proto#envoy-v3-api-msg-config-common-matcher-v3-matcher-matcherlist-predicate-singlepredicate there's either a value_match which uses a StringMatcher or a custom match which supports using a registered match extension like the ones listed in the docs
yanjunxiang-google
changed the title
|
LGTM - can I get another pair of eyes from @envoyproxy/api-shepherds to verify this technically breaking change is safe as there was no contractually valid way to use these rejected values? |
Per Snow's comment, and given that this type is only used for headers, I think this is a safe change. |
6 participants
It is observed that Envoy crash in ASSERT() due to regex rewrite path string contains invalid character '\r'.
We should prohibit null characters \0, \r, \n in the regex rewrite substitution string. This is well guarded in most other cases like RouteAction:prefix_rewrite, but is missing in RouteAction:regex_rewrite:substitution.
Signed-off-by: Yanjun Xiang yanjunxiang@google.com
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:]