Path normalization controls

[htuch]

The fix for CVE-2019-9900, provided a coarse grained ability to opt-in/out of path normalization. That is, you could either normalize, in which case both normalization was used for matching and transforming the path to the upstream, or not normalize.

Ideally we provide finer grained controls similar to Nginx, where users can opt to normalize for match independent of transforming the path to the upstream.

CC @PiotrSikora

Action item for CVE-2019-9901

[htuch]

Bumping priority, since today we still have normalization disabled by default.

[htuch]

Once we have this, we should ensure that normalization occurs by default.

[twghu]

Looking at this along with #6588

[twghu]

Work on finding a library for normalisation progessed on #6588 seems tied to the solution for this. Will monitor and action pending outcome of same.

[htuch]

@twghu related but not dependent, I think we can add in the controls with the existing PathUtil interface.

[jonnysgomes]

Hi @htuch , I'm using Istio and I'm having an issue related with path traversal. The Envoy version is 1.12. Istio brings the normalize path configuration enabled by default, as we can see here. Even with that configuration enabled, I still can navigate through the url path, for instance:

https://my.domain.net/myservice/../otherplace

According with this link, Envoy already has fixed that. I've checked out the normalize_path flag and it's enabled. By my understanding, the url above should be resolved as https://my.domain.net/myservice/. So, Could it be a bad Envoy behavior or am I missing something?

[PiotrSikora]

@jonnysgomes normalized https://my.domain.net/myservice/../otherplace is https://my.domain.net/otherplace, not https://my.domain.net/myservice.

Without normalization, /myservice/../otherplace is matched against /myservice prefix, which is potentially a security issue, since you're really accessing /otherplace.

With normalization, /myservice/../otherplace is matched against /otherplace prefix, which is correct.

[twghu]

Implementation:

Requirements:

1. All existing behavior will be preserved.
2. Normalization should occur by default ( following previous comment htuch 22/1 ).
3. Code changes will be restricted to the upstream connection only with no changes to routing code to eliminate introduction of new security/function bugs.

Changes:

1. Add a configuration option to HttpConnectionManager normalize_path_for_upstream with default to value of normalize_path (support existing behavior).
2. Add a new header entry OriginalPathForUpstream, This will default to Path() as per current functionality. If normalize_path_for_upstream is false only then will OriginalPathForUpstream be passed to upstream non-normalized. This allows Envoy the option to use a normalized path from downstream to perform route matching while optionally allowing the non-normalized path to be passed through to upstream. The header entry is not forwarded to upstream and is private to Envoy. The addition of this new entry provides visibility to other internal functionality that may have an interest in the upstream path while minimizing code changes in the code base.
3. If the normalize_path configuration is false, then no normalization occurs for upstream (support existing behavior) by default. If normalize_path_for_upstream is set to true then the upstream path can be normalized independently.

[htuch]

This seems like a good design to me. @PiotrSikora @mattklein123 thoughts?

You'll need to be careful about dumping OriginalPathForUpstream on the wire by accident, or reaching upstream code with it not set (e.g. from an sync client).

[mattklein123]

Sorry I'm having a hard time wrapping my head around ^ without seeing it in context with the existing config. Can you potentially do a config/docs only PR and we can discuss the proposal there?

[twghu]

Hey @mattklein123 See #11111

[howardjohn]

Is this only about path? Or would something like #14491 fall in scope? Essentially allowing the original Host header (with port) to be forwarded while routing without the port internally. The current behavior forwards the modified host header, much like the path normalization

[howardjohn]

Another similar question as above: Is header case normalization in scope? I can open a new issue if its both desired and out of scope. istio/istio#30579 for reference

[htuch]

I think #8463 is tracking this one @howardjohn.

[asraa]

For observability, if we split this path we may want to default log the original path (that contains the most information, normalized paths can be recovered from the original). We should add formatters like %UPSTREAM_PATH% or %NORMALIZED_PATH% in case users want a way to view the upstream or internal normalization for matching.

[htuch]

Yes, I think original path should be default as anyone doing audit logging will want to see that.

[wrowe]

For observability, if we split this path we may want to default log the original path (that contains the most information, normalized paths can be recovered from the original). We should add formatters like %UPSTREAM_PATH% or %NORMALIZED_PATH% in case users want a way to view the upstream or internal normalization for matching.

I was considering this upstream as well. While there is always a risk passing on an unsanitized, unnormalized URI, there might be security applications which want to know the original URI. It would make sense to be able to pass these as an x-original-uri header field (with appropriate header value quoting and escaping, obviously.)