From fa99814967c44e654aae64802947209e2817359f Mon Sep 17 00:00:00 2001

From: Severin Gehwolf <sgehwolf@redhat.com>

Date: Tue, 6 Dec 2022 17:32:30 +0100

Subject: [PATCH] Fix CVE-2022-1471 and add a test

---

 .../java/io/prometheus/jmx/JmxCollector.java  | 34 +++++++++----

 .../io/prometheus/jmx/JmxCollectorTest.java   | 51 +++++++++++++++++++

 .../test/java/io/prometheus/jmx/Person.java   | 38 ++++++++++++++

 .../test/resources/testyaml_javabean.config   |  6 +++

 4 files changed, 119 insertions(+), 10 deletions(-)

 create mode 100644 collector/src/test/java/io/prometheus/jmx/Person.java

 create mode 100644 collector/src/test/resources/testyaml_javabean.config

diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java

index a5fc96f..38f13ed 100644

--- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java

+++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java

@@ -1,12 +1,7 @@

 package io.prometheus.jmx;

-import io.prometheus.client.Collector;

-import io.prometheus.client.Counter;

-import org.yaml.snakeyaml.Yaml;

-import org.yaml.snakeyaml.error.YAMLException;

+import static java.lang.String.format;

-import javax.management.MalformedObjectNameException;

-import javax.management.ObjectName;

 import java.io.File;

 import java.io.FileReader;

 import java.io.IOException;

@@ -25,7 +20,19 @@ import java.util.logging.Logger;

 import java.util.regex.Matcher;

 import java.util.regex.Pattern;

-import static java.lang.String.format;

+import javax.management.MalformedObjectNameException;

+import javax.management.ObjectName;

+

+import org.yaml.snakeyaml.DumperOptions;

+import org.yaml.snakeyaml.LoaderOptions;

+import org.yaml.snakeyaml.Yaml;

+import org.yaml.snakeyaml.constructor.SafeConstructor;

+import org.yaml.snakeyaml.error.YAMLException;

+import org.yaml.snakeyaml.representer.Representer;

+import org.yaml.snakeyaml.resolver.Resolver;

+

+import io.prometheus.client.Collector;

