Skip to content

Commit 01be8ac

Browse files
yaroslavafenkinyaroslavafenkin
committed
[SECURITY-2949]
1 parent b96f0a4 commit 01be8ac

File tree

3 files changed

+144
-9
lines changed

3 files changed

+144
-9
lines changed

‎src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadPropertiesStepExecution.java‎

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,14 @@
2727
import edu.umd.cs.findbugs.annotations.NonNull;
2828
import hudson.FilePath;
2929
import hudson.model.TaskListener;
30+
import jenkins.util.SystemProperties;
3031
import org.apache.commons.configuration2.AbstractConfiguration;
3132
import org.apache.commons.configuration2.Configuration;
3233
import org.apache.commons.configuration2.ConfigurationConverter;
34+
import org.apache.commons.configuration2.interpol.ConfigurationInterpolator;
35+
import org.apache.commons.configuration2.interpol.DefaultLookups;
36+
import org.apache.commons.configuration2.interpol.InterpolatorSpecification;
37+
import org.apache.commons.configuration2.interpol.Lookup;
3338
import org.apache.commons.lang.StringUtils;
3439
import org.jenkinsci.plugins.pipeline.utility.steps.AbstractFileOrTextStepExecution;
3540
import org.jenkinsci.plugins.workflow.steps.StepContext;
@@ -49,6 +54,16 @@
4954
public class ReadPropertiesStepExecution extends AbstractFileOrTextStepExecution<Map<String, Object>> {
5055
private static final long serialVersionUID = 1L;
5156

57+
private static final Map<String, Lookup> SAFE_PREFIX_INTERPOLATOR_LOOKUPS = new HashMap<String, Lookup>() {{
58+
put(DefaultLookups.BASE64_DECODER.getPrefix(), DefaultLookups.BASE64_DECODER.getLookup());
59+
put(DefaultLookups.BASE64_ENCODER.getPrefix(), DefaultLookups.BASE64_ENCODER.getLookup());
60+
put(DefaultLookups.DATE.getPrefix(), DefaultLookups.DATE.getLookup());
61+
put(DefaultLookups.URL_DECODER.getPrefix(), DefaultLookups.URL_DECODER.getLookup());
62+
put(DefaultLookups.URL_ENCODER.getPrefix(), DefaultLookups.URL_ENCODER.getLookup());
63+
}};
64+
65+
static /* not final */ String CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS = SystemProperties.getString(ReadPropertiesStepExecution.class.getName() + ".CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS");
66+
5267
private transient ReadPropertiesStep step;
5368

5469
protected ReadPropertiesStepExecution(@NonNull ReadPropertiesStep step, @NonNull StepContext context) {
@@ -122,22 +137,31 @@ private void addAll(Map<Object, Object> src, Map<String, Object> dst) {
122137
private Properties interpolateProperties(Properties properties) throws Exception {
123138
if ( properties == null)
124139
return null;
125-
Configuration interpolatedProp;
126140
PrintStream logger = getLogger();
127141
try {
142+
ConfigurationInterpolator configurationInterpolator = ConfigurationInterpolator.fromSpecification(
143+
new InterpolatorSpecification.Builder()
144+
.withPrefixLookups(
145+
CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS == null ?
146+
SAFE_PREFIX_INTERPOLATOR_LOOKUPS :
147+
parseLookups(CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS)
148+
)
149+
.create()
150+
);
128151
// Convert the Properties to a Configuration object in order to apply the interpolation
129152
Configuration conf = ConfigurationConverter.getConfiguration(properties);
153+
conf.setInterpolator(configurationInterpolator);
130154

131155
// Apply interpolation
132-
interpolatedProp = ((AbstractConfiguration)conf).interpolatedConfiguration();
156+
Configuration interpolatedProp = ((AbstractConfiguration)conf).interpolatedConfiguration();
157+
158+
// Convert back to properties
159+
return ConfigurationConverter.getProperties(interpolatedProp);
133160
} catch (Exception e) {
134161
logger.println("Got exception while interpolating the variables: " + e.getMessage());
135162
logger.println("Returning the original properties list!");
136163
return properties;
137164
}
138-
139-
// Convert back to properties
140-
return ConfigurationConverter.getProperties(interpolatedProp);
141165
}
142166

143167
/**
@@ -150,4 +174,28 @@ private PrintStream getLogger() throws Exception {
150174
assert listener != null;
151175
return listener.getLogger();
152176
}
177+
178+
/*
179+
* Method was copied from https://github.com/apache/commons-configuration/blob/aff776e3d4d81f1f856304306353be3279aec11a/src/main/java/org/apache/commons/configuration2/interpol/ConfigurationInterpolator.java#L673-L687
180+
* licensed under https://github.com/apache/commons-configuration/blob/aff776e3d4d81f1f856304306353be3279aec11a/LICENSE.txt
181+
* and slightly modified.
182+
*/
183+
private static Map<String, Lookup> parseLookups(final String str) {
184+
final Map<String, Lookup> lookupMap = new HashMap<>();
185+
if (StringUtils.isBlank(str))
186+
return lookupMap;
187+
188+
try {
189+
for (final String lookupName : str.split("[\\s,]+")) {
190+
if (!lookupName.isEmpty()) {
191+
DefaultLookups lookup = DefaultLookups.valueOf(lookupName.toUpperCase());
192+
lookupMap.put(lookup.getPrefix(), lookup.getLookup());
193+
}
194+
}
195+
} catch (IllegalArgumentException exc) {
196+
throw new IllegalArgumentException("Invalid default lookups definition: " + str, exc);
197+
}
198+
199+
return lookupMap;
200+
}
153201
}

‎src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadPropertiesStep/help.html‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,17 @@
4747
</li>
4848
<li>
4949
<code>interpolate</code>:
50-
Flag to indicate if the properties should be interpolated or not.
51-
In case of error or cycling dependencies, the original properties will be returned.
50+
Flag to indicate if the properties should be interpolated or not. <br>
51+
52+
Prefix interpolations allowed by default are: <code>urlDecoder</code>, <code>urlEncoder</code>,
53+
<code>date</code>, <code>base64Decoder</code>, <code>base64Encoder</code>.
54+
55+
Default prefix interpolations can be overridden by setting the
56+
<a href="https://www.jenkins.io/redirect/setting-system-properties">system property</a>: <br>
57+
<code>org.jenkinsci.plugins.pipeline.utility.steps.conf.ReadPropertiesStepExecution.CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS</code><br>
58+
<b>Note that overriding default prefix interpolations can be insecure depending on which ones you enable.</b>
59+
60+
In case of error or cyclic dependencies, the original properties will be returned.
5261
</li>
5362
</ul>
5463
<p>
@@ -73,4 +82,4 @@
7382
assert props.fullUrl = 'http://localhost/README.txt'
7483
</pre>
7584
</code>
76-
</p>
85+
</p>

‎src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadPropertiesStepTest.java‎

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@
2828
import hudson.model.Result;
2929

3030
import static org.jenkinsci.plugins.pipeline.utility.steps.FilenameTestsUtils.separatorsToSystemEscaped;
31+
import static org.jenkinsci.plugins.pipeline.utility.steps.conf.ReadPropertiesStepExecution.CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS;
32+
import static org.junit.Assert.assertEquals;
33+
import static org.junit.Assert.assertNotEquals;
3134

3235
import org.jenkinsci.plugins.pipeline.utility.steps.Messages;
3336
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
@@ -37,6 +40,8 @@
3740
import org.junit.Rule;
3841
import org.junit.Test;
3942
import org.junit.rules.TemporaryFolder;
43+
import org.jvnet.hudson.test.FlagRule;
44+
import org.jvnet.hudson.test.Issue;
4045
import org.jvnet.hudson.test.JenkinsRule;
4146

4247
import java.io.File;
@@ -54,6 +59,8 @@ public class ReadPropertiesStepTest {
5459
public JenkinsRule j = new JenkinsRule();
5560
@Rule
5661
public TemporaryFolder temp = new TemporaryFolder();
62+
@Rule
63+
public FlagRule<String> customLookups = new FlagRule<>(() -> CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS, x -> CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS = x);
5764

5865
@Before
5966
public void setup() throws Exception {
@@ -341,4 +348,75 @@ public void readFileAndTextInterpolatedWithCyclicValues() throws Exception {
341348
"}", true));
342349
j.assertBuildStatusSuccess(p.scheduleBuild2(0));
343350
}
344-
}
351+
352+
@Issue("SECURITY-2949")
353+
@Test
354+
public void unsafeInterpolatorsDoNotInterpolate() throws Exception {
355+
Properties props = new Properties();
356+
props.setProperty("file", "${file:utf8:/etc/passwd}");
357+
props.setProperty("hax", "${script:Groovy:jenkins.model.Jenkins.get().systemMessage = 'pwn3d'}");
358+
File textFile = temp.newFile();
359+
try (FileWriter f = new FileWriter(textFile)) {
360+
props.store(f, "Pipeline test");
361+
}
362+
363+
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
364+
p.setDefinition(new CpsFlowDefinition(
365+
"node('slaves') {\n" +
366+
" def props = readProperties interpolate: true, file: '" + separatorsToSystemEscaped(textFile.getAbsolutePath()) + "'\n" +
367+
" assert props['file'] == '${file:utf8:/etc/passwd}'\n" +
368+
" assert props['hax'] == '${script:Groovy:jenkins.model.Jenkins.get().systemMessage = \\'pwn3d\\'}'\n" +
369+
"}", true));
370+
j.assertBuildStatusSuccess(p.scheduleBuild2(0));
371+
assertNotEquals("pwn3d", j.jenkins.getSystemMessage());
372+
}
373+
374+
@Issue("SECURITY-2949")
375+
@Test
376+
public void safeInterpolatorsDoInterpolate() throws Exception {
377+
Properties props = new Properties();
378+
props.setProperty("urld", "${urlDecoder:Hello+World%21}");
379+
props.setProperty("urle", "${urlEncoder:Hello World!}");
380+
props.setProperty("base64d", "${base64Decoder:SGVsbG9Xb3JsZCE=}");
381+
props.setProperty("base64e", "${base64Encoder:HelloWorld!}");
382+
File textFile = temp.newFile();
383+
try (FileWriter f = new FileWriter(textFile)) {
384+
props.store(f, "Pipeline test");
385+
}
386+
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
387+
p.setDefinition(new CpsFlowDefinition(
388+
"node('slaves') {\n" +
389+
" def props = readProperties interpolate: true, file: '" + separatorsToSystemEscaped(textFile.getAbsolutePath()) + "'\n" +
390+
" assert props['base64d'] == 'HelloWorld!'\n" +
391+
" assert props['base64e'] == 'SGVsbG9Xb3JsZCE='\n" +
392+
" assert props['urld'] == 'Hello World!'\n" +
393+
" assert props['urle'] == 'Hello+World%21'\n" +
394+
"}", true));
395+
j.assertBuildStatusSuccess(p.scheduleBuild2(0));
396+
}
397+
398+
@Issue("SECURITY-2949")
399+
@Test
400+
public void customLookupsOverrideDefaultInterpolators() throws Exception {
401+
CUSTOM_PREFIX_INTERPOLATOR_LOOKUPS = "script,base64_encoder";
402+
403+
Properties props = new Properties();
404+
props.setProperty("hax", "${script:Groovy:jenkins.model.Jenkins.get().systemMessage = 'pwn3d'}");
405+
props.setProperty("base64d", "${base64Decoder:SGVsbG9Xb3JsZCE=}");
406+
props.setProperty("base64e", "${base64Encoder:HelloWorld!}");
407+
File textFile = temp.newFile();
408+
try (FileWriter f = new FileWriter(textFile)) {
409+
props.store(f, "Pipeline test");
410+
}
411+
412+
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
413+
p.setDefinition(new CpsFlowDefinition(
414+
"node('slaves') {\n" +
415+
" def props = readProperties interpolate: true, file: '" + separatorsToSystemEscaped(textFile.getAbsolutePath()) + "'\n" +
416+
" assert props['base64d'] == '${base64Decoder:SGVsbG9Xb3JsZCE=}'\n" +
417+
" assert props['base64e'] == 'SGVsbG9Xb3JsZCE='\n" +
418+
"}", true));
419+
j.assertBuildStatusSuccess(p.scheduleBuild2(0));
420+
assertEquals("pwn3d", j.jenkins.getSystemMessage());
421+
}
422+
}

0 commit comments

Comments
 (0)