-
Notifications
You must be signed in to change notification settings - Fork 722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Leading white spaces in an env var set in the Settings cause unexpected results #4139
Comments
IMHO, It is not wise to drop leading white spaces or any white spaces for that matter. There may be a real need to have those white spaces in the value. Only the second proposed solution makes sense.
Both approaches get the correct output. The '*' guards are added just for display purposes.
|
Could you point to a legitimate use case of environment variables where the leading white space is required? I am of the opinion that it is more likely to be a user error than not. |
This is just a hypothetical situation. Let's say, I want to specify a
Output:
If the risk of exposing the value is what you concern here then you can safe-guard that using stderr redirection, like so.
The EDIT: Thus, you may probably want to employ both quoting/escaping + stderr redirection. |
Hmm. I am still not convinced. If a user needs spaces in their environment variables, secure or otherwise, it is easy to inject them by putting the value in quotes; e.g., https://travis-ci.org/BanzaiMan/travis_production_test/builds/68571201#L97 (Here, I put Redirect to |
Fair enough. Having said that, normally I would not trust my user input. So, properly quoting or escaping the user input before injecting it into my script is a proper thing to do. In the context of Travis worker VM, if a malicious user could find a way to inject other code to be executed in the escalated privilege, I suppose the user is just hurting him/herself as the VM image is only used by that one user before the VM image is reverted back to the original snapshot. Hopefully the exploit stops there and not breach further into the container. Too far fetch may be 😄. EDIT: have anybody tried to put this in the value |
If the user wants to run |
Yes, I know that. That is just one of the stupid example payload for user to shoot himself in his foot, but only when you are sure he cannot breach further in. Anyway, this is just hypothetical. I am a very happy Travis CI user so I cannot think of any reason to do anything nasty. |
I agree we should just trim leading/tailing whitespace when we store the variable. @drogus any chance you could have a look? I've tried making sense out of the Virtus stuff, and couldn't quite figure it out quickly. |
I think we have two different problems here: an UI issue and a security issue. The UI is not very friendly, it don't warn you about a no scaped string and neighter show you some tips about how it works. $ export SONATYPE_NEXUS_PASSWORD=[secure]
/home/travis/build.sh: eval: line 41: syntax error near unexpected token `)'
/home/travis/build.sh: eval: line 41: `export SONATYPE_NEXUS_PASSWORD=>~1I)"K&F'gb'&D' I saw this issue with my old password: |
@BraisGabin That sounds like a separate issue entirely, though. Yours is a case where the use of special characters resulted in revelation of the value. It is true, however, that we should do a better job of explaining how the values are processed. |
I was thinking about open a new one. I didn't because the main problem is the same: scape special characters like white space or |
Any updates on this issue? I am having this problem as my password is something like
I'd guess it is caused by the My build system is the following
|
@felipesabino In your case, quoting is best. |
This is actually not just a theoretical problem. |
The same problem occurs when a variable name contains a dot . |
OMG, serious security issue when builds are indexed by Google! I can see all my passwords in the logs. |
+1 for bumping the severity of this issue. I just inadvertently exposed my AWS secret key by copying and pasting it with extra spaces. Specifically, I was following the instructions for uploading build artifacts: https://docs.travis-ci.com/user/uploading-artifacts/ I set an ARTIFACTS_SECRET environment variable with value
(Note I've of course generated a new secret key value.) |
+1 Just had the exact same issue, although just a token was exposed that can be regenerated. I was not aware of unwanted whitespaces that inadvertently got added in front of the real secret key. As a security issue, this should definitely be addressed, hard to believe this has been pending for over one year already. |
+1 I had a related issue with a setting that was URL-like with The generated script should properly escape any embedded double-quotes in the setting string, and then double-quote that value. That way it will correctly handle leading/trailing whitespace, special characters and variable substitution. |
I'm closing this in favor of #1775. |
To reproduce:
If the first non-space character is a numeric, then the value will be exposed (as above). If it is an alphabet character, then it is silently ignored. This is due to the compiled bash script which looks like this:
and so on.
This is probably not what the user is expecting. If the value is really a secret, it will be exposed unexpectedly.
Possible solutions:
Whatever the solution, the user should be notified of the changes made (if any) to the value the user entered.
The text was updated successfully, but these errors were encountered: