From f7fae961986687c5070ac5b6543280c39f0cfe88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=98=BF=E9=AD=81?= <670569467@qq.com> Date: Wed, 22 Nov 2023 14:32:17 +0800 Subject: [PATCH] =?UTF-8?q?[ISSUE=20#314]Fix=20the=20RCE=20caused=20by=20t?= =?UTF-8?q?he=20vulnerability=20in=20YAML=20deseriali=E2=80=A6=20(#328)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 26 ++ .../util/parse/DefaultYamlConfigParse.java | 355 ++++++++---------- 2 files changed, 185 insertions(+), 196 deletions(-) diff --git a/README.md b/README.md index 49f5c2b..e0733ef 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,7 @@ Content - [4.1.1. Enable Nacos](#411-enable-nacos) - [4.1.2. Configure Change Listener method](#412-configure-change-listener-method) - [4.1.2.1. Type Conversion](#4121-type-conversion) + - [4.1.2.1.1. Type Conversion during YAML Parsing](#41211-Type Conversion during YAML Parsing) - [4.1.2.2. Timeout of Execution](#4122-timeout-of-execution) - [4.1.3. Global and Special Nacos Properties](#413-global-and-special-nacos-properties) - [4.1.4. `@NacosProperties`](#414-nacosproperties) @@ -315,8 +316,33 @@ The `UserNacosConfigConverter` class binds the `@NacosConfigListener.converter() } ``` +##### 4.1.2.1.1. Type Conversion during YAML Parsing +By default, starting from version 1.1.2, this library uses `SafeConstructor` for type conversion during YAML parsing. This is done to ensure that potentially unsafe code is not executed during the parsing process. `SafeConstructor` provides a secure construction logic for mapping YAML structures to Java objects. +**System Property Toggle** + +To maintain compatibility with versions prior to 1.1.2, we have introduced a system property toggle named `yamlAllowComplexObject`. Prior to version 1.1.2, the library defaulted to using `Constructor`, another constructor in the SnakeYAML library that supports more complex object mapping. Starting from version 1.1.2, the default is switched to `SafeConstructor`. + +**Potential Risks** + +It's important to note that enabling `Constructor` introduces some potential risks, particularly the risk of Remote Code Execution (`RCE`). This is because `Constructor` allows more flexible object construction, but it also increases the risk of handling malicious YAML input. + +**Recommendations** + +- We recommend using the `NacosConfigConverter` for custom conversions. + +**If You Must Use `Constructor`** + +- Ensure that the source of the YAML data is secure. + +**How to Disable `SafeConstructor`** + +You can set the toggle by adding a JVM system property when starting your application. For example, in the command line: + +```bash +java -DyamlAllowComplexObject=true -jar your-application.jar +``` - See [Type Conversion Sample of `@NacosConfigListener`](https://github.com/nacos-group/nacos-spring-project/blob/master/nacos-spring-samples/nacos-spring-webmvc-sample/src/main/java/com/alibaba/nacos/samples/spring/listener/PojoNacosConfigListener.java) diff --git a/nacos-spring-context/src/main/java/com/alibaba/nacos/spring/util/parse/DefaultYamlConfigParse.java b/nacos-spring-context/src/main/java/com/alibaba/nacos/spring/util/parse/DefaultYamlConfigParse.java index d043830..5c03ff7 100644 --- a/nacos-spring-context/src/main/java/com/alibaba/nacos/spring/util/parse/DefaultYamlConfigParse.java +++ b/nacos-spring-context/src/main/java/com/alibaba/nacos/spring/util/parse/DefaultYamlConfigParse.java @@ -17,15 +17,8 @@ package com.alibaba.nacos.spring.util.parse; -import java.util.AbstractMap; -import java.util.Collection; -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; -import java.util.regex.Pattern; - +import com.alibaba.nacos.api.config.ConfigType; +import com.alibaba.nacos.spring.util.AbstractConfigParse; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -33,14 +26,21 @@ import org.yaml.snakeyaml.LoaderOptions; import org.yaml.snakeyaml.Yaml; import org.yaml.snakeyaml.constructor.Constructor; +import org.yaml.snakeyaml.constructor.SafeConstructor; import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Tag; import org.yaml.snakeyaml.parser.ParserException; import org.yaml.snakeyaml.representer.Representer; import org.yaml.snakeyaml.resolver.Resolver; -import com.alibaba.nacos.api.config.ConfigType; -import com.alibaba.nacos.spring.util.AbstractConfigParse; +import java.util.AbstractMap; +import java.util.Collection; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; +import java.util.regex.Pattern; /** * DefaultYamlConfigParse. @@ -49,189 +49,152 @@ * @since 0.3.0 */ public class DefaultYamlConfigParse extends AbstractConfigParse { - - protected static final Logger LOGGER = LoggerFactory - .getLogger(DefaultYamlConfigParse.class); - - protected static Yaml createYaml() { - MapAppenderConstructor mapAppenderConstructor = new MapAppenderConstructor(); - Representer representer = new Representer(); - DumperOptions dumperOptions = new DumperOptions(); - LimitedResolver resolver = new LimitedResolver(); - LoaderOptions loaderOptions = new LoaderOptions(); - loaderOptions.setAllowDuplicateKeys(false); - return new Yaml(mapAppenderConstructor, representer, dumperOptions, loaderOptions, resolver); - } - - protected static boolean process(MatchCallback callback, Yaml yaml, String content) { - int count = 0; - if (LOGGER.isDebugEnabled()) { - LOGGER.debug("Loading from YAML: " + content); - } - for (Object object : yaml.loadAll(content)) { - if (object != null && process(asMap(object), callback)) { - count++; - } - } - if (LOGGER.isDebugEnabled()) { - LOGGER.debug("Loaded " + count + " document" + (count > 1 ? "s" : "") - + " from YAML resource: " + content); - } - return (count > 0); - } - - protected static boolean process(Map map, MatchCallback callback) { - if (LOGGER.isDebugEnabled()) { - LOGGER.debug("Merging document (no matchers set): " + map); - } - callback.process(getFlattenedMap(map)); - return true; - } - - @SuppressWarnings("unchecked") - protected static Map asMap(Object object) { - // YAML can have numbers as keys - Map result = new LinkedHashMap(); - if (!(object instanceof Map)) { - // A document can be a text literal - result.put("document", object); - return result; - } - - Map map = (Map) object; - for (Map.Entry entry : map.entrySet()) { - Object key = entry.getKey(); - Object value = entry.getValue(); - if (value instanceof Map) { - value = asMap(value); - } - if (key instanceof CharSequence) { - result.put(key.toString(), value); - } - else { - result.put("[" + key.toString() + "]", value); - } - } - return result; - } - - private static class LimitedResolver extends Resolver { - - @Override - public void addImplicitResolver(Tag tag, Pattern regexp, String first) { - if (tag == Tag.TIMESTAMP) { - return; - } - super.addImplicitResolver(tag, regexp, first); - } - } - - protected static Map getFlattenedMap(Map source) { - Map result = new LinkedHashMap(); - buildFlattenedMap(result, source, null); - return result; - } - - protected static void buildFlattenedMap(Map result, - Map source, String path) { - for (Map.Entry entry : source.entrySet()) { - String key = entry.getKey(); - if (!StringUtils.isBlank(path)) { - if (key.startsWith("[")) { - key = path + key; - } - else { - key = path + '.' + key; - } - } - Object value = entry.getValue(); - if (value instanceof String) { - result.put(key, value); - } - else if (value instanceof Map) { - // Need a compound key - @SuppressWarnings("unchecked") - Map map = (Map) value; - buildFlattenedMap(result, map, key); - } - else if (value instanceof Collection) { - // Need a compound key - @SuppressWarnings("unchecked") - Collection collection = (Collection) value; - int count = 0; - for (Object object : collection) { - buildFlattenedMap(result, - Collections.singletonMap("[" + (count++) + "]", object), key); - } - } - else { - result.put(key, (value != null ? value.toString() : "")); - } - } - } - - @Override - public Map parse(String configText) { - final AtomicReference> result = new AtomicReference>(); - process(new MatchCallback() { - @Override - public void process(Map map) { - result.set(map); - } - }, createYaml(), configText); - return result.get(); - } - - @Override - public String processType() { - return ConfigType.YAML.getType(); - } - - protected interface MatchCallback { - - /** - * Put Map to Properties. - * - * @param map {@link Map} - */ - void process(Map map); - } - - protected static class MapAppenderConstructor extends Constructor { - - MapAppenderConstructor() { - super(); - } - - @Override - protected Map constructMapping(MappingNode node) { - try { - return super.constructMapping(node); - } - catch (IllegalStateException ex) { - throw new ParserException("while parsing MappingNode", - node.getStartMark(), ex.getMessage(), node.getEndMark()); - } - } - - @Override - protected Map createDefaultMap() { - final Map delegate = super.createDefaultMap(); - return new AbstractMap() { - @Override - public Object put(Object key, Object value) { - if (delegate.containsKey(key)) { - throw new IllegalStateException("Duplicate key: " + key); - } - return delegate.put(key, value); - } - - @Override - public Set> entrySet() { - return delegate.entrySet(); - } - }; - } - } - + + protected static final Logger LOGGER = LoggerFactory.getLogger(DefaultYamlConfigParse.class); + + private static final String YAML_ALLOW_COMPLEX_OBJECT = "yamlAllowComplexObject"; + + private static boolean getYamlAllowComplexObject() { + return Boolean.getBoolean(YAML_ALLOW_COMPLEX_OBJECT); + } + + protected static Yaml createYaml() { + SafeConstructor constructor; + if (getYamlAllowComplexObject()) { + constructor = new Constructor(); + } else { + constructor = new SafeConstructor(); + } + Representer representer = new Representer(); + DumperOptions dumperOptions = new DumperOptions(); + LimitedResolver resolver = new LimitedResolver(); + LoaderOptions loaderOptions = new LoaderOptions(); + loaderOptions.setAllowDuplicateKeys(false); + return new Yaml(constructor, representer, dumperOptions, loaderOptions, resolver); + } + + protected static boolean process(MatchCallback callback, Yaml yaml, String content) { + int count = 0; + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("Loading from YAML: " + content); + } + for (Object object : yaml.loadAll(content)) { + if (object != null && process(asMap(object), callback)) { + count++; + } + } + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("Loaded " + count + " document" + (count > 1 ? "s" : "") + " from YAML resource: " + content); + } + return (count > 0); + } + + protected static boolean process(Map map, MatchCallback callback) { + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("Merging document (no matchers set): " + map); + } + callback.process(getFlattenedMap(map)); + return true; + } + + @SuppressWarnings("unchecked") + protected static Map asMap(Object object) { + // YAML can have numbers as keys + Map result = new LinkedHashMap(); + if (!(object instanceof Map)) { + // A document can be a text literal + result.put("document", object); + return result; + } + + Map map = (Map) object; + for (Map.Entry entry : map.entrySet()) { + Object key = entry.getKey(); + Object value = entry.getValue(); + if (value instanceof Map) { + value = asMap(value); + } + if (key instanceof CharSequence) { + result.put(key.toString(), value); + } else { + result.put("[" + key.toString() + "]", value); + } + } + return result; + } + + private static class LimitedResolver extends Resolver { + + @Override + public void addImplicitResolver(Tag tag, Pattern regexp, String first) { + if (tag == Tag.TIMESTAMP) { + return; + } + super.addImplicitResolver(tag, regexp, first); + } + } + + protected static Map getFlattenedMap(Map source) { + Map result = new LinkedHashMap(); + buildFlattenedMap(result, source, null); + return result; + } + + protected static void buildFlattenedMap(Map result, Map source, String path) { + for (Map.Entry entry : source.entrySet()) { + String key = entry.getKey(); + if (!StringUtils.isBlank(path)) { + if (key.startsWith("[")) { + key = path + key; + } else { + key = path + '.' + key; + } + } + Object value = entry.getValue(); + if (value instanceof String) { + result.put(key, value); + } else if (value instanceof Map) { + // Need a compound key + @SuppressWarnings("unchecked") Map map = (Map) value; + buildFlattenedMap(result, map, key); + } else if (value instanceof Collection) { + // Need a compound key + @SuppressWarnings("unchecked") Collection collection = (Collection) value; + int count = 0; + for (Object object : collection) { + buildFlattenedMap(result, Collections.singletonMap("[" + (count++) + "]", object), key); + } + } else { + result.put(key, (value != null ? value.toString() : "")); + } + } + } + + @Override + public Map parse(String configText) { + final AtomicReference> result = new AtomicReference>(); + process(new MatchCallback() { + @Override + public void process(Map map) { + result.set(map); + } + }, createYaml(), configText); + return result.get(); + } + + @Override + public String processType() { + return ConfigType.YAML.getType(); + } + + protected interface MatchCallback { + + /** + * Put Map to Properties. + * + * @param map {@link Map} + */ + void process(Map map); + } + }