Aws::ProcessCredentials and security warning

[phene]

When the security warning was added about passing a single string argument to Kernel#system, someone reported and @mullermp created a PR suppress the warning when using the credential_process configuration from a profile. Because of the way this configuration is loaded, it will not detect arguments and produce a "proper" array of arguments, and still leverages the "insecure" call to Kernel#system, despite not warning of that. For example, if I have a profile like

[default]
credential_process = fetch-aws-credentials --arg1 foo --arg2 bar

SharedProfile will still load that as the string "fetch-aws-credentials --arg1 foo --arg2 bar", and despite wrapping it in an array, when it's splatted to Kernel#system as system("fetch-aws-credentials --arg1 foo --arg2 bar", out: w), we still end up with a single commandline argument, which this library is labeling as "insecure."

Is the intent here that we should implicitly trust the contents of the a shared profile configuration and explicitly distrust other programmatic uses of ProcessCredentials? Regardless, having a single-element array argument is functionally equivalent to having a string argument. If we really care about protecting ProcessCredentials from command-injection, we probably need to:

1. Raise the warning for single-element arrays if they have whitespace
2. Implementing parsing of credentials_process values in SharedProfile to produce an array of command + arguments.

I don't don't think anyone is really interested in trying to do (2), which would more likely than not introduce its own command injection vulnerabilities. On the other hand, bypassing the warning is trivially easy, to the point where I question if it improves security. Maybe it does only by virtue of getting someone to investigate why the warning occurs? (e.g. now)

Just wanted to raise this discussion and get people thinking about it. Maybe all that we can reasonably improve is adding more to the error message about the underlying issue or a link to some documentation about the issue, with a clearer explanation of why the warning exists and what questions a developer should ask themselves about where the arguments come from and whether or not they should be trusted.

[phene]

Another thing to consider is that this is likely an issue in all SDKs that try to spawn processes. Is there a broader discussion to be had in AWS about how SDKs should generally handle this? I understand not wanting to break compatibility, but maybe credential_process should be deprecated in favor of a new profile config option should take its place which provides a more structured value?

[mullermp]

Hey, thanks for reaching out. This one was an organizational ask. The idea was to prevent command injection possibilities. Shared configuration (~/.aws/config and credentials) parses everything a string, so it has to come in that way. It is very very very unlikely that it can be parsed as anything other than that. Hopefully the warning should be suppressed in this case because there's no way around this. You are correct that the contents of shared configuration is assumed trusted, at least from my point of view - far worse things can happen if an attacker had access to the machine. It's more focused towards in-code usage, i.e. creating a ProcessCredentials object yourself, we would prefer a list of arguments.

[phene]

Thanks, @mullermp. Can we just address this with better error messaging/documentation/guidance for those who run into the issue?

[mullermp]

What would you suggest? We're happy to take a pull request if you want to do a quick one.

[alextwoods]

Thanks for opening this - its a good discussion.

I believe in general, there is less concern about the shared credentials file as a vector of injection - its assumed that an attacker that has access to change that file or manipulate the ENV has other vectors of attack, so I don't think we need to print this warning when process credentials are used from the shared credentials file. Note: there is a slight compatibility issue, some other SDKs (eg, python) which do not execute these commands in a shell will fail on process credential configurations that chain commands, eg: cmd1 && cmd2, but this isn't a security concern.

Where there is a greater risk of injection is when constructing ProcessCredentials from user input, eg: ProcessCredentials.new("mycnd --accountId #{account_id_from_user_input}" - and this is where that warning I think is most useful.