JENKINS-46670: Escape quotes in environment variables #218

MattLud · 2017-09-06T22:42:24Z

Simple escape for quotes on environment variables to avoid dangling multi-line variables during 'export' part of container.

marvinthepa · 2017-09-08T09:14:21Z

Does this break with already escaped quotes?

See https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Escaping_JavaScript_escapes.

Would be nice if you could add a test for that.

IMHO it is always better to use a battle tested library for stuff like that, instead of implementing it with string replace..

marvinthepa

Please use a method of escaping that takes into account already escaped values..

marvinthepa · 2017-09-08T10:20:21Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java

@@ -212,7 +212,7 @@ public void onClose(int i, String s) {
                        environmentExpander.expand(envVars);
                        for (Map.Entry<String, String> entry : envVars.entrySet()) {
                            watch.getInput().write(
-                                    String.format("export %s=\"%s\"%s", entry.getKey(), entry.getValue(), NEWLINE).getBytes(StandardCharsets.UTF_8));
+                                    String.format("export %s=\"%s\"%s", entry.getKey(), entry.getValue().replace("\"","\\\""), NEWLINE).getBytes(StandardCharsets.UTF_8));


I checked, this is vulnerable to double escaping:

podTemplate(name: "horst", label: "label", containers: [ containerTemplate(name: 'ubuntu', image: 'ubuntu', ttyEnabled: true, command: 'cat', ) ]) { withEnv(['C="C', 'D=D"', 'E=\\"E', 'F=F\\"']) { node("label") { container('ubuntu') { sh '''echo "C='$C'" ''' sh '''echo "D='$D'" ''' sh '''echo "E='$E'" ''' sh '''echo "F='$F'" ''' } } } }

leads to

[full] Running shell script + echo C='"C' C='"C' [Pipeline] sh [full] Running shell script + echo D='D"' D='D"' [Pipeline] sh [full] Running shell script + echo E='\E export F=F\' E='\E export F=F\' [Pipeline] sh [full] Running shell script + echo F='' F=''

marvinthepa · 2017-09-08T10:23:33Z

Note: corrected the example, '\\' is necessary to trigger this.

While not necessarily a vulnerability, users might trigger unexpected errors when using escaped quotes.

MattLud · 2017-09-08T17:01:29Z

Agreed - switched to using StringEscapeUtils and added test cases.

marvinthepa · 2017-09-11T11:09:58Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java

                    }

                    if (environmentExpander != null) {
                        EnvVars envVars = new EnvVars();
                        environmentExpander.expand(envVars);
                        for (Map.Entry<String, String> entry : envVars.entrySet()) {
                            watch.getInput().write(
-                                    String.format("export %s=\"%s\"%s", entry.getKey(), entry.getValue(), NEWLINE).getBytes(StandardCharsets.UTF_8));
+                                    String.format("export %s=\"%s\"%s", entry.getKey(),  StringEscapeUtils.escapeJavaScript(entry.getValue()), NEWLINE).getBytes(StandardCharsets.UTF_8));


This escapes single quotes as well, which

a) does not even work in bash
b) is wrong as we are already in double quotes (%s=\"%s\"%s")

see e.g. a password that contains a single quote (stupid idea, but happens), where it will be encoded as \' (literally).

example pipeline:

podTemplate(name: "horst", label: "label", containers: [ containerTemplate(name: 'ubuntu', image: 'ubuntu', ttyEnabled: true, command: 'cat', ) ]) { withEnv(["C='C'", 'D=\'', 'E= ', 'F=\n']) { node("label") { container('ubuntu') { sh '''#!/bin/bash -x ENC='Jw=='; [[ $D = $(base64 -d <<<$ENC) ]] ''' } } } }

Note: 'Jw==' is a single quote base64 encoded.

The single quote part can be fixed by using escapeJava instead of escapeJavaScript, but I don't know if that will have more issues. It also escapes "quotes and control-chars (tab, backslash, cr, ff, etc.)", see Javadoc. It also escapes unicode characters, see source code :-).

Escaping is hard, especially for bash. Maybe it makes sense to add more tests, especially regarding control chars and unicode.

