Ensure secrets ID does not contain invalid file name characters

[LordMike]

A quick test showed that I could trick "dotnet user-secrets" to operate outside the defined directory (in the Roaming folder).

By setting the userSecretsId property in the project.json file to f.ex.

"userSecretsId": "..",

.. the tool now reads in the parent folder to where it normally would operate. This can be extended as necessary to go further up and into other directories, and even to other drives.

The parameter probably shouldn't accept paths in general (just a file name, or exclude elements not allowed in file names).

[natemcmaster]

This is technically a valid use of user secrets. Altering this behavior could be a breaking change.

Thoughts? @divega @muratg

cref the code: https://github.com/aspnet/Configuration/blob/a8b6008d3798693d03bb00d35238ce251c267ab7/src/Microsoft.Extensions.Configuration.UserSecrets/PathHelper.cs#L86-L114

[Genbox]

@natemcmaster Setting the userSecretsId to "../../../../Evil.json" escapes the %AppData% folder, which does not seem to be intended behavior to me. It is in fact a security vulnerability classified as a Directory Traversal.

[Eilon]

Only the app developer can set the userSecretsId, but then the app developer can just call File.Open("../../whatever.json") anyway. So, there is no security bug here that we can see.

There is, however, perhaps a bug here, where maybe there could be more rules about valid user secrets ids.

[natemcmaster]

We already validate the user id against Path.GetInvalidPathChars(). We could add / and \ to the set of invalid chars.

[Eilon]

@natemcmaster that does seem reasonable. We of course never intended those to be permitted.

[divega]

I wonder if using a PhysicalFileProvider in the right path (e.g. Environment.GetEnvironmentVariable("APPDATA") ?? Environment.GetEnvironmentVariable("HOME")) instead of parsing and flowing the id as an arbitrary path would lead to a more robust solution. We end up using PhysicalFileProvider anyway.

[Eilon]

I'm also fine if we just close this bug 😄

[natemcmaster]

Proposed fix aspnet/Configuration#558