Skip to content

Commit 13e4f14

Browse files
carlosu7carlosu7
authored
Update semgrep query for HardenJavaDeserializationCodemod and update paths to exclude (#267)
* Update semgrep query for HardenJavaDeserializationCodemod * Updated paths to exclude *Test.java files
1 parent 926a7ec commit 13e4f14

File tree

5 files changed

+311
-2
lines changed

5 files changed

+311
-2
lines changed

‎core-codemods/src/main/resources/io/codemodder/codemods/harden-java-deserialization.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ rules:
1313
- pattern-not-inside: >
1414
$RETURNTYPE $METHOD(...) {
1515
...
16-
(io.github.pixee.security.ObjectInputFilters).enableObjectFilterIfUnprotected($OIS);
16+
ObjectInputFilters.enableObjectFilterIfUnprotected($OIS);
1717
...
1818
}
1919
- focus-metavariable: $OIS

‎core-codemods/src/test/resources/harden-java-deserialization/ObjectInputFiltersTest.java.after‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
package io.github.pixee.security;
2+
3+
import io.github.pixee.security.ObjectInputFilters;
4+
import static org.hamcrest.CoreMatchers.*;
5+
import static org.hamcrest.MatcherAssert.assertThat;
6+
import static org.junit.jupiter.api.Assertions.assertThrows;
7+
import static org.junit.jupiter.api.Assertions.fail;
8+
import static org.mockito.Mockito.*;
9+
10+
import java.io.*;
11+
import java.nio.file.Files;
12+
import org.apache.commons.fileupload.disk.DiskFileItem;
13+
import org.junit.jupiter.api.BeforeAll;
14+
import org.junit.jupiter.api.Test;
15+
16+
final class ObjectInputFiltersTest {
17+
18+
private static DiskFileItem gadget; // this is an evil gadget type
19+
private static byte[] serializedGadget; // this the serialized bytes of that gadget
20+
21+
@BeforeAll
22+
static void setup() throws IOException {
23+
ByteArrayOutputStream baos = new ByteArrayOutputStream();
24+
gadget =
25+
new DiskFileItem(
26+
"fieldName",
27+
"text/html",
28+
false,
29+
"foo.html",
30+
100,
31+
Files.createTempDirectory("adi").toFile());
32+
gadget.getOutputStream(); // needed to make the object serializable
33+
ObjectOutputStream oos = new ObjectOutputStream(baos);
34+
oos.writeObject(gadget);
35+
serializedGadget = baos.toByteArray();
36+
}
37+
38+
@Test
39+
void default_is_unprotected() throws Exception {
40+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
41+
ObjectInputFilters.enableObjectFilterIfUnprotected(ois);
42+
Object o = ois.readObject();
43+
assertThat(o, instanceOf(DiskFileItem.class));
44+
}
45+
46+
47+
@Test
48+
void ois_harden_works() throws Exception {
49+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
50+
ObjectInputFilters.enableObjectFilterIfUnprotected(ois);
51+
assertThrows(
52+
InvalidClassException.class,
53+
() -> {
54+
ois.readObject();
55+
fail("this should have been blocked");
56+
});
57+
}
58+
59+
@Test
60+
void objectinputfilter_works_when_none_present() throws Exception {
61+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
62+
ois.setObjectInputFilter(ObjectInputFilters.getHardenedObjectFilter());
63+
assertThrows(
64+
InvalidClassException.class,
65+
() -> {
66+
ois.readObject();
67+
fail("this should have been blocked");
68+
});
69+
}
70+
71+
/**
72+
* This test makes sure that if there's an existing {@link ObjectInputFilter}, that we honor it
73+
* while we also do our protection. It bans a BadType, and allows a GoodType, so that behavior
74+
* should still work as well as still reject our evil gadgets.
75+
*/
76+
@Test
77+
void objectinputfilter_works_and_honors_existing() throws Exception {
78+
ObjectInputFilter filter =
79+
ObjectInputFilter.Config.createFilter(
80+
"!" + BadType.class.getName() + ";" + GoodType.class.getName());
81+
{
82+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
83+
// this joins the default filter to the existing
84+
ois.setObjectInputFilter(ObjectInputFilters.createCombinedHardenedObjectFilter(filter));
85+
assertThrows(
86+
InvalidClassException.class,
87+
() -> {
88+
ois.readObject();
89+
fail("this should have been blocked");
90+
});
91+
}
92+
93+
// make sure we still reject the bad type
94+
{
95+
byte[] serializedBadType = serialize(new BadType());
96+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedBadType));
97+
ois.setObjectInputFilter(
98+
ObjectInputFilters.createCombinedHardenedObjectFilter(filter)); // this is our weave
99+
100+
assertThrows(
101+
InvalidClassException.class,
102+
() -> {
103+
ois.readObject();
104+
fail("this should have been blocked -- the original filter should have rejected it");
105+
});
106+
}
107+
108+
// make we still allow the good type
109+
{
110+
byte[] serializedGoodType = serialize(new GoodType());
111+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGoodType));
112+
ois.setObjectInputFilter(ObjectInputFilters.createCombinedHardenedObjectFilter(filter));
113+
GoodType goodType = (GoodType) ois.readObject();
114+
assertThat(goodType, is(notNullValue()));
115+
}
116+
}
117+
118+
@Test
119+
void the_filter_works_as_expected() {
120+
ObjectInputFilter filter = ObjectInputFilters.getHardenedObjectFilter();
121+
ObjectInputFilter.FilterInfo filterInfo = mock(ObjectInputFilter.FilterInfo.class);
122+
123+
// we never want to interfere with existing logic, so we never explicitly approve anything, even
124+
// innocent j.l.String
125+
doReturn(String.class).when(filterInfo).serialClass();
126+
ObjectInputFilter.Status status = filter.checkInput(filterInfo);
127+
assertThat(status, is(ObjectInputFilter.Status.UNDECIDED));
128+
129+
// this confirm that the exact match of ProcessBuilder in our list is caught by the filter
130+
doReturn(ProcessBuilder.class).when(filterInfo).serialClass();
131+
ObjectInputFilter.Status exactMatch = filter.checkInput(filterInfo);
132+
assertThat(exactMatch, is(ObjectInputFilter.Status.REJECTED));
133+
134+
// this confirms that although the Redirect type is not explicitly in the tokens, it's still
135+
// caught by the wildcard
136+
doReturn(ProcessBuilder.Redirect.class).when(filterInfo).serialClass();
137+
ObjectInputFilter.Status partialMatch = filter.checkInput(filterInfo);
138+
assertThat(partialMatch, is(ObjectInputFilter.Status.REJECTED));
139+
}
140+
141+
byte[] serialize(Serializable s) throws IOException {
142+
ByteArrayOutputStream stream = new ByteArrayOutputStream();
143+
new ObjectOutputStream(stream).writeObject(s);
144+
return stream.toByteArray();
145+
}
146+
147+
static class BadType implements Serializable {}
148+
149+
static class GoodType implements Serializable {}
150+
}

