Skip to content
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.

Sign up for GitHub

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

[JENKINS-67380] Detect oversized maps, collections and arrays and truncate #492

Merged
merged 5 commits into from
Dec 16, 2021
Merged

[JENKINS-67380] Detect oversized maps, collections and arrays and truncate #492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f47d90
[JENKINS-67380] Detect oversized maps, collections and arrays and tru…
Vlatombe Dec 16, 2021
7944408
Keep root structure for arguments
Vlatombe Dec 16, 2021
4f4eac2
Also count key size
Vlatombe Dec 16, 2021
7afa765
Edits based on Jesse's review
Vlatombe Dec 16, 2021
80c2750
Clarify the implementation
Vlatombe Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 56 additions & 6 deletions src/main/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import hudson.EnvVars;
import hudson.model.Describable;
import hudson.model.Result;
import java.util.Collection;
import jenkins.model.Jenkins;
import org.apache.commons.io.output.NullOutputStream;
import org.jenkinsci.plugins.structs.describable.DescribableModel;
Expand Down Expand Up @@ -73,7 +74,7 @@ public class ArgumentsActionImpl extends ArgumentsAction {

public ArgumentsActionImpl(@Nonnull Map<String, Object> stepArguments, @CheckForNull EnvVars env, @Nonnull Set<String> sensitiveVariables) {
this.sensitiveVariables = new HashSet<>(sensitiveVariables);
arguments = serializationCheck(sanitizeMapAndRecordMutation(stepArguments, env));
this.arguments = serializationCheck(sanitizeStepArguments(stepArguments, env));
}

/** Create a step, sanitizing strings for secured content */
Expand All @@ -83,7 +84,7 @@ public ArgumentsActionImpl(@Nonnull Map<String, Object> stepArguments) {

/** For testing use only */
ArgumentsActionImpl(@Nonnull Set<String> sensitiveVariables){
this.isUnmodifiedBySanitization = false;
this.isUnmodifiedBySanitization = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was wrong. If you do nothing then the data is unmodified.

this.arguments = Collections.emptyMap();
this.sensitiveVariables = sensitiveVariables;
}
Expand Down Expand Up @@ -147,9 +148,14 @@ Object sanitizeListAndRecordMutation(@Nonnull List objects, @CheckForNull EnvVar

boolean isMutated = false;
List output = new ArrayList(objects.size());
long size = objects.size();
for (Object o : objects) {
Object modded = sanitizeObjectAndRecordMutation(o, variables);

size += shallowSize(modded);
if (size > MAX_RETAINED_LENGTH) {
this.isUnmodifiedBySanitization = false;
return NotStoredReason.OVERSIZE_VALUE;
}
if (modded != o) {
// Sanitization stripped out some values, so we need to store the mutated object
output.add(modded);
Expand Down Expand Up @@ -223,7 +229,7 @@ Object sanitizeObjectAndRecordMutation(@CheckForNull Object o, @CheckForNull Env
Object modded = tempVal;
if (modded instanceof Map) {
// Recursive sanitization, oh my!
modded = sanitizeMapAndRecordMutation((Map)modded, vars);
modded = sanitizeMapAndRecordMutation((Map)modded, vars, false);
} else if (modded instanceof List) {
modded = sanitizeListAndRecordMutation((List) modded, vars);
} else if (modded != null && modded.getClass().isArray()) {
Expand Down Expand Up @@ -295,13 +301,35 @@ Map<String, Object> serializationCheck(@Nonnull Map<String, Object> arguments) {
* Goes through {@link #sanitizeObjectAndRecordMutation(Object, EnvVars)} for each value in a map input.
*/
@Nonnull
Map<String,Object> sanitizeMapAndRecordMutation(@Nonnull Map<String, Object> mapContents, @CheckForNull EnvVars variables) {
Object sanitizeMapAndRecordMutation(@Nonnull Map<String, Object> mapContents, @CheckForNull EnvVars variables) {
// Package scoped so we can test it directly
return sanitizeMapAndRecordMutation(mapContents, variables, false);
}

private Map<String, Object> sanitizeStepArguments(Map<String, Object> stepArguments, EnvVars env) {
// Guaranteed to be a map, the block returning something else is guarded by topLevel == false
return (Map<String, Object>) sanitizeMapAndRecordMutation(stepArguments, env, true);
}

/**
* Goes through {@link #sanitizeObjectAndRecordMutation(Object, EnvVars)} for each value in a map input.
*/
@Nonnull
private Object sanitizeMapAndRecordMutation(@Nonnull Map<String, Object> mapContents, @CheckForNull EnvVars variables, boolean topLevel) {
LinkedHashMap<String, Object> output = new LinkedHashMap<>(mapContents.size());
long size = mapContents.size();
jglick marked this conversation as resolved.
Show resolved Hide resolved

boolean isMutated = false;
for (Map.Entry<String,?> param : mapContents.entrySet()) {
Object modded = sanitizeObjectAndRecordMutation(param.getValue(), variables);
if (!topLevel) {
size += param.getKey().length();
size += shallowSize(modded);
if (size > MAX_RETAINED_LENGTH) {
this.isUnmodifiedBySanitization = false;
return NotStoredReason.OVERSIZE_VALUE;
}
}
if (modded != param.getValue()) {
// Sanitization stripped out some values, so we need to store the mutated object
output.put(param.getKey(), modded);
Expand All @@ -311,7 +339,29 @@ Map<String,Object> sanitizeMapAndRecordMutation(@Nonnull Map<String, Object> map
}
}

return (isMutated) ? output : mapContents;
return isMutated ? output : mapContents;
}

static long shallowSize(Object o) {
if (o == null) {
return 0;
}
if (o instanceof CharSequence) {
return ((CharSequence) o).length();
}
if (o instanceof Collection) {
Collection<?> collection = (Collection<?>) o;
return collection.size();
}
if (o.getClass().isArray()) {
Object[] array = (Object[]) o;
return array.length;
}
if (o instanceof Map) {
Map<?, ?> map = (Map<?, ?>) o;
return map.size();
}
return 1;
}

/** Accessor for testing use */
Expand Down
86 changes: 72 additions & 14 deletions src/test/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@
import hudson.Functions;
import hudson.XmlFile;
import hudson.model.Action;
import hudson.model.ParametersAction;
import hudson.model.ParametersDefinitionProperty;
import hudson.model.PasswordParameterDefinition;
import hudson.model.PasswordParameterValue;
import hudson.tasks.ArtifactArchiver;
import org.apache.commons.lang.RandomStringUtils;
import org.hamcrest.MatcherAssert;
Expand Down Expand Up @@ -181,9 +177,7 @@ public void testRecursiveSanitizationOfContent() {
int maxLen = ArgumentsActionImpl.getMaxRetainedLength();
ArgumentsActionImpl impl = new ArgumentsActionImpl(sensitiveVariables);

char[] oversized = new char[maxLen+10];
Arrays.fill(oversized, 'a');
String oversizedString = new String (oversized);
String oversizedString = generateStringOfSize(maxLen + 10);

// Simplest masking of secret and oversized value
Assert.assertEquals("${USERVARIABLE}", impl.sanitizeObjectAndRecordMutation(secretUsername, env));
Expand All @@ -210,28 +204,92 @@ public void testRecursiveSanitizationOfContent() {
// Maps
HashMap<String, Object> dangerous = new HashMap<>();
dangerous.put("name", secretUsername);
Map<String, Object> sanitizedMap = impl.sanitizeMapAndRecordMutation(dangerous, env);
Assert.assertNotEquals(sanitizedMap, dangerous);
Object sanitized = impl.sanitizeMapAndRecordMutation(dangerous, env);
Assert.assertNotEquals(sanitized, dangerous);
assertThat(sanitized, instanceOf(Map.class));
Map<String, Object> sanitizedMap = (Map<String, Object>) sanitized;
Assert.assertEquals("${USERVARIABLE}", sanitizedMap.get("name"));
Assert.assertFalse(impl.isUnmodifiedArguments());
impl.isUnmodifiedBySanitization = true;

Map<String, Object> identicalMap = impl.sanitizeMapAndRecordMutation(dangerous, new EnvVars()); // String is no longer dangerous
Assert.assertEquals(identicalMap, dangerous);
Object identical = impl.sanitizeMapAndRecordMutation(dangerous, new EnvVars()); // String is no longer dangerous
Assert.assertEquals(identical, dangerous);
Assert.assertTrue(impl.isUnmodifiedArguments());

// Lists
List unsanitizedList = Arrays.asList("cheese", null, secretUsername);
List sanitized = (List)impl.sanitizeListAndRecordMutation(unsanitizedList, env);
Assert.assertEquals(3, sanitized.size());
Object sanitized2 = impl.sanitizeListAndRecordMutation(unsanitizedList, env);
assertThat(sanitized2, instanceOf(List.class));
List sanitizedList = (List) sanitized2;
Assert.assertEquals(3, sanitizedList.size());
Assert.assertFalse(impl.isUnmodifiedArguments());
Assert.assertEquals("${USERVARIABLE}", sanitized.get(2));
Assert.assertEquals("${USERVARIABLE}", sanitizedList.get(2));
impl.isUnmodifiedBySanitization = true;

Assert.assertEquals(unsanitizedList, impl.sanitizeObjectAndRecordMutation(unsanitizedList, new EnvVars()));
Assert.assertEquals(unsanitizedList, impl.sanitizeListAndRecordMutation(unsanitizedList, new EnvVars()));
}

@Test
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
@Issue("JENKINS-67380")
public void oversizedMap() {
{
// a map with reasonable size should not be truncated
ArgumentsActionImpl impl = new ArgumentsActionImpl(Collections.emptySet());
Map<String, Object> smallMap = new HashMap<>();
smallMap.put("key1", generateStringOfSize(ArgumentsActionImpl.getMaxRetainedLength() / 10));
Object sanitizedSmallMap = impl.sanitizeMapAndRecordMutation(smallMap, null);
Assert.assertEquals(sanitizedSmallMap, smallMap);
Assert.assertTrue(impl.isUnmodifiedArguments());
impl.isUnmodifiedBySanitization = true;
}

{
// arguments map keys should be kept, but values should be truncated if too large
Map<String, Object> bigMap = new HashMap<>();
String bigString = generateStringOfSize(ArgumentsActionImpl.getMaxRetainedLength() + 10);
bigMap.put("key1", bigString);
ArgumentsActionImpl impl = new ArgumentsActionImpl(bigMap, null, Collections.emptySet());
Assert.assertEquals(ArgumentsAction.NotStoredReason.OVERSIZE_VALUE, impl.getArgumentValueOrReason("key1"));
Assert.assertFalse(impl.isUnmodifiedArguments());
}

{
// an arbitrary map should be truncated if it is too large overall
Map<String, Object> bigMap2 = new HashMap<>();
String bigString2 = generateStringOfSize(ArgumentsActionImpl.getMaxRetainedLength());
bigMap2.put("key1", bigString2);
ArgumentsActionImpl impl = new ArgumentsActionImpl(Collections.emptySet());
Object sanitizedBigMap2 = impl.sanitizeMapAndRecordMutation(bigMap2, null);
Assert.assertNotEquals(sanitizedBigMap2, bigMap2);
Assert.assertEquals(ArgumentsAction.NotStoredReason.OVERSIZE_VALUE, sanitizedBigMap2);
Assert.assertFalse(impl.isUnmodifiedArguments());
impl.isUnmodifiedBySanitization = true;
}
}

@Test
public void oversizedList() {
ArgumentsActionImpl impl = new ArgumentsActionImpl(Collections.emptySet());
List unsanitized = Arrays.asList(generateStringOfSize(ArgumentsActionImpl.getMaxRetainedLength()));
Object sanitized = impl.sanitizeListAndRecordMutation(unsanitized, null);
Assert.assertEquals(ArgumentsAction.NotStoredReason.OVERSIZE_VALUE, sanitized);
}

@Test
public void oversizedArray() {
ArgumentsActionImpl impl = new ArgumentsActionImpl(Collections.emptySet());
String[] unsanitized = new String[] {generateStringOfSize(ArgumentsActionImpl.getMaxRetainedLength())};
Object sanitized = impl.sanitizeArrayAndRecordMutation(unsanitized, null);
Assert.assertEquals(ArgumentsAction.NotStoredReason.OVERSIZE_VALUE, sanitized);
}

private static String generateStringOfSize(int size) {
char[] bigChars = new char[size];
Arrays.fill(bigChars, 'a');
return String.valueOf(bigChars);
}

@Test
public void testArraySanitization() {
EnvVars env = new EnvVars();
Expand Down