Skip to content

Prevent XMLDecoder from loading other than whitelisted classes #3526

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

Merged
merged 2 commits into from
Apr 7, 2021
Merged

Prevent XMLDecoder from loading other than whitelisted classes #3526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ed2cab
Prevent XMLDecoder from loading other than whitelisted classes
ahornace Apr 7, 2021
7ca68a8
Add additional classes to configuration classloader whitelist
ahornace Apr 7, 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
3 changes: 2 additions & 1 deletion opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/Configuration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,7 +1418,8 @@ private static Configuration decodeObject(InputStream in) throws IOException {
final LinkedList<Exception> exceptions = new LinkedList<>();
ExceptionListener listener = exceptions::addLast;

try (XMLDecoder d = new XMLDecoder(new BufferedInputStream(in), null, listener)) {
try (XMLDecoder d = new XMLDecoder(new BufferedInputStream(in), null, listener,
new ConfigurationClassLoader())) {
ret = d.readObject();
}

Expand Down
85 changes: 85 additions & 0 deletions ...ok-indexer/src/main/java/org/opengrok/indexer/configuration/ConfigurationClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/*
* CDDL HEADER START
*
* The contents of this file are subject to the terms of the
* Common Development and Distribution License (the "License").
* You may not use this file except in compliance with the License.
*
* See LICENSE.txt included in this distribution for the specific
* language governing permissions and limitations under the License.
*
* When distributing Covered Code, include this CDDL HEADER in each
* file and include the License file at LICENSE.txt.
* If applicable, add the following below this CDDL HEADER, with the
* fields enclosed by brackets "[]" replaced with your own identifying
* information: Portions Copyright [yyyy] [name of copyright owner]
*
* CDDL HEADER END
*/

/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
*/
package org.opengrok.indexer.configuration;

import io.micrometer.statsd.StatsdFlavor;
import org.opengrok.indexer.authorization.AuthControlFlag;
import org.opengrok.indexer.authorization.AuthorizationEntity;
import org.opengrok.indexer.authorization.AuthorizationPlugin;
import org.opengrok.indexer.authorization.AuthorizationStack;
import org.opengrok.indexer.authorization.IAuthorizationPlugin;
import org.opengrok.indexer.configuration.Configuration.RemoteSCM;
import org.opengrok.indexer.history.RepositoryInfo;

import java.beans.XMLDecoder;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Set;
import java.util.TreeSet;
import java.util.stream.Collectors;

/**
* Temporary hack to prevent {@link XMLDecoder} to deserialize other than allowed classes. This tries to prevent
* calling of methods on {@link ProcessBuilder} or {@link Runtime} (or similar) which could be used for code execution.
*/
public class ConfigurationClassLoader extends ClassLoader {

private static final Set<String> allowedClasses = Set.of(
ArrayList.class,
AuthControlFlag.class,
AuthorizationEntity.class,
AuthorizationPlugin.class,
AuthorizationStack.class,
Collections.class,
Configuration.class,
Enum.class,
Filter.class,
Group.class,
HashMap.class,
HashSet.class,
IAuthorizationPlugin.class,
IgnoredNames.class,
LuceneLockName.class,
Project.class,
RemoteSCM.class,
RepositoryInfo.class,
Set.class,
StatsdConfig.class,
StatsdFlavor.class,
String.class,
SuggesterConfig.class,
TreeSet.class,
XMLDecoder.class
).stream().map(Class::getName).collect(Collectors.toSet());

@Override
public Class<?> loadClass(final String name) throws ClassNotFoundException {
if (!allowedClasses.contains(name)) {
throw new IllegalAccessError(name + " is not allowed to be used in configuration");
}

return getClass().getClassLoader().loadClass(name);
}
}
65 changes: 65 additions & 0 deletions opengrok-indexer/src/test/java/org/opengrok/indexer/configuration/ConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,22 @@
import java.beans.XMLDecoder;
import java.beans.XMLEncoder;
import java.io.BufferedInputStream;
import java.io.BufferedReader;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.LinkedList;
import java.util.stream.Collectors;
import javax.xml.XMLConstants;
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.opengrok.indexer.util.ClassUtil;

import org.xml.sax.Attributes;
Expand All @@ -44,6 +50,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

/**
*
Expand Down Expand Up @@ -168,4 +175,62 @@ public void serializationOrderTest() throws IOException {
assertEquals(oldCfg.getGroups(), cfg.getGroups());
}
}

@ParameterizedTest
@ValueSource(strings = {
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<java version=\"11.0.8\" class=\"java.beans.XMLDecoder\">\n" +
" <object class=\"java.lang.Runtime\" method=\"getRuntime\">\n" +
" <void method=\"exec\">\n" +
" <array class=\"java.lang.String\" length=\"2\">\n" +
" <void index=\"0\">\n" +
" <string>/usr/bin/nc</string>\n" +
" </void>\n" +
" <void index=\"1\">\n" +
" <string>-l</string>\n" +
" </void>\n" +
" </array>\n" +
" </void>\n" +
" </object>\n" +
"</java>",
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<java version=\"11.0.8\" class=\"java.beans.XMLDecoder\">\n" +
" <object class=\"java.lang.ProcessBuilder\">\n" +
" <array class=\"java.lang.String\" length=\"1\" >\n" +
" <void index=\"0\"> \n" +
" <string>/usr/bin/curl https://oracle.com</string>\n" +
" </void>\n" +
" </array>\n" +
" <void method=\"start\"/>\n" +
" </object>\n" +
"</java>",
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<java version=\"11.0.8\" class=\"java.beans.XMLDecoder\">\n" +
" <object class = \"java.io.FileOutputStream\"> \n" +
" <string>opengrok_test.txt</string>\n" +
" <method name = \"write\">\n" +
" <array class=\"byte\" length=\"3\">\n" +
" <void index=\"0\"><byte>96</byte></void>\n" +
" <void index=\"1\"><byte>96</byte></void>\n" +
" <void index=\"2\"><byte>96</byte></void>\n" +
" </array>\n" +
" </method>\n" +
" <method name=\"close\"/>\n" +
" </object>\n" +
"</java>"
})
void testDeserializationOfNotWhiteListedClassThrowsError(final String exploit) {
assertThrows(IllegalAccessError.class, () -> Configuration.makeXMLStringAsConfiguration(exploit));
}

@Test
void testLoadingValidConfiguration() throws IOException {
try (var br = new BufferedReader(new InputStreamReader(ConfigurationTest.class.getClassLoader()
.getResourceAsStream("configuration/valid_configuration.xml")))) {
Copy link
Member

Choose a reason for hiding this comment

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

would be more robust to encode a Configuration instance and store it in the file however let's leave it like it is for now.

String xml = br.lines().collect(Collectors.joining(System.lineSeparator()));
var config = Configuration.makeXMLStringAsConfiguration(xml);
assertEquals("/opt/opengrok_data", config.getDataRoot());
}
}

}
4 changes: 2 additions & 2 deletions ...grok-indexer/src/test/java/org/opengrok/indexer/configuration/RuntimeEnvironmentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ public void testAuthorizationFlagDecodeInvalid() throws IOException {
* @throws IOException I/O exception
*/
@Test
public void testAuthorizationDecodeInvalid() throws IOException {
void testAuthorizationDecodeInvalid() {
String confString = "<?xml version='1.0' encoding='UTF-8'?>\n"
+ "<java class=\"java.beans.XMLDecoder\" version=\"1.8.0_121\">\n"
+ " <object class=\"org.opengrok.indexer.configuration.Configuration\">\n"
Expand All @@ -752,7 +752,7 @@ public void testAuthorizationDecodeInvalid() throws IOException {
+ "\t</void>\n"
+ " </object>\n"
+ "</java>";
assertThrows(IOException.class, () -> Configuration.makeXMLStringAsConfiguration(confString));
assertThrows(IllegalAccessError.class, () -> Configuration.makeXMLStringAsConfiguration(confString));
}

@Test
Expand Down
160 changes: 160 additions & 0 deletions opengrok-indexer/src/test/resources/configuration/valid_configuration.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
<?xml version="1.0" encoding="UTF-8"?>
<java version="11.0.8" class="java.beans.XMLDecoder">
<object class="org.opengrok.indexer.configuration.Configuration" id="Configuration0">
<void property="ctags">
<string>/usr/local/bin/ctags</string>
</void>
<void property="dataRoot">
<string>/opt/opengrok_data</string>
</void>
<void id="IgnoredNames0" property="ignoredNames">
<void id="IgnoredDirs0" property="ignoredDirs">
<void property="items">
<void method="add">
<string>.git</string>
</void>
<void method="add">
<string>.hg</string>
</void>
<void method="add">
<string>.repo</string>
</void>
<void method="add">
<string>.bk</string>
</void>
<void method="add">
<string>.bzr</string>
</void>
<void method="add">
<string>.svn</string>
</void>
<void method="add">
<string>SCCS</string>
</void>
<void method="add">
<string>.razor</string>
</void>
<void method="add">
<string>RCS</string>
</void>
<void method="add">
<string>CVS</string>
</void>
<void method="add">
<string>CVSROOT</string>
</void>
</void>
</void>
<void id="IgnoredFiles0" property="ignoredFiles">
<void property="items">
<void method="add">
<string>.git</string>
</void>
<void method="add">
<string>.hgtags</string>
</void>
<void method="add">
<string>.hgignore</string>
</void>
<void method="add">
<string>.cvsignore</string>
</void>
<void method="add">
<string>.p4config</string>
</void>
</void>
</void>
</void>
<void property="projects">
<void method="put">
<string>opengrok</string>
<object class="org.opengrok.indexer.configuration.Project">
<void property="historyEnabled">
<boolean>true</boolean>
</void>
<void property="indexed">
<boolean>true</boolean>
</void>
<void property="mergeCommitsEnabled">
<boolean>true</boolean>
</void>
<void property="name">
<string>opengrok</string>
</void>
<void property="path">
<string>/opengrok</string>
</void>
</object>
</void>
<void method="put">
<string>cuda-samples</string>
<object class="org.opengrok.indexer.configuration.Project">
<void property="historyEnabled">
<boolean>true</boolean>
</void>
<void property="indexed">
<boolean>true</boolean>
</void>
<void property="mergeCommitsEnabled">
<boolean>true</boolean>
</void>
<void property="name">
<string>cuda-samples</string>
</void>
<void property="path">
<string>/cuda-samples</string>
</void>
</object>
</void>
<void method="put">
<string>CudaSift</string>
<object class="org.opengrok.indexer.configuration.Project">
<void property="historyEnabled">
<boolean>true</boolean>
</void>
<void property="indexed">
<boolean>true</boolean>
</void>
<void property="mergeCommitsEnabled">
<boolean>true</boolean>
</void>
<void property="name">
<string>CudaSift</string>
</void>
<void property="path">
<string>/CudaSift</string>
</void>
</object>
</void>
</void>
<void property="projectsEnabled">
<boolean>true</boolean>
</void>
<void property="sourceRoot">
<string>/opt/opengrok_src</string>
</void>
<void id="SuggesterConfig0" property="suggesterConfig">
<void property="allowedFields">
<void method="clear"/>
<void method="add">
<string>defs</string>
</void>
<void method="add">
<string>path</string>
</void>
<void method="add">
<string>hist</string>
</void>
<void method="add">
<string>refs</string>
</void>
<void method="add">
<string>type</string>
</void>
<void method="add">
<string>full</string>
</void>
</void>
</void>
</object>
</java>