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