Skip to content

Commit

Permalink
updated the fix for CVE-2023-24620 and CVE-2023-24621 to disable by d…
Browse files Browse the repository at this point in the history
…efault the insecure logic.

Updated the readme and added a security.md
updated the version of junit.
  • Loading branch information
JoeBeeContrast committed Aug 2, 2023
1 parent 33a0f91 commit ca86928
Show file tree
Hide file tree
Showing 16 changed files with 301 additions and 135 deletions.
49 changes: 48 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,51 @@
## YamlBeans
# YamlBeans

## Contrast Security Fork
This fork was created to make and push the fixes for CVE-2023-24620 and CVE-2023-24621 to maven as the maintainer of com.esoteric:yamlbeans no longer has the ability to push to maven.
More details of the vulnerabilities can be found at [SECURITY.md](SECURITY.md)
### Upgrading from com.esoteric:yamlbeans to com.contrast:yamlbeans
It "should" be easy to upgrade to the fixed version of yamlbeans. The API has not changed, and so for most user's updating the yamlbeans dependency should be enough.
However the fix for the two CVEs has changed the underlying logic of the library slightly.

#### Class Tags
If you are ingesting YAML that utilises class tag e.g
```yaml
!com.example.data.SomeClass
var1: "some data"
var2: "some more data"
```
YAMLBeans will no longer read this, this is due to the fix for CVE-2023-24621 which disables this functionality by default.
This type of user controlled polymorphic deserialization is inherently unsafe and is strongly discouraged.
A safe alternative is to specify in the code which class YamlBeans should deserialize to e.g

```java
YamlReader reader = new YamlReader(new StringReader(yamlFile));
SomeClass data = reader.read(SomeClass.class);
```
If it is not possible in your use case, it is possible to re-enable the unsafe polymorphic deserialization by doing the following.
```java
YamlReader reader = new YamlReader(new StringReader(yamlFile), new UnsafeYamlConfig());
SomeClass data = (SomeClass) reader.read();
```
Or to write them
```java
YamlWriter writer = new YamlWriter(yamlFile, new UnsafeYamlConfig());
```
This should only be done if it is not possible to use the above example where the code specifies the class to be deserialized to and
only then when the YAML document being read is from a trusted source.

#### Anchors
Anchors are by default disabled, this fixes the Denial of Service Vulnerability CVE-2023-24620.
If they are required and the YAML document is from a trusted source, it is possible to re-enable them using
```java
YamlReader reader = new YamlReader(new StringReader(yamlFile), new UnsafeYamlConfig());
```
Or to write them
```java
YamlWriter writer = new YamlWriter(yamlFile, new UnsafeYamlConfig());
```



