Skip to content

Commit

Permalink
fix: CWE 502 Deserialization of Untrusted Data + fix yaml list resour…
Browse files Browse the repository at this point in the history
…ce split

Signed-off-by: Marc Nuri <marc@marcnuri.com>
  • Loading branch information
manusa committed Mar 26, 2020
1 parent 1e16651 commit 02b0410
Show file tree
Hide file tree
Showing 7 changed files with 210 additions and 54 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Usage:
* Fix #112: Fix windows specific path error while splitting file path
* Fix #102: HelmMojo works again
* Fix #120: Critical bugs reported by Sonar
* Fix #122: Bug 561261 - jkube-kit - insecure yaml load leading to RCE (CWE-502)

### 0.2.0 (05-03-2020)
* Fix #71: script to extract changelog information for notifications
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public void testYamlToPropertiesParsing() {

}

@Test(expected = IllegalArgumentException.class)
@Test(expected = IllegalStateException.class)
public void testInvalidFileThrowsException() {
YamlUtil.getPropertiesFromYamlResource(SpringBootUtilTest.class.getResource("/util/invalid-application.yml"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

public class ThorntailUtil {

private ThorntailUtil() {}

/**
* Returns the thorntail configuration (supports `project-defaults.yml`)
* or an empty properties object if not found
Expand All @@ -28,8 +30,6 @@ public class ThorntailUtil {
*/
public static Properties getThorntailProperties(URLClassLoader compileClassLoader) {
URL ymlResource = compileClassLoader.findResource("project-defaults.yml");

Properties props = YamlUtil.getPropertiesFromYamlResource(ymlResource);
return props;
return YamlUtil.getPropertiesFromYamlResource(ymlResource);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,47 +14,50 @@
package org.eclipse.jkube.kit.common.util;

import java.io.IOException;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
import java.util.SortedMap;
import org.yaml.snakeyaml.Yaml;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
import org.apache.commons.lang3.StringUtils;

public class YamlUtil {

private static final YAMLFactory YAML_FACTORY = new YAMLFactory();
private static final ObjectMapper YAML_MAPPER = new ObjectMapper(YAML_FACTORY);
private static final String EMPTY_YAML = "---\n";

private YamlUtil() {
}

protected static Properties getPropertiesFromYamlResource(URL resource) {
return getPropertiesFromYamlResource(null, resource);
}

protected static Properties getPropertiesFromYamlResource(String activeProfile, URL resource) {
if (resource != null) {
try {
Properties properties = new Properties();
// Splitting file for the possibility of different profiles, by default
// only first profile would be considered.
List<String> profiles = getYamlListFromFile(resource);
if (profiles.size() > 0) {
try {
properties.putAll(getPropertiesFromYamlString(getYamlFromYamlList(activeProfile, profiles)));
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException(String.format("Spring Boot configuration file %s is not formatted correctly. %s",
resource.toString(), e.getMessage()));
}
}
return properties;
} catch (IOException | URISyntaxException e) {
throw new IllegalStateException("Error while reading Yaml resource from URL " + resource, e);
}
protected static Properties getPropertiesFromYamlResource(String activeProfile, URL resource) {
if (resource != null) {
try {
Properties properties = new Properties();
List<String> profiles = splitYamlResource(resource);
if (!profiles.isEmpty()) {
properties.putAll(getPropertiesFromYamlString(getYamlFromYamlList(activeProfile, profiles)));
}
return new Properties();
return properties;
} catch (IOException e) {
throw new IllegalStateException("Error while reading Yaml resource from URL " + resource, e);
}
}
return new Properties();
}

/**
* Build a flattened representation of the Yaml tree. The conversion is compliant with the thorntail spring-boot rules.
Expand Down Expand Up @@ -108,32 +111,38 @@ else if (value instanceof Collection) {
}
}

public static Properties getPropertiesFromYamlString(String yamlString) throws IllegalArgumentException {
Yaml yaml = new Yaml();
Properties properties = new Properties();

@SuppressWarnings("unchecked")
SortedMap<String, Object> source = yaml.loadAs(yamlString, SortedMap.class);
if (source != null) {
properties.putAll(getFlattenedMap(source));
}
return properties;
public static Properties getPropertiesFromYamlString(String yamlString) throws IOException {
final Properties properties = new Properties();
final SortedMap<String, ?> source = YAML_MAPPER.readValue(
Optional.ofNullable(yamlString).filter(StringUtils::isNotBlank).orElse(EMPTY_YAML),
new TypeReference<SortedMap<String, ?>>() {
});
if (source != null) {
properties.putAll(getFlattenedMap(source));
}
return properties;
}

public static List<String> getYamlListFromFile(URL resource) throws URISyntaxException, IOException {
String fileAsString = new String(Files.readAllBytes(Paths.get(resource.toURI())));
String[] profiles = fileAsString.split("---");
return Arrays.asList(profiles);
static List<String> splitYamlResource(URL resource) throws IOException {
final List<String> serializedYamlList = new ArrayList<>();
final List<Map<String, ?>> parsedList = YAML_MAPPER.readValues(
YAML_FACTORY.createParser(resource), new TypeReference<Map<String, ?>>() {
}).readAll();
for (Map<String, ?> listEntry : parsedList) {
serializedYamlList.add(YAML_MAPPER.writeValueAsString(listEntry));
}
return serializedYamlList;
}

public static String getYamlFromYamlList(String pattern, List<String> yamlAsStringList) {
static String getYamlFromYamlList(String pattern, List<String> yamlAsStringList) {
if (pattern != null) {
for (String yamlStr : yamlAsStringList) {
if (yamlStr.contains(pattern))
return yamlStr;
if (yamlStr.contains(pattern)) {
return yamlStr;
}
}
}
return yamlAsStringList.get(0);
return yamlAsStringList.iterator().next();
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@

public class ThorntailUtilTest {

@Test
public void testReadThorntailPort() {
Properties props = YamlUtil.getPropertiesFromYamlResource(ThorntailUtilTest.class.getResource("/util/project-default.yml"));
assertNotNull(props);
assertEquals("8082", props.getProperty("thorntail.http.port"));

}
@Test
public void testReadThorntailPort() {
Properties props = YamlUtil.getPropertiesFromYamlResource(ThorntailUtilTest.class.getResource("/util/project-default.yml"));
assertNotNull(props);
assertEquals("8082", props.getProperty("thorntail.http.port"));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/**
* Copyright (c) 2019 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at:
*
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.jkube.kit.common.util;


import java.net.URL;
import java.util.List;
import java.util.Properties;

import com.fasterxml.jackson.databind.JsonMappingException;
import org.junit.Test;

import static org.eclipse.jkube.kit.common.util.YamlUtil.getPropertiesFromYamlString;
import static org.eclipse.jkube.kit.common.util.YamlUtil.splitYamlResource;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.CoreMatchers.startsWith;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;

public class YamlUtilTest {

@Test
public void getPropertiesFromYamlStringEmptyStringTest() throws Exception {
// Given
final String yamlString = "";
// When
final Properties result = getPropertiesFromYamlString(yamlString);
// Then
assertThat(result, notNullValue());
assertThat(result.size(), is(0));
}

@Test
public void getPropertiesFromYamlStringNullStringTest() throws Exception {
// When
final Properties result = getPropertiesFromYamlString(null);
// Then
assertThat(result, notNullValue());
assertThat(result.size(), is(0));
}

@Test(expected = JsonMappingException.class)
public void getPropertiesFromYamlStringInvalidStringTest() throws Exception {
// Given
final String yamlString = "not\na\nvalid\nyaml";
// When
getPropertiesFromYamlString(yamlString);
// Then
fail();
}

@Test
public void getPropertiesFromYamlStringValidStringTest() throws Exception {
// Given
final String yamlString = "---\ntest: 1\nlist:\n - name: item 1\n value: value 1\nstill-test: 1";
// When
final Properties result = getPropertiesFromYamlString(yamlString);
// Then
assertThat(result, notNullValue());
assertThat(result.size(), is(4));
assertThat(result.getProperty("test"), is("1"));
assertThat(result.getProperty("list[0].name"), is("item 1"));
assertThat(result.getProperty("list[0].value"), is("value 1"));
assertThat(result.getProperty("still-test"), is("1"));
}

// https://bugs.eclipse.org/bugs/show_bug.cgi?id=561261
@Test
public void getPropertiesFromYamlCWE502Test() throws Exception {
// Given
final String yamlString = "maps: !!javax.script.ScriptEngineManager [!!java.net.URLClassLoader [[!!java.net.URL [\\\"http://localhost:9000/\\\"]]]]";
// When
final Properties result = getPropertiesFromYamlString(yamlString);
// Then
assertThat(result, notNullValue());
assertThat(result.size(), is(1));
assertThat(result.getProperty("maps[0][0][0][0]"), is("\\\"http://localhost:9000/\\\""));
}

@Test
public void splitYamlResourceTest() throws Exception {
// Given
final URL resource = YamlUtilTest.class.getResource("/util/yaml-list.yml");
// When
final List<String> result = splitYamlResource(resource);
// Then
assertThat(result, notNullValue());
assertThat(result, hasSize(4));
assertThat(result.get(1), containsString("name: \"YAML --- 2\""));
assertThat(result.get(3), startsWith("---\nname: \"Edge case --- 1"));
}

}
40 changes: 40 additions & 0 deletions jkube-kit/common/src/test/resources/util/yaml-list.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#
# Copyright (c) 2019 Red Hat, Inc.
# This program and the accompanying materials are made
# available under the terms of the Eclipse Public License 2.0
# which is available at:
#
# https://www.eclipse.org/legal/epl-2.0/
#
# SPDX-License-Identifier: EPL-2.0
#
# Contributors:
# Red Hat, Inc. - initial API and implementation
#

---
name: YAML 1
list:
- id: 1
nested:
- element: 1
name: nested element 1
- element: 2
name: nested element 1
- id: 2
nested:
property: true
---
name: YAML --- 2
---
name: YAML 3
list:
- id: 1
- id: 2
name: nested element 2
---
name: Edge case --- 1
list:
- id: first corner ---
- id: 2
name: nested element 2

0 comments on commit 02b0410

Please sign in to comment.