From cb17a9ea613bc87d356ce54bfeaf02cab4c1aba1 Mon Sep 17 00:00:00 2001 From: manusa Date: Thu, 26 Mar 2020 08:50:02 +0100 Subject: [PATCH] fix: CWE 502 Deserialization of Untrusted Data + fix yaml list resource split Signed-off-by: Marc Nuri --- .github/workflows/integration-tests.yml | 3 +- CHANGELOG.md | 1 + .../kit/common/util/SpringBootUtilTest.java | 2 +- .../jkube/kit/common/util/ThorntailUtil.java | 6 +- .../jkube/kit/common/util/YamlUtil.java | 95 +++++++++------- .../kit/common/util/ThorntailUtilTest.java | 13 +-- .../jkube/kit/common/util/YamlUtilTest.java | 107 ++++++++++++++++++ .../src/test/resources/util/yaml-list.yml | 40 +++++++ 8 files changed, 212 insertions(+), 55 deletions(-) create mode 100644 jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/YamlUtilTest.java create mode 100644 jkube-kit/common/src/test/resources/util/yaml-list.yml diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 4690e5e3eb..42c78233f7 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -101,9 +101,10 @@ jobs: - name: Checkout uses: actions/checkout@v2 - name: Setup OpenShift - uses: manusa/actions-setup-openshift@v1.0.1 + uses: manusa/actions-setup-openshift@v1.0.3 with: oc version: ${{ matrix.openshift }} + github token: ${{ secrets.GITHUB_TOKEN }} - name: Setup Java 11 uses: actions/setup-java@v1 with: diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e4e5196ba..04b4a81497 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/jkube-kit/common-maven/src/test/java/org/eclipse/jkube/kit/common/util/SpringBootUtilTest.java b/jkube-kit/common-maven/src/test/java/org/eclipse/jkube/kit/common/util/SpringBootUtilTest.java index a1557d6559..f216cad51b 100644 --- a/jkube-kit/common-maven/src/test/java/org/eclipse/jkube/kit/common/util/SpringBootUtilTest.java +++ b/jkube-kit/common-maven/src/test/java/org/eclipse/jkube/kit/common/util/SpringBootUtilTest.java @@ -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")); } diff --git a/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ThorntailUtil.java b/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ThorntailUtil.java index 67604e8eaf..076200922d 100644 --- a/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ThorntailUtil.java +++ b/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ThorntailUtil.java @@ -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 @@ -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); } } diff --git a/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/YamlUtil.java b/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/YamlUtil.java index fdacd7f683..bc6a0e6c77 100644 --- a/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/YamlUtil.java +++ b/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/YamlUtil.java @@ -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 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 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. @@ -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 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 source = YAML_MAPPER.readValue( + Optional.ofNullable(yamlString).filter(StringUtils::isNotBlank).orElse(EMPTY_YAML), + new TypeReference>() { + }); + if (source != null) { + properties.putAll(getFlattenedMap(source)); } + return properties; + } - public static List 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 splitYamlResource(URL resource) throws IOException { + final List serializedYamlList = new ArrayList<>(); + final List> parsedList = YAML_MAPPER.readValues( + YAML_FACTORY.createParser(resource), new TypeReference>() { + }).readAll(); + for (Map listEntry : parsedList) { + serializedYamlList.add(YAML_MAPPER.writeValueAsString(listEntry)); } + return serializedYamlList; + } - public static String getYamlFromYamlList(String pattern, List yamlAsStringList) { + static String getYamlFromYamlList(String pattern, List 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(); } } diff --git a/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/ThorntailUtilTest.java b/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/ThorntailUtilTest.java index a63542c6f5..a1f729b8c6 100644 --- a/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/ThorntailUtilTest.java +++ b/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/ThorntailUtilTest.java @@ -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")); + } } diff --git a/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/YamlUtilTest.java b/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/YamlUtilTest.java new file mode 100644 index 0000000000..981cc3cfff --- /dev/null +++ b/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/YamlUtilTest.java @@ -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 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")); + } + +} \ No newline at end of file diff --git a/jkube-kit/common/src/test/resources/util/yaml-list.yml b/jkube-kit/common/src/test/resources/util/yaml-list.yml new file mode 100644 index 0000000000..e524f6e172 --- /dev/null +++ b/jkube-kit/common/src/test/resources/util/yaml-list.yml @@ -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