![Maven Central](https://maven-badges.herokuapp.com/maven-central/com.esotericsoftware.yamlbeans/yamlbeans/badge.svg)

Expand Down
68 changes: 68 additions & 0 deletions SECURITY.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Security Policy

## Reporting a Vulnerability

To report a vulnerability please contact security@contrastsecurity.com and please see our
[Vulnerability Disclosure Policy
](https://www.contrastsecurity.com/disclosure-policy)


## CVE-2023-24620 Denial of Service

It is possible to perform a Denial of Service attack against an application using YamlBeans. This is through a entity expansion attack ( Billion Laughs ) similar to SnakeYaml's CVE-2017-18640.
The following is an example of a YAML Document
### POC
```yaml
lol1: &lol1 ["lol","lol","lol","lol","lol","lol","lol","lol","lol"]
lol2: &lol2 [*lol1,*lol1,*lol1,*lol1,*lol1,*lol1,*lol1,*lol1,*lol1,*lol1]
lol3: &lol3 [*lol2,*lol2,*lol2,*lol2,*lol2,*lol2,*lol2,*lol2,*lol2]
lol4: &lol4 [*lol3,*lol3,*lol3,*lol3,*lol3,*lol3,*lol3,*lol3,*lol3]
lol5: &lol5 [*lol4,*lol4,*lol4,*lol4,*lol4,*lol4,*lol4,*lol4,*lol4]
lol6: &lol6 [*lol5,*lol5,*lol5,*lol5,*lol5,*lol5,*lol5,*lol5,*lol5]
lol7: &lol7 [*lol6,*lol6,*lol6,*lol6,*lol6,*lol6,*lol6,*lol6,*lol6]
lol8: &lol8 [*lol7,*lol7,*lol7,*lol7,*lol7,*lol7,*lol7,*lol7,*lol7]
lol9: &lol9 [*lol8,*lol8,*lol8,*lol8,*lol8,*lol8,*lol8,*lol8,*lol8]
lolz: &lolz [*lol9]
```
In the following code the Denial of Service occurs at the point the data is traversed on the line System.out.Println() as data.toString() is implicitly called, which reads through all elements to generate the toString().
This would also happen if any sort of recursive traversing of the data structure occured.
```java
@PostMapping("/loadYaml")
public void loadYaml(@RequestBody String yamlFile) throws YamlException {
YamlReader reader = new YamlReader(new StringReader(yamlFile));
Map<String, ?> data = (Map<String, ?>) reader.read();
System.out.println(data);
}
```



## CVE-2023-24621 Untrusted Polymorphic Deserialization to Java Classes
Within YamlBeans it is possible for the YAML file to contain the Java Class the data will be deserialized to. The class name in the YAML file overrides any class specified by the developer. For example
### POC
```java
@PostMapping("/loadYamlAsSpecificClass")
public void loadYamlAsSpecificClass(@RequestBody String yamlFile) throws YamlException {
YamlReader reader = new YamlReader(new StringReader(yamlFile));
Config data = reader.read(Config.class);
System.out.println(data);
}
```

The developer, in the above example would reasonably expect that the data was deserialized to the Config.class. This does not occur if the YAML contains a reference to another class.
The value in the YAML overrides both `reader.read(Config.class);` and `reader.read();`
```yaml
!com.contrast.labs.yamlbeanspoc.Gadget
cmd: "open /System/Applications/Calculator.app"
```
In the above YAML the class `com.contrast.labs.yamlbeanspoc.Gadget` is instantiated instead of Config.class, and the variable "cmd" is set via it's setter method `setCmd(String value)`

#### Potential Gadget Classes
By default YamlBeans expects the class to follow the JavaBeans spec of having :
* Public No Args Constructor
* Private variables.
* Getters/Setters that follow the JavaBean spec. e.g that match the variable name, but with getVariableName() setVariableName().

If they are missing or the setter does not match the underlying variable name, it will not set that value.
This is similar to Jackson-Databind’s polymorphic deserialization. But Jackson does not check the underlying variable name. Just looks for setter method and calls it with the supplied value. But within that list there should be several Jackson-Databind gadgets that can be reused.

37 changes: 18 additions & 19 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@
</parent>

<name>YamlBeans</name>
<groupId>com.esotericsoftware.yamlbeans</groupId>
<groupId>com.contrast.yamlbeans</groupId>
<artifactId>yamlbeans</artifactId>
<version>1.16-SNAPSHOT</version>
<version>1.17-SNAPSHOT</version>

<description>Java object graphs, to and from YAML</description>
<url>https://github.com/EsotericSoftware/yamlbeans</url>
<url>https://github.com/Contrast-Security-OSS/yamlbeans</url>

<licenses>
<license>
Expand All @@ -25,9 +25,9 @@
</license>
</licenses>
<scm>
<url>scm:git:git@github.com:esotericsoftware/yamlbeans.git</url>
<connection>scm:git:git@github.com:esotericsoftware/yamlbeans.git</connection>
<developerConnection>scm:git:git@github.com:esotericsoftware/yamlbeans.git</developerConnection>
<url>git@github.com:Contrast-Security-OSS/yamlbeans.git</url>
<connection>git@github.com:Contrast-Security-OSS/yamlbeans.git</connection>
<developerConnection>git@github.com:Contrast-Security-OSS/yamlbeans.git</developerConnection>
<tag>HEAD</tag>
</scm>

Expand All @@ -36,7 +36,7 @@
<scope>test</scope>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.8.2</version>
<version>4.13.2</version>
</dependency>
</dependencies>

Expand Down Expand Up @@ -120,20 +120,19 @@
<role>author</role>
</roles>
</developer>
</developers>

<contributors>
<contributor>
<name>Tony Chemit</name>
<email>dev@tchemit.fr</email>
<organization>Ultreia</organization>
<organizationUrl>http://ultreia.io</organizationUrl>
<timezone>Europe/Paris</timezone>
<developer>
<id>JoeBeeContrast</id>
<name>Joseph Beeton</name>
<email>joseph.beeton@</email>
<organization>Contrast Security</organization>
<organizationUrl>https://contrastsecurity.com</organizationUrl>
<roles>
<role>Maven packager</role>
<role>developer</role>
</roles>
</contributor>
</contributors>
</developer>
</developers>



<profiles>
<profile>
Expand Down
42 changes: 0 additions & 42 deletions src/com/esotericsoftware/yamlbeans/SafeYamlConfig.java

This file was deleted.

42 changes: 42 additions & 0 deletions src/com/esotericsoftware/yamlbeans/UnsafeYamlConfig.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@

package com.esotericsoftware.yamlbeans;

/** UnsafeYamlConfig extends YamlConfig and enables class tags and anchors. If this config is used, it opens the user
* to Denial of Service and Deserialization attacks. Only use this if you trust the author of the YAML document being
* read and utilise the class tag or anchor functionality.
* <p>
* Usage :
* <pre>
* UnsafeYamlConfig config = new UnsafeYamlConfig();
* YamlReader reader = new YamlReader(yamlData.toString(), config);
* Data data = reader.read();
* </pre>
*/
public class UnsafeYamlConfig extends YamlConfig {
public UnsafeYamlConfig() {
super.readConfig = new UnsafeReadConfig();
super.writeConfig = new UnsafeWriteConfig();
}

static public class UnsafeReadConfig extends ReadConfig {
public UnsafeReadConfig () {
super.anchors = true;
super.classTags = true;
}

public void setClassTags (boolean classTags) {
super.classTags = classTags;
}

public void setAnchors (boolean anchors) {
super.anchors = anchors;
}
}

static public class UnsafeWriteConfig extends WriteConfig {
public UnsafeWriteConfig () {
super.autoAnchor = true;
super.writeClassName = WriteClassName.AUTO;
}
}
}
16 changes: 8 additions & 8 deletions src/com/esotericsoftware/yamlbeans/YamlConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* @author <a href="mailto:misc@n4te.com">Nathan Sweet</a> */
public class YamlConfig {
/** Configuration for writing YAML. */
public final WriteConfig writeConfig = new WriteConfig();
public WriteConfig writeConfig = new WriteConfig();

/** Configuration for reading YAML. */
public ReadConfig readConfig = new ReadConfig();
Expand All @@ -50,7 +50,7 @@ public class YamlConfig {
boolean allowDuplicates = true;
String tagSuffix;

public YamlConfig () {
public YamlConfig() {
scalarSerializers.put(Date.class, new DateSerializer());

tagToClass.put("tag:yaml.org,2002:str", String.class);
Expand Down Expand Up @@ -137,9 +137,9 @@ static public class WriteConfig {
boolean writeDefaultValues = false;
boolean writeRootTags = true;
boolean writeRootElementTags = true;
boolean autoAnchor = true;
boolean autoAnchor = false;
boolean keepBeanPropertyOrder = false;
WriteClassName writeClassName = WriteClassName.AUTO;
WriteClassName writeClassName = WriteClassName.NEVER;
Quote quote = Quote.NONE;
Version version;
Map<String, String> tags;
Expand Down Expand Up @@ -265,9 +265,9 @@ static public class ReadConfig {
final Map<Class, ConstructorParameters> constructorParameters = new IdentityHashMap();
boolean ignoreUnknownProperties;
boolean autoMerge = true;
boolean classTags = true;
boolean classTags = false;
boolean guessNumberTypes;
boolean anchors = true;
boolean anchors = false;

ReadConfig () {
}
Expand Down Expand Up @@ -308,7 +308,7 @@ public void setIgnoreUnknownProperties (boolean allowUnknownProperties) {

/** When false, tags are not used to look up classes. Default is true. */
public void setClassTags (boolean classTags) {
this.classTags = classTags;
if (classTags) throw new IllegalArgumentException("Class Tags cannot be enabled in YamlConfig, use UnsafeYamlConfig instead.");
}

/** When false, the merge key (<<) is not used to merge values into the current map. Default is true. */
Expand All @@ -324,7 +324,7 @@ public void setGuessNumberTypes (boolean guessNumberTypes) {

/** When false, anchors in the YAML are ignored. Default is true. */
public void setAnchors (boolean anchors) {
this.anchors = anchors;
if (anchors) throw new IllegalArgumentException("Anchors cannot be enabled in YamlConfig, use UnsafeYamlConfig instead.");
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/com/esotericsoftware/yamlbeans/YamlReader.java
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ private Class<?> chooseType (String tag, Class<?> defaultType, Class<?> provided
try {
Class<?> loadedFromTag = findTagClass(tag, classLoader);
if (loadedFromTag != null) {
if (!providedType.isAssignableFrom(loadedFromTag)) {
if (providedType!=null&&!providedType.isAssignableFrom(loadedFromTag)) {
throw new YamlReaderException("Class specified by tag is incompatible with expected type: "
+ loadedFromTag.getName() + " (expected " + providedType.getName() + ")");
}
Expand Down
8 changes: 4 additions & 4 deletions test/com/esotericsoftware/yamlbeans/MergeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public class MergeTest {
public void testMerge() throws FileNotFoundException, YamlException {
InputStream input = new FileInputStream("test/test-merge.yml");
Reader reader = new InputStreamReader(input);
Map data = new YamlReader(reader).read(Map.class);
Map data = new YamlReader(reader, new UnsafeYamlConfig()).read(Map.class);
Map stuff = (Map)data.get("merged");
assertEquals("v1", stuff.get("v1"));
assertEquals("v2", stuff.get("v2"));
Expand All @@ -35,7 +35,7 @@ public void testMergeMultipleMaps() throws YamlException {
sb.append("test1: &1\n").append(" - key1: value1\n").append(" - key2: value2\n");
sb.append("test2:\n").append(" << : *1");

YamlReader yamlReader = new YamlReader(sb.toString());
YamlReader yamlReader = new YamlReader(sb.toString(), new UnsafeYamlConfig());
Map<String, Object> map = (Map<String, Object>) yamlReader.read();
assertEquals("value1", ((Map<String, String>) map.get("test2")).get("key1"));
assertEquals("value2", ((Map<String, String>) map.get("test2")).get("key2"));
Expand All @@ -49,7 +49,7 @@ public void testMergeUpdateValue() throws YamlException {

sb.append("test2:\n").append(" key2: value22\n").append(" << : *1\n").append(" key3: value33\n");

YamlReader yamlReader = new YamlReader(sb.toString());
YamlReader yamlReader = new YamlReader(sb.toString(), new UnsafeYamlConfig());
Map<String, Object> map = (Map<String, Object>) yamlReader.read();
assertEquals("value1", ((Map<String, String>) map.get("test2")).get("key1"));
assertEquals("value22", ((Map<String, String>) map.get("test2")).get("key2"));
Expand All @@ -62,7 +62,7 @@ public void testMergeExpectThrowYamlReaderException() throws YamlException {
sb.append("test1: &1 123\n");
sb.append("<< : *1\n");

YamlReader yamlReader = new YamlReader(sb.toString());
YamlReader yamlReader = new YamlReader(sb.toString(), new UnsafeYamlConfig());
try {
yamlReader.read();
fail("Expected a mapping or a sequence of mappings");
Expand Down

0 comments on commit ca86928

Please sign in to comment.