[JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings #370

car-roll · 2020-07-28T22:57:13Z

PoC version 2 for groovy string interpolation interception.

This version does not require any modifications to core. Sensitive variables are registered in BindingStep via API from EnvironmentExtender. Scanning occurs in DSL.flattenGString().

Requires:
jenkinsci/credentials-binding-plugin#105
jenkinsci/workflow-step-api-plugin#57
jenkins-infra/jenkins.io#3814

This PR also subsumes jenkinsci/workflow-durable-task-step-plugin#132

Strings that have been interpolated through DSL.flattenGstring are now tracked through DSL.parseArgs() and stored in NamedArgsAndClosure. The interpolated strings are then compared against the list of sensitive variables from EnvironmentExpander via the api established in jenkinsci/workflow-step-api-plugin#57. Interpolated string warnings are displayed in the console output as well as the build page.

Special note: changes were made to DSL.invokeDescribable, specifically when there is no associated meta-step for a symbol (i.e. when building up a nested argument for a step). Previously, the method would return an UninstantiatedDescribable and it would be instantiated later in DSL.invokeStep. Now, the UninstantiatedDescribable is packed into NamedArgsAndClosure and later unpacked when invokeStep calls parseArgs.

The changes in ArgumentsActionImpl address Jennkins-47101. It uses the new workflow-step API to identify sensitive variables. The SAFE_ENVIRONMENT_VARIABLES list has been removed and instead of checking for safe variables, we now check for sensitive variables. Sensitive variables are no longer dropped with NotStoredReason.MASKED_VALUE, but are now replaced with the variables name (e.g. "echo s3cret" becomes "echo ${PASSWORD}"). This allows us to display the signature of the steps that contain interpolation warnings.
Example screenshot here:

dwnusbaum

Looking good! Added a few comments/suggestions.

pom.xml

src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java

src/main/java/org/jenkinsci/plugins/workflow/cps/EnvironmentWatcher.java

src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java

pom.xml

jglick · 2020-08-19T16:54:55Z

From a very brief glance I suspect this is what should also be extended to finally fix JENKINS-47101.

.../resources/org/jenkinsci/plugins/workflow/cps/view/EnvironmentWatcherRunReport/summary.jelly

jglick · 2020-09-04T15:37:11Z

be extended to finally fix JENKINS-47101

Let me know if you want suggestions about that. I do not think it would be out of scope: you look to be defining the same APIs which have long been proposed to clean up the way sensitive environment variables are handled in ArgumentsActionImpl, so it would be appropriate to verify that the API design is right by actually doing that cleanup. Basically, with the API in place, you should just be able to delete a bunch of tricky code and replace it with a short, obvious call.

dwnusbaum

Looking really close, but we need to fix the use of the system property and the accidental serialization of Run in the inner class of InterpolatedSecretsAction.

dwnusbaum · 2020-10-30T18:44:46Z

src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java

 /**
 * Calls {@link Step}s and other DSL objects.
 */
 @PersistIn(PROGRAM)
 public class DSL extends GroovyObjectSupport implements Serializable {
+    @SuppressFBWarnings("MS_SHOULD_BE_FINAL") // Used to control warning behavior of unsafe Groovy interpolation
+    @Restricted(NoExternalUse.class)
+    public static String UNSAFE_GROOVY_INTERPOLATION = DSL.class.getName() + ".UNSAFE_GROOVY_INTERPOLATION";


Right now this is just a string, you need to look up the property with this name, either here or in the places where you want to check the property's value, and I would set a default value so that there are no NPEs if the property is unset:

Suggested change

public static String UNSAFE_GROOVY_INTERPOLATION = DSL.class.getName() + ".UNSAFE_GROOVY_INTERPOLATION";

public static String UNSAFE_GROOVY_INTERPOLATION = System.getProperty(DSL.class.getName() + ".UNSAFE_GROOVY_INTERPOLATION", "warn");

src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java

dwnusbaum · 2020-10-30T18:59:21Z

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java

+    public static class InterpolatedWarnings {
+        final String stepName;
+        final List<String> interpolatedVariables;
+        final Run run;


Since InterpolatedWarnings is stored in a non-transient field in InterpolatedSecretsAction, the run field here must also be transient or else there will be issues. It looks like you do use the field in getStepSignature, so I think you could either delete the field and make that method take the Run as a parameter, make run transient here and add methods to restore it when onLoad and onAttached are called in the outer class, or maybe you could make InterpolatedWarnings a non-static inner class and use the transient run field from the parent directly (but I'm not sure if @ExportedBean works with non-static inner classes).

3e54d36
made Run transient. Confirmed stepSignature in api

dwnusbaum · 2020-10-30T19:01:11Z

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java

+            Map<String, ?> udArgs = ud.getArguments();
+            valueString = udArgs.entrySet().stream()
+                    .map(InterpolatedSecretsAction::argumentToString)
+                    .collect(Collectors.joining(", ", udName + "(", ")"));


This is right when using symbols, but when $class is used the syntax is a list like [$class: udName, args...].

(A map, not a list.)

updated in b52438d to handle $class. Not sure what I should be using to test those changes. I just looked up some examples I found online.

Basically any extension without a @Symbol.

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java

src/test/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsActionTest.java

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java

Remove @ symbol for UninstantiatedDescribable getStepSignature

dwnusbaum · 2020-11-04T17:21:05Z

src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java

+        List<InterpolatedSecretsAction.InterpolatedWarnings> warnings = reportAction.getWarnings();
+        MatcherAssert.assertThat(warnings.size(), is(1));
+        InterpolatedSecretsAction.InterpolatedWarnings stepWarning = warnings.get(0);
+        MatcherAssert.assertThat(stepWarning.getStepSignature(), is("archiveArtifacts(delegate: archiveArtifacts(<anonymous>: ${PASSWORD}))"));


This test and a few others shows there are still a few issues with InterpolatedWarnings.getStepSignature. It would be good to fix these, but I also think they are relatively unlikely to occur in practice with interpolated passwords, so maybe it would be better to merge this and handle that in a follow-up?

When using symbol-based syntax and the UninstantiatedDescribable only has one required parameter and there is only one provided argument (i.e. hasSoleRequiredParameter returns true), the value can be provided directly instead of using the key: value syntax, meaning that we should never see things like <anonymous>: in the output. In this particular case, fixing this issue (but not the other issue) would make the output archiveArtifacts(archiveArtifacts(${PASSWORD})), which is a bit closer to what was written in the Pipeline.

Meta steps like archiveArtifacts are handled incorrectly. For example, in this case the outer archiveArtifacts is actually the step step, but generally the outer step is should be omitted so only the inner describable is shown. The direct meta step invocation in DSL.invokeDescribable specifically exists to handle this case so users don't have to write use step explicitly. You can check for this case using StepDescriptor.isMetaStep (check that that FlowNode is an instance of StepNode and then call StepNode.getDescriptor) along with making sure that the argument is for the Describable type corresponding to the required parameter of the meta step, maybe with something like this in InterpolatedWarnings.getStepSignature:

FlowNode node; // Maybe return this when looking up step arguments? if (node instanceof StepNode) { StepDescriptor descriptor = ((StepNode)node).getDescriptor(); if (descriptor.isMetaStep()) { DescribableParameter p = DescribableModel.of(descriptor.clazz).getFirstRequiredParameter(); if (p != null) { Object arg = ArgumentsAction.getResolvedArguments(f).get(p.getName()); return argumentToString(arg); } } } // handle as normal step using existing code

Attempted to address comments in 7d8672c

car-roll · 2020-11-05T08:21:11Z

src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java

@@ -436,6 +437,7 @@ protected Object invokeDescribable(String symbol, Object _args) {
        NamedArgsAndClosure args = parseArgs(_args, metaStep!=null && metaStep.takesImplicitBlockArgument(),
                UninstantiatedDescribable.ANONYMOUS_KEY, singleArgumentOnly, new HashSet<>());
        UninstantiatedDescribable ud = new UninstantiatedDescribable(symbol, null, args.namedArgs);
+        ud.setModel(symbolModel);


Needed to set the DescribableModel in order for hasSoleRequiredArgument to work.

car-roll · 2020-11-05T08:24:01Z

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java

+                        if (arg instanceof UninstantiatedDescribable) {
+                            return argumentToString(arg);
+                        } else {
+                            return stepName + "(" + argumentToString(arg) + ")";


I know in the case of ArchiveArtifacts the inner argument was an UninstantiatedDescribable with a symbol so that it would print out the signature correctly. What I was less sure about, was if this was always going to be the case in these scenarios where there is an outer CoreStep.

dwnusbaum · 2020-11-05T22:10:43Z

pom.xml

@@ -101,7 +102,6 @@
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>script-security</artifactId>
-            <version>1.75</version>


Current version of the BOM makes this redundant.

dwnusbaum · 2020-11-05T22:10:57Z

pom.xml

@@ -93,6 +93,7 @@
        <dependency>
            <groupId>org.jenkins-ci.plugins.workflow</groupId>
            <artifactId>workflow-support</artifactId>
+            <version>3.6-rc759.3e4ba3120e7a</version> <!-- TODO: https://github.com/jenkinsci/workflow-support-plugin/pull/110 -->


That PR needs to be released before we merge and release this one.

dwnusbaum · 2020-11-05T22:11:19Z

src/test/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImplTest.java

@@ -621,6 +626,22 @@ public void testReallyUnusualStepInstantiations() throws Exception {
                equalTo(NotStoredReason.UNSERIALIZABLE));
    }

+    @Issue("JENKINS-47101")
+    @Test public void passwordParametersSanitized() throws Exception {


@car-roll This is the new test I added for jenkinsci/workflow-support-plugin#110.

src/test/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsActionTest.java

car-roll marked this pull request as draft July 28, 2020 23:03

This was referenced Jul 28, 2020

[JENKINS-63254][JENKINS-47101] Watch sensitive variables jenkinsci/credentials-binding-plugin#105

Merged

[JENKINS-63254][JENKINS-47101] add watch variables to EnvironmentExpander jenkinsci/workflow-step-api-plugin#57

Merged

car-roll added 6 commits July 30, 2020 20:17

added unit test showing leaked password

009bad3

working PoC

52f6f4b

add listener to report errors to pipeline output

75c7406

PoC 2 for groovy interpolation interception. Does not require core mods

191f08e

Wrap EnvironmentExpander and EnvVars together for parseArgs

ccf9040

use incrementals, revert jenkins version

5c53b94

car-roll force-pushed the interpolation-v2 branch from 906fe37 to 5c53b94 Compare July 31, 2020 03:17

car-roll changed the title ~~Interpolation v2~~ [JENKINS-63254] Insecure Groovy String Interpolation Warnings v2 Jul 31, 2020

car-roll added 2 commits July 30, 2020 20:37

code cleanup

c5ef8ad

catch null arguments

1a7793d

car-roll closed this Jul 31, 2020

car-roll reopened this Jul 31, 2020

Make unit test windows friendly. Remove dollar sign from password

0829025

car-roll requested review from dwnusbaum and bitwiseman July 31, 2020 17:48

dwnusbaum mentioned this pull request Jul 31, 2020

[PoC] Warn against Groovy string interpolation #369

Closed

car-roll added 2 commits August 4, 2020 09:19

update to use newer implementation of EnvironmentExpander

18653c0

Use updated api in EnvironmentExpander

5ba9b2a

dwnusbaum reviewed Aug 5, 2020

View reviewed changes

jglick self-requested a review August 19, 2020 16:54

car-roll added 3 commits August 27, 2020 11:55

address review comments

aeebf17

add factory method, add report action, and summary page

02804b3

change from table to list, change icon

20d781d

jglick reviewed Sep 2, 2020

View reviewed changes

.../resources/org/jenkinsci/plugins/workflow/cps/view/EnvironmentWatcherRunReport/summary.jelly Outdated Show resolved Hide resolved

update jelly formatting, update unit test

b404458

dwnusbaum reviewed Oct 30, 2020

View reviewed changes

car-roll added 3 commits November 2, 2020 14:29

address review comments

41e50e6

make InterpolatedWarnings.run transient field

3e54d36

update UninstantiatedDescribable $class toString

b52438d

car-roll closed this Nov 3, 2020

car-roll reopened this Nov 3, 2020

car-roll requested a review from dwnusbaum November 3, 2020 21:26

dwnusbaum reviewed Nov 3, 2020

View reviewed changes

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java Outdated Show resolved Hide resolved

update InterpolatedSecretesAction onLoad and onAttached

49695e0

Remove @ symbol for UninstantiatedDescribable getStepSignature

car-roll requested a review from dwnusbaum November 4, 2020 00:03

dwnusbaum reviewed Nov 4, 2020

View reviewed changes

Update getStepSignature to better reflect pipeline input

7d8672c

car-roll commented Nov 5, 2020

View reviewed changes

car-roll requested a review from dwnusbaum November 5, 2020 08:24

Remove printing of step signature

611326c

dwnusbaum mentioned this pull request Nov 5, 2020

[JENKINS-47101] Add a default EnvironmentExpander that registers password parameters as sensitive variables jenkinsci/workflow-support-plugin#110

Merged

car-roll and others added 2 commits November 5, 2020 14:28

remove setting the model for the Uninstantiated Describable

96c2f30

Make sure password parameters are masked in step arguments

7811148

dwnusbaum approved these changes Nov 5, 2020

View reviewed changes

dwnusbaum and others added 5 commits November 5, 2020 17:25

Remove InterpolatedSecretsActionTest.java

f7798af

Align workflow-support tests jar with incremental version

40eb64a

Update to latest workflow-support incremental

bc8d268

Merge remote-tracking branch 'upstream/master' into interpolation-v2

e173261

update pom, update changelog to prepare for release

4524d26

car-roll merged commit de5e69b into jenkinsci:master Nov 6, 2020

car-roll deleted the interpolation-v2 branch November 6, 2020 05:11

car-roll mentioned this pull request Nov 6, 2020

Prevent lease leak when ws or node is called with no body jenkinsci/workflow-durable-task-step-plugin#132

Closed

jglick mentioned this pull request Nov 6, 2020

[JEP-229] Generate temporary Artifactory tokens and submit them to GitHub repos for CD jenkins-infra/repository-permissions-updater#1747

Merged

7 tasks

car-roll mentioned this pull request Nov 17, 2020

[JENKINS-64185] Replace use of NamedArgsAndClosure as return value for invokeDescribable #399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings #370

[JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings #370

car-roll commented Jul 28, 2020 •

edited

Loading

dwnusbaum left a comment

jglick commented Aug 19, 2020

jglick commented Sep 4, 2020

dwnusbaum left a comment

dwnusbaum Oct 30, 2020

dwnusbaum Oct 30, 2020

car-roll Nov 3, 2020

dwnusbaum Oct 30, 2020

jglick Oct 30, 2020

car-roll Nov 3, 2020

jglick Nov 3, 2020

dwnusbaum Nov 4, 2020 •

edited

Loading

car-roll Nov 5, 2020

car-roll Nov 5, 2020

car-roll Nov 5, 2020

dwnusbaum Nov 5, 2020

dwnusbaum Nov 5, 2020

dwnusbaum Nov 5, 2020

	public static String UNSAFE_GROOVY_INTERPOLATION = DSL.class.getName() + ".UNSAFE_GROOVY_INTERPOLATION";
	public static String UNSAFE_GROOVY_INTERPOLATION = System.getProperty(DSL.class.getName() + ".UNSAFE_GROOVY_INTERPOLATION", "warn");

[JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings #370

[JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings #370

Conversation

car-roll commented Jul 28, 2020 • edited Loading

dwnusbaum left a comment

Choose a reason for hiding this comment

jglick commented Aug 19, 2020

jglick commented Sep 4, 2020

dwnusbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

car-roll commented Jul 28, 2020 •

edited

Loading

dwnusbaum Nov 4, 2020 •

edited

Loading