‎core-codemods/src/test/resources/harden-java-deserialization/ObjectInputFiltersTest.java.before‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
package io.github.pixee.security;
2+
3+
import static org.hamcrest.CoreMatchers.*;
4+
import static org.hamcrest.MatcherAssert.assertThat;
5+
import static org.junit.jupiter.api.Assertions.assertThrows;
6+
import static org.junit.jupiter.api.Assertions.fail;
7+
import static org.mockito.Mockito.*;
8+
9+
import java.io.*;
10+
import java.nio.file.Files;
11+
import org.apache.commons.fileupload.disk.DiskFileItem;
12+
import org.junit.jupiter.api.BeforeAll;
13+
import org.junit.jupiter.api.Test;
14+
15+
final class ObjectInputFiltersTest {
16+
17+
private static DiskFileItem gadget; // this is an evil gadget type
18+
private static byte[] serializedGadget; // this the serialized bytes of that gadget
19+
20+
@BeforeAll
21+
static void setup() throws IOException {
22+
ByteArrayOutputStream baos = new ByteArrayOutputStream();
23+
gadget =
24+
new DiskFileItem(
25+
"fieldName",
26+
"text/html",
27+
false,
28+
"foo.html",
29+
100,
30+
Files.createTempDirectory("adi").toFile());
31+
gadget.getOutputStream(); // needed to make the object serializable
32+
ObjectOutputStream oos = new ObjectOutputStream(baos);
33+
oos.writeObject(gadget);
34+
serializedGadget = baos.toByteArray();
35+
}
36+
37+
@Test
38+
void default_is_unprotected() throws Exception {
39+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
40+
Object o = ois.readObject();
41+
assertThat(o, instanceOf(DiskFileItem.class));
42+
}
43+
44+
45+
@Test
46+
void ois_harden_works() throws Exception {
47+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
48+
ObjectInputFilters.enableObjectFilterIfUnprotected(ois);
49+
assertThrows(
50+
InvalidClassException.class,
51+
() -> {
52+
ois.readObject();
53+
fail("this should have been blocked");
54+
});
55+
}
56+
57+
@Test
58+
void objectinputfilter_works_when_none_present() throws Exception {
59+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
60+
ois.setObjectInputFilter(ObjectInputFilters.getHardenedObjectFilter());
61+
assertThrows(
62+
InvalidClassException.class,
63+
() -> {
64+
ois.readObject();
65+
fail("this should have been blocked");
66+
});
67+
}
68+
69+
/**
70+
* This test makes sure that if there's an existing {@link ObjectInputFilter}, that we honor it
71+
* while we also do our protection. It bans a BadType, and allows a GoodType, so that behavior
72+
* should still work as well as still reject our evil gadgets.
73+
*/
74+
@Test
75+
void objectinputfilter_works_and_honors_existing() throws Exception {
76+
ObjectInputFilter filter =
77+
ObjectInputFilter.Config.createFilter(
78+
"!" + BadType.class.getName() + ";" + GoodType.class.getName());
79+
{
80+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGadget));
81+
// this joins the default filter to the existing
82+
ois.setObjectInputFilter(ObjectInputFilters.createCombinedHardenedObjectFilter(filter));
83+
assertThrows(
84+
InvalidClassException.class,
85+
() -> {
86+
ois.readObject();
87+
fail("this should have been blocked");
88+
});
89+
}
90+
91+
// make sure we still reject the bad type
92+
{
93+
byte[] serializedBadType = serialize(new BadType());
94+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedBadType));
95+
ois.setObjectInputFilter(
96+
ObjectInputFilters.createCombinedHardenedObjectFilter(filter)); // this is our weave
97+
98+
assertThrows(
99+
InvalidClassException.class,
100+
() -> {
101+
ois.readObject();
102+
fail("this should have been blocked -- the original filter should have rejected it");
103+
});
104+
}
105+
106+
// make we still allow the good type
107+
{
108+
byte[] serializedGoodType = serialize(new GoodType());
109+
ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(serializedGoodType));
110+
ois.setObjectInputFilter(ObjectInputFilters.createCombinedHardenedObjectFilter(filter));
111+
GoodType goodType = (GoodType) ois.readObject();
112+
assertThat(goodType, is(notNullValue()));
113+
}
114+
}
115+
116+
@Test
117+
void the_filter_works_as_expected() {
118+
ObjectInputFilter filter = ObjectInputFilters.getHardenedObjectFilter();
119+
ObjectInputFilter.FilterInfo filterInfo = mock(ObjectInputFilter.FilterInfo.class);
120+
121+
// we never want to interfere with existing logic, so we never explicitly approve anything, even
122+
// innocent j.l.String
123+
doReturn(String.class).when(filterInfo).serialClass();
124+
ObjectInputFilter.Status status = filter.checkInput(filterInfo);
125+
assertThat(status, is(ObjectInputFilter.Status.UNDECIDED));
126+
127+
// this confirm that the exact match of ProcessBuilder in our list is caught by the filter
128+
doReturn(ProcessBuilder.class).when(filterInfo).serialClass();
129+
ObjectInputFilter.Status exactMatch = filter.checkInput(filterInfo);
130+
assertThat(exactMatch, is(ObjectInputFilter.Status.REJECTED));
131+
132+
// this confirms that although the Redirect type is not explicitly in the tokens, it's still
133+
// caught by the wildcard
134+
doReturn(ProcessBuilder.Redirect.class).when(filterInfo).serialClass();
135+
ObjectInputFilter.Status partialMatch = filter.checkInput(filterInfo);
136+
assertThat(partialMatch, is(ObjectInputFilter.Status.REJECTED));
137+
}
138+
139+
byte[] serialize(Serializable s) throws IOException {
140+
ByteArrayOutputStream stream = new ByteArrayOutputStream();
141+
new ObjectOutputStream(stream).writeObject(s);
142+
return stream.toByteArray();
143+
}
144+
145+
static class BadType implements Serializable {}
146+
147+
static class GoodType implements Serializable {}
148+
}

