Enhance path validation logic

[sebastienros]

Adding query arguments tests for the chars ?, /, %, #,

Adding these rules for path segment arguments:

• If the string contains '/' or '' anywhere, fail.
• If after calling string.Trim():
  • The string is empty, fail.
  • The string contains only '.' characters, fail.

This is because we don't think it's legitimate for users to be able to use these as valid path arguments since it could be provided by user data (tenant name, username, ...) and escaping could be tricky as servers might un-encode them.

I am not sure ArgumentException is the best here so please send suggestions.

Microsoft Reviewers: Open in CodeFlow

[geeknoid]

I'm not sure what we're trying to achieve here. This generator is producing a client library that calls an endpoint. Are we trying to protect the endpoint by preventing the client from sending specific strings? Is this really appropriate? The endpoint is the one that should defend against this, right?

What if the client legitimately needs to send those funny strings?

[sebastienros]

Are we trying to protect the endpoint by preventing the client from sending specific strings?

It's not to protect then endpoint, but to protect the application using the generated code from accessing endpoints that it's not supposed to. The code path it's changing uses method arguments to create the destination endpoint. If this argument is not sanitized then it could be formed in a way that the application would call other endpoints that could potentially be abused, leaking other information from the request, or altering another part of the system.

Simple example: your application intends to send a request to delete a product using /products/{productId}/delete, but the user of the app manages to pass a productId value of ../users/admin (this could come from a query string in your own app), which could then delete a user account instead on the destination system.

[GrabYourPitchforks]

For context, this is something that came up while threat modeling the AutoClient functionality. When it was presented in the threat modeling session, the scenario provided was that it was trying to simplify the client's ability to invoke these endpoints by allowing the client to perform the equivalent of format string building. Threat modeling asked if it was reasonable to expect that these inputs might come directly from untrusted user input (e.g., provided directly by a remote caller) and thus might be hostile, and the answer was yes.

So we end up with a path traversal-style attack. A malicious client tricks an innocent frontend into making unintended privileged requests to an access-controlled backend. Sebastien talked about this earlier at #4495 (comment). There are two mitigations recommended for this risk.

First, the arguments need to be properly escaped when the URI string is being built. (Sebastien already got to this in some previous PRs.)

Second, certain patterns - even after escaping - may still be dangerous. It's fairly common for web servers to collapse . and .. segments, or to collapse empty segments, or to unescape %2F and treat it as a normal path separator, or to normalize backslash to slash and to treat it as a directory separator, etc. So if a malicious caller gets one of these patterns into a path segment, it could still lead to path traversal. How you want to approach this depends on the philosophy behind how you're presenting this library to your consumers. If you're positioning this library as something "friendly" that callers don't need much experience to use correctly, then it's prudent to put some defensive checks in place to thwart these types of attacks. If you're positioning this library as something meant for power users and that it's really just a glorified string builder, then you're implicitly expecting your callers to know what values are [un]safe to put in any of these segments, and you should document this expectation so that callers explicitly know it.

The security review team was fine with either of the two approaches: defense-in-depth or document warnings for consumers. It's primarily a function of what audience you see consuming this library. Looks like this PR opts for the former.

[geeknoid]

If we assume that 99% of the times, the strings will not contain slashes or dots, can we make a fast trivial exit paths to reduce computation?

if (value.IndexOfAny(SlahesOrDots) < 0)
return true;

Or something like it.

[sebastienros]

Done

[sebastienros]

Waiting for approvals or feedback

[joperezr]

Changes look good to me. Thanks for following up on this @sebastienros