Back now and able to look at this - I had originally thought of the issue of blasting these out to build console log and thought to use the javascript to escape - how wrong that was!

re: xss - I check and the console log viewer escapes already

export HACK="<script>alert('XSS);</script>"

becomes

export HACK="<script>alert('XSS');<\/script>"

so I think we should be good there unless I'm not seeing it right.

To sum up: We want to only escape double quotes if they exist in the environment variable, not anything else with it - otherwise, we're guessing at what the user REALLY meant to escape. If the user provided an escaped quote such as

environment { //note that pipeline requires that extra \ quote = 'Testing-\\"-qoute' }

it comes to bash as
quote = 'Testing-"-qoute'
and we should provide the same functionality as the other executors provide by putting executing export quote = "Testing-\"-qoute"
and seeing
quote = 'Testing-"-qoute'

So I'm not sure where to go from here other than making naive assumption to ONLY escape a double quote in the provided variable then let bash double quotes handle the rest.

XSS is not an issue here, I never wanted to hint at that. OWASP is just a nice resource for escaping issues of any kind 😃 .

So I'm not sure where to go from here

I would suggest to use escapeJava instead of escapeJavascript, and add a few test cases to make sure that whitespace characters (tab, backslash, cr, ff, etc.) are encoded correctly. See https://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/StringEscapeUtils.html#escapeJava(java.lang.String) for reference.

scoheb · 2017-10-11T19:45:29Z

@MattLud See https://issues.jenkins-ci.org/browse/JENKINS-46651

MattLud · 2017-10-11T20:49:02Z

This is updated now - I'll have the candidate fix for "global properties" variables behind it since they'll use that common escape.

carlossg · 2017-10-12T07:15:22Z

@marvinthepa looks like your feedback has been addressed, can we merge? I'd like to cut a new release

marvinthepa · 2017-10-12T08:33:55Z

@MattLud looks better.

Did you check what happens with whitespace/control characters?

See https://commons.apache.org/proper/commons-lang/javadocs/api-3.1/org/apache/commons/lang3/StringEscapeUtils.html#escapeJava(java.lang.String):

Deals correctly with quotes and control-chars (tab, backslash, cr, ff, etc.)

So a tab becomes the characters '\' and 't'.

marvinthepa · 2017-10-12T08:36:43Z

Looks like this is one cause for "script returned exit code -1", see https://issues.jenkins-ci.org/browse/JENKINS-46651

marvinthepa · 2017-10-12T12:44:04Z

@carlossg I would still like to see a test that verifies the correct handling of how tabs, newlines and such.

I am all for a new release, though, I'll check manually. Could we also have #232 in the release?

marvinthepa · 2017-10-12T13:57:24Z

@carlossg @MattLud whitespace is not correctly handled:

podTemplate(name: "horst", label: "label", containers: [
                        containerTemplate(name: 'ubuntu',
                                image: 'ubuntu',
                                ttyEnabled: true,
                                command: 'cat',
                        )
]) {
    withEnv(["NL=before newline\nafter newline", 'TAB=before tab	after tab', '''F=before newline
after newline''']) {
        node("label") {
            container('ubuntu') {
                sh '''#!/bin/bash -x
echo NL $NL
echo TAB $TAB
echo F $F
'''
            }
        }
    }
}

leads to

[full] Running shell script
+ echo NL before 'newline\nafter' newline
NL before newline\nafter newline
+ echo TAB before 'tab\tafter' tab
TAB before tab\tafter tab
+ echo F before 'newline\nafter' newline
F before newline\nafter newline

I.e., a literal newline becomes the characters '' and 'n', while it should be a literal newline.

marvinthepa · 2017-10-12T14:20:33Z

@MattLud

Maybe escapeXsi is more useful here?

@carlossg would it be okay to add an apache commons-text dependency?

MattLud · 2017-10-12T19:25:29Z

EscapeXSI was no better -

F='before\ newlineafter\ newline'
NL='before\ newlineafter\ newline'
TAB='before\ tab\	after\ tab'

marvinthepa · 2017-10-13T09:47:53Z

That is annoying. If only kubernetes would provide an API to set environment variables directly, we wouldn't have this problem..

marvinthepa · 2017-10-13T12:43:12Z

~~Although this goes against all I said earlier, this seems to work to only escape double quotes, and prevent double escaping:~~

/* entry.getValue().replaceAll("\\\\([\\s\\S])|(\")", "\\\\$1$2") */ // this is wrong

~~See https://gist.github.com/getify/3667624 for reference.~~

~~But I guess this needs extensive tests.~~

~~@MattLud do you have the time to try that out and/or write tests for this?~~

Edited: this was wrong, sorry about that.

scoheb · 2017-10-13T13:08:44Z

I noticed there is also another problem...it seems when you throw global libraries into the mix, it creates env vars for each one that gets loaded...

I see this in my logs:

export: `library.ci-pipeline.version=debug-int-tests': not a valid identifier

'ci-pipeline' is my library...and the version is my branch which is 'debug-int-tests'

I patched my env with something simple like:

key.replaceAll("\\.", "_").replaceAll("-", "_")

make sense?

marvinthepa · 2017-10-13T15:08:01Z

After a discussion with a security-aware colleague of mine, we came up with a different approach.

I will push it for you to review.

It involves a self-implemented escaper, but it is a very simple one.

marvinthepa · 2017-10-13T15:20:05Z

@scoheb:

when you throw global libraries into the mix

Could you provide a code sample, so I can reproduce it? Thanks.

I tried something like

@Library('horst.kevin') _

Before the whole pipeline, but it didn't trigger any error for me.

That way, we only need to escape single quotes, nothing else. Single quotes cannot be escaped using baslash, as that is treated literally inside single quotes. I.e. to get a literal single quotes, we have to break out of the single quotes and escape it outside. "Simple". So "it's cool" becomes 'it'\''s cool,i', Treated by the shell as the concatenation of "it", "'", and "s cool". See e.g. https://stackoverflow.com/a/1315213.

marvinthepa · 2017-10-13T15:31:13Z

@MattLud @carlossg please see 3b2132b.

scoheb · 2017-10-13T18:37:45Z

@marvinthepa

Trying to put together something for the env vars from libraries...nothing yet...

but, take a look at this: https://github.com/jenkinsci/workflow-cps-global-lib-plugin/blob/master/src/main/resources/org/jenkinsci/plugins/workflow/libs/LibraryConfiguration/help-name.html

marvinthepa · 2017-10-16T06:47:33Z

@scoheb: I was able to reproduce your problem (just using withEnv(["a.b=c"])), but just together with #232, so I suggest solving that problem there (or in a new PR that @MattLud might open for that).

@carlossg do you have time to review my change and merge to master, so that we can merge it into #232?

carlossg · 2017-10-16T07:17:42Z

@marvinthepa so this is ready to merge now?

carlossg · 2017-10-16T07:24:40Z

there's a findbugs error

[INFO] Format string should use %n rather than \n in org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator$1.setupEnvironmentVariable(EnvVars, ExecWatch) [org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator$1] At ContainerExecDecorator.java:[line 248] VA_FORMAT_STRING_USES_NEWLINE

scoheb · 2017-10-16T10:31:51Z

@marvinthepa were you also able to reproduce the "-1" script return error?

marvinthepa · 2017-10-16T15:05:49Z

@carlossg: findbugs error fixed, good to merge.

marvinthepa requested changes Sep 8, 2017

View reviewed changes

JENKINS-46670: Escape quotes in environment variables

6a196d3

MattLud force-pushed the JENKINS-46670 branch from 362019c to 6a196d3 Compare September 8, 2017 16:53

marvinthepa requested changes Sep 11, 2017

View reviewed changes

JENKINS-46670: change to java; add common method for exporting

c912545

carlossg approved these changes Oct 12, 2017

View reviewed changes

marvinthepa mentioned this pull request Oct 13, 2017

JENKINS-47225: add proc starter cmdenvs and listener to executing code #232

Closed

fix findbugs warning

a27fd94

marvinthepa force-pushed the JENKINS-46670 branch from cd9ad5a to a27fd94 Compare October 16, 2017 15:19

carlossg merged commit ecb8779 into jenkinsci:master Oct 16, 2017

MattLud deleted the JENKINS-46670 branch October 16, 2017 16:40

marvinthepa mentioned this pull request Oct 16, 2017

Jenkins 47225: Fix ProcStarter #236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-46670: Escape quotes in environment variables #218

JENKINS-46670: Escape quotes in environment variables #218

MattLud commented Sep 6, 2017

marvinthepa commented Sep 8, 2017

marvinthepa left a comment

marvinthepa Sep 8, 2017 •

edited

Loading

marvinthepa commented Sep 8, 2017 •

edited

Loading

MattLud commented Sep 8, 2017

marvinthepa Sep 11, 2017 •

edited

Loading

MattLud Sep 29, 2017 •

edited

Loading

marvinthepa Oct 9, 2017

marvinthepa Oct 9, 2017

scoheb commented Oct 11, 2017

MattLud commented Oct 11, 2017

carlossg commented Oct 12, 2017

marvinthepa commented Oct 12, 2017

marvinthepa commented Oct 12, 2017

marvinthepa commented Oct 12, 2017 •

edited

Loading

marvinthepa commented Oct 12, 2017

marvinthepa commented Oct 12, 2017

MattLud commented Oct 12, 2017 •

edited

Loading

marvinthepa commented Oct 13, 2017

marvinthepa commented Oct 13, 2017 •

edited

Loading

scoheb commented Oct 13, 2017

marvinthepa commented Oct 13, 2017

marvinthepa commented Oct 13, 2017 •

edited

Loading

marvinthepa commented Oct 13, 2017

scoheb commented Oct 13, 2017

marvinthepa commented Oct 16, 2017

carlossg commented Oct 16, 2017

carlossg commented Oct 16, 2017

scoheb commented Oct 16, 2017 •

edited

Loading

marvinthepa commented Oct 16, 2017

JENKINS-46670: Escape quotes in environment variables #218

JENKINS-46670: Escape quotes in environment variables #218

Conversation

MattLud commented Sep 6, 2017

marvinthepa commented Sep 8, 2017

marvinthepa left a comment

Choose a reason for hiding this comment

marvinthepa Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

marvinthepa commented Sep 8, 2017 • edited Loading

MattLud commented Sep 8, 2017

marvinthepa Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

MattLud Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

marvinthepa Oct 9, 2017

Choose a reason for hiding this comment

marvinthepa Oct 9, 2017

Choose a reason for hiding this comment

scoheb commented Oct 11, 2017

MattLud commented Oct 11, 2017

carlossg commented Oct 12, 2017

marvinthepa commented Oct 12, 2017

marvinthepa commented Oct 12, 2017

marvinthepa commented Oct 12, 2017 • edited Loading

marvinthepa commented Oct 12, 2017

marvinthepa commented Oct 12, 2017

MattLud commented Oct 12, 2017 • edited Loading

marvinthepa commented Oct 13, 2017

marvinthepa commented Oct 13, 2017 • edited Loading

scoheb commented Oct 13, 2017

marvinthepa commented Oct 13, 2017

marvinthepa commented Oct 13, 2017 • edited Loading

marvinthepa commented Oct 13, 2017

scoheb commented Oct 13, 2017

marvinthepa commented Oct 16, 2017

carlossg commented Oct 16, 2017

carlossg commented Oct 16, 2017

scoheb commented Oct 16, 2017 • edited Loading

marvinthepa commented Oct 16, 2017

marvinthepa Sep 8, 2017 •

edited

Loading

marvinthepa commented Sep 8, 2017 •

edited

Loading

marvinthepa Sep 11, 2017 •

edited

Loading

MattLud Sep 29, 2017 •

edited

Loading

marvinthepa commented Oct 12, 2017 •

edited

Loading

MattLud commented Oct 12, 2017 •

edited

Loading

marvinthepa commented Oct 13, 2017 •

edited

Loading

marvinthepa commented Oct 13, 2017 •

edited

Loading

scoheb commented Oct 16, 2017 •

edited

Loading