+import io.prometheus.client.Counter;

 public class JmxCollector extends Collector implements Collector.Describable {

     static final Counter configReloadSuccess = Counter.build()

@@ -76,7 +83,7 @@ public class JmxCollector extends Collector implements Collector.Describable {

         // will be thrown for bad configs

         Map<String, Object> yamlConfig = null;

         try {

-            yamlConfig = (Map<String, Object>)new Yaml().load(new FileReader(in));

+            yamlConfig = (Map<String, Object>)createYamlInstance().load(new FileReader(in));

         } catch (YAMLException e) {

             System.err.println("YAML configuration error: " + e.getMessage());

             throw new IllegalArgumentException(e);

@@ -86,7 +93,14 @@ public class JmxCollector extends Collector implements Collector.Describable {

     }

     public JmxCollector(String yamlConfig) throws MalformedObjectNameException {

-        config = loadConfig((Map<String, Object>)new Yaml().load(yamlConfig));

+        config = loadConfig((Map<String, Object>)createYamlInstance().load(yamlConfig));

+    }

+

+    private static Yaml createYamlInstance() {

+        // Equivalent to default Yaml() constructor but using SafeConstructor()

+        // over Constructor();

+        return new Yaml(new SafeConstructor(), new Representer(), new DumperOptions(), new LoaderOptions(),

+                new Resolver());

     }

     private void reloadConfig() {

@@ -94,7 +108,7 @@ public class JmxCollector extends Collector implements Collector.Describable {

         FileReader fr = new FileReader(configFile);

         try {

-          Map<String, Object> newYamlConfig = (Map<String, Object>)new Yaml().load(fr);

+          Map<String, Object> newYamlConfig = (Map<String, Object>)createYamlInstance().load(fr);

           config = loadConfig(newYamlConfig);

           config.lastUpdate = configFile.lastModified();

           configReloadSuccess.inc();

diff --git a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java

index 3d4ecf0..f499fb3 100644

--- a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java

+++ b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java

@@ -8,12 +8,15 @@ import static org.junit.Assert.fail;

 import java.io.File;

 import java.lang.management.ManagementFactory;

+import java.nio.file.Files;

+import java.util.stream.Collectors;

 import javax.management.MBeanServer;

 import org.junit.Before;

 import org.junit.BeforeClass;

 import org.junit.Test;

+import org.yaml.snakeyaml.constructor.ConstructorException;

 import org.yaml.snakeyaml.error.YAMLException;

 import io.prometheus.client.Collector;

@@ -272,4 +275,52 @@ public class JmxCollectorTest {

         assertEquals(prefix + "Number of aliases for non-scalar nodes exceeds the specified max=50", e.getMessage());

       }

     }

+

+	@Test

+	public void javaBeanLoadTestFile() throws Exception {

+	    try {

+	        File configFile = new File(getClass().getResource("/testyaml_javabean.config").getPath());

+	        doJavaBeanLoadTest(configFile, false);

+	    } finally {

+	        Person.fixtureReset();

+	    }

+	}

+

+	@Test

+    public void javaBeanLoadTestString() throws Exception {

+	    try {

+	        File configFile = new File(getClass().getResource("/testyaml_javabean.config").getPath());

+	        doJavaBeanLoadTest(configFile, true);

+	    } finally {

+	        Person.fixtureReset();

+	    }

+    }

+

+    private void doJavaBeanLoadTest(File configFile, boolean passAsString) throws Exception {

+        assertTrue(configFile.exists());

+        assertEquals(0, Person.loadCount.get());

+        try {

+            if (passAsString) {

+                String config = Files.readAllLines(configFile.toPath()).stream().collect(Collectors.joining("\n"));

+                new JmxCollector(config);

+            } else {

+                new JmxCollector(configFile);

+            }

+            assertEquals("Expected Person class to not be instantiated", 0, Person.loadCount.get());

+        } catch (ConstructorException e) {

+            String problem = e.getProblem();

+            assertTrue(problem.contains(Person.class.getName()));

+            assertTrue(problem.contains("could not determine a constructor"));

+        } catch (IllegalArgumentException e) {

+            Throwable cause = e.getCause();

+            if (cause instanceof ConstructorException) {

+                ConstructorException ce = (ConstructorException) cause;

+                String problem = ce.getProblem();

+                assertTrue(problem.contains(Person.class.getName()));

+                assertTrue(problem.contains("could not determine a constructor"));

+            } else {

+                fail("Expected ConstructorException as cause!");

+            }

+        }

+    }

 }

diff --git a/collector/src/test/java/io/prometheus/jmx/Person.java b/collector/src/test/java/io/prometheus/jmx/Person.java

new file mode 100644

index 0000000..e3b339a

--- /dev/null

+++ b/collector/src/test/java/io/prometheus/jmx/Person.java

@@ -0,0 +1,38 @@

+package io.prometheus.jmx;

+

+import java.util.concurrent.atomic.AtomicInteger;

+

+/**

+ * Simple java bean (used by yaml load test)

+ *

+ */

+public class Person {

+

+    public static final AtomicInteger loadCount = new AtomicInteger(0);

+

+    private String firstName;

+    private String lastName;

+

+    public String getFirstName() {

+        return firstName;

+    }

+

+    public void setFirstName(String firstName) {

+        loadCount.incrementAndGet();

+        this.firstName = firstName;

+    }

+

+    public String getLastName() {

+        return lastName;

+    }

+

+    public void setLastName(String lastName) {

+        loadCount.incrementAndGet();

+        this.lastName = lastName;

+    }

+

+    public static void fixtureReset() {

+        loadCount.set(0);

+    }

+

+}

diff --git a/collector/src/test/resources/testyaml_javabean.config b/collector/src/test/resources/testyaml_javabean.config

new file mode 100644

index 0000000..6552d83

--- /dev/null

+++ b/collector/src/test/resources/testyaml_javabean.config

@@ -0,0 +1,6 @@

+lowercaseOutputName: true

+lowercaseOutputLabelNames: true

+blacklistObjectNames:

+# handled by agent's default exporter

+- "java.lang:*"

+other: !!io.prometheus.jmx.Person { firstName: Andrey, lastName: Tool }

-- 

2.38.1