‎framework/codemodder-base/src/main/java/io/codemodder/CLI.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,7 @@ private List<ParameterArgument> createFromParameterStrings(final List<String> pa
618618
private static final List<String> defaultPathExcludes =
619619
List.of(
620620
"**/test/**",
621+
"**/*Test.java",
621622
"**/intTest/**",
622623
"**/tests/**",
623624
"**/target/**",

‎framework/codemodder-base/src/test/java/io/codemodder/CLITest.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ final class CLITest {
3535
private Path workingRepoDir;
3636
private Path fooJavaFile;
3737
private Path barJavaFile;
38+
private Path testFile;
3839
private List<SourceDirectory> sourceDirectories;
3940

4041
@BeforeEach
@@ -46,10 +47,12 @@ void setup(final @TempDir Path tmpDir) throws IOException {
4647
Files.createDirectories(tmpDir.resolve("module2/src/main/java/com/acme/util/"));
4748
fooJavaFile = module1JavaDir.resolve("Foo.java");
4849
barJavaFile = module2JavaDir.resolve("Bar.java");
50+
testFile = module2JavaDir.resolve("MyTest.java");
4951
Files.write(
5052
fooJavaFile,
5153
"import com.acme.util.Bar; class Foo {private var bar = new Bar();}".getBytes());
5254
Files.write(barJavaFile, "public class Bar {}".getBytes());
55+
Files.write(testFile, "public class MyTest {}".getBytes());
5356

5457
/*
5558
* Only add module2 to the discovered source directories. This will help prove that the module1 files can still be seen and changed, even if we couldn't locate it as a "source directory".
@@ -71,6 +74,13 @@ void it_works_normally() throws IOException {
7174
assertThat(Files.exists(outputFile)).isTrue();
7275
}
7376

77+
@Test
78+
void it_doesnt_change_test_file() throws IOException {
79+
String[] args = new String[] {"--dont-exit", workingRepoDir.toString()};
80+
Runner.run(List.of(Cloud9Changer.class), args);
81+
assertThat(Files.readString(testFile)).doesNotContain("cloud9");
82+
}
83+
7484
@Test
7585
void it_works_without_output_file() throws IOException {
7686
String[] args = new String[] {"--dont-exit", workingRepoDir.toString()};
@@ -203,7 +213,7 @@ void file_finder_works() throws IOException {
203213

204214
IncludesExcludes all = IncludesExcludes.any();
205215
List<Path> files = finder.findFiles(workingRepoDir, all);
206-
assertThat(files).containsExactly(fooJavaFile, barJavaFile);
216+
assertThat(files).containsExactly(fooJavaFile, barJavaFile, testFile);
207217

208218
IncludesExcludes onlyFoo =
209219
IncludesExcludes.withSettings(workingRepoDir.toFile(), List.of("**/Foo.java"), List.of());

0 commit comments

Comments
 (0)