From fa99814967c44e654aae64802947209e2817359f Mon Sep 17 00:00:00 2001 From: Severin Gehwolf 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 yamlConfig = null; try { - yamlConfig = (Map)new Yaml().load(new FileReader(in)); + yamlConfig = (Map)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)new Yaml().load(yamlConfig)); + config = loadConfig((Map)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 newYamlConfig = (Map)new Yaml().load(fr); + Map newYamlConfig = (Map)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