Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the XML nesting depth for CVE-2022-45688 #720

Merged
merged 5 commits into from
Feb 6, 2023
Merged

Limit the XML nesting depth for CVE-2022-45688 #720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f566a1d
fix: limit the nesting depth
cleydyr Jan 31, 2023
a14cb12
refactor: keep consistence with other tests and tidy up constant
cleydyr Feb 1, 2023
651511f
tests: add new test to verify that an XML having the permitted nestin…
cleydyr Feb 1, 2023
eb56704
fix: set default maximum nesting depth as 512
cleydyr Feb 2, 2023
448e204
docs: remove wrong description of parse method
cleydyr Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/main/java/org/json/XML.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ public static void noSpace(String string) throws JSONException {
* @return true if the close tag is processed.
* @throws JSONException
*/
private static boolean parse(XMLTokener x, JSONObject context, String name, XMLParserConfiguration config)
private static boolean parse(XMLTokener x, JSONObject context, String name, XMLParserConfiguration config, int currentNestingDepth)
throws JSONException {
char c;
int i;
Expand Down Expand Up @@ -402,7 +402,11 @@ private static boolean parse(XMLTokener x, JSONObject context, String name, XMLP

} else if (token == LT) {
// Nested element
if (parse(x, jsonObject, tagName, config)) {
if (currentNestingDepth == config.getMaxNestingDepth()) {
throw x.syntaxError("Maximum nesting depth of " + config.getMaxNestingDepth() + " reached");
}

if (parse(x, jsonObject, tagName, config, currentNestingDepth + 1)) {
if (config.getForceList().contains(tagName)) {
// Force the value to be an array
if (jsonObject.length() == 0) {
Expand Down Expand Up @@ -655,7 +659,7 @@ public static JSONObject toJSONObject(Reader reader, XMLParserConfiguration conf
while (x.more()) {
x.skipPast("<");
if(x.more()) {
parse(x, jo, null, config);
parse(x, jo, null, config, 0);
}
}
return jo;
Expand Down
46 changes: 46 additions & 0 deletions src/main/java/org/json/XMLParserConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@
*/
@SuppressWarnings({""})
public class XMLParserConfiguration {
/**
* Used to indicate there's no defined limit to the maximum nesting depth when parsing a XML
* document to JSON.
*/
public static final int UNDEFINED_MAXIMUM_NESTING_DEPTH = -1;

/**
* The default maximum nesting depth when parsing a XML document to JSON.
*/
public static final int DEFAULT_MAXIMUM_NESTING_DEPTH = 512;

/** Original Configuration of the XML Parser. */
public static final XMLParserConfiguration ORIGINAL
= new XMLParserConfiguration();
Expand Down Expand Up @@ -54,6 +65,12 @@ public class XMLParserConfiguration {
*/
private Set<String> forceList;

/**
* When parsing the XML into JSON, specifies the tags whose values should be converted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is incorrect (just a duplicate from the previous section) - suggested content:

The maximum nesting depth when parsing a XML document to JSON.

* to arrays
*/
private int maxNestingDepth = DEFAULT_MAXIMUM_NESTING_DEPTH;

/**
* Default parser configuration. Does not keep strings (tries to implicitly convert
* values), and the CDATA Tag Name is "content".
Expand Down Expand Up @@ -297,4 +314,33 @@ public XMLParserConfiguration withForceList(final Set<String> forceList) {
newConfig.forceList = Collections.unmodifiableSet(cloneForceList);
return newConfig;
}

/**
* The maximum nesting depth that the parser will descend before throwing an exception
* when parsing the XML into JSON.
* @return the maximum nesting depth set for this configuration
*/
public int getMaxNestingDepth() {
return maxNestingDepth;
}

/**
* Defines the maximum nesting depth that the parser will descend before throwing an exception
* when parsing the XML into JSON. The default max nesting depth is 512, which means the parser
* will go as deep as the maximum call stack size allows. Using any negative value as a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is slightly incorrect - now we have a default limit of 512, so the parser won't go as deep as the maximum call stack size allows, instead it will stop at the depth limit and throw a JsonException.
You can rephrase it like this:

The default max nesting depth is 512, which means the parser will throw a JsonException 
if the maximum depth is reached.  
Using any negative value as a parameter is equivalent to setting no limit to the nesting depth, 
which means the parses will go as deep as the maximum call stack size allows.

* parameter is equivalent to setting no limit to the nesting depth.
* @param maxNestingDepth the maximum nesting depth allowed to the XML parser
* @return The existing configuration will not be modified. A new configuration is returned.
*/
public XMLParserConfiguration withMaxNestingDepth(int maxNestingDepth) {
XMLParserConfiguration newConfig = this.clone();

if (maxNestingDepth > UNDEFINED_MAXIMUM_NESTING_DEPTH) {
newConfig.maxNestingDepth = maxNestingDepth;
} else {
newConfig.maxNestingDepth = UNDEFINED_MAXIMUM_NESTING_DEPTH;
}

return newConfig;
}
}
1 change: 0 additions & 1 deletion src/test/java/org/json/junit/JSONArrayTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand Down
23 changes: 23 additions & 0 deletions src/test/java/org/json/junit/XMLConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,29 @@ public void testEmptyTagForceList() {

Util.compareActualVsExpectedJsonObjects(jsonObject, expetedJsonObject);
}

@Test
public void testMaxNestingDepthIsSet() {
XMLParserConfiguration xmlParserConfiguration = XMLParserConfiguration.ORIGINAL;

assertEquals(xmlParserConfiguration.getMaxNestingDepth(), XMLParserConfiguration.DEFAULT_MAXIMUM_NESTING_DEPTH);

xmlParserConfiguration = xmlParserConfiguration.withMaxNestingDepth(42);

assertEquals(xmlParserConfiguration.getMaxNestingDepth(), 42);

xmlParserConfiguration = xmlParserConfiguration.withMaxNestingDepth(0);

assertEquals(xmlParserConfiguration.getMaxNestingDepth(), 0);

xmlParserConfiguration = xmlParserConfiguration.withMaxNestingDepth(-31415926);

assertEquals(xmlParserConfiguration.getMaxNestingDepth(), XMLParserConfiguration.UNDEFINED_MAXIMUM_NESTING_DEPTH);

xmlParserConfiguration = xmlParserConfiguration.withMaxNestingDepth(Integer.MIN_VALUE);

assertEquals(xmlParserConfiguration.getMaxNestingDepth(), XMLParserConfiguration.UNDEFINED_MAXIMUM_NESTING_DEPTH);
}

/**
* Convenience method, given an input string and expected result,
Expand Down
59 changes: 59 additions & 0 deletions src/test/java/org/json/junit/XMLTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,65 @@ public void testIndentComplicatedJsonObjectWithArrayAndWithConfig(){
fail("file writer error: " +e.getMessage());
}
}

@Test
public void testMaxNestingDepthOf42IsRespected() {
final String wayTooLongMalformedXML = new String(new char[6000]).replace("\0", "<a>");

final int maxNestingDepth = 42;

try {
XML.toJSONObject(wayTooLongMalformedXML, XMLParserConfiguration.ORIGINAL.withMaxNestingDepth(maxNestingDepth));

fail("Expecting a JSONException");
} catch (JSONException e) {
assertTrue("Wrong throwable thrown: not expecting message <" + e.getMessage() + ">",
e.getMessage().startsWith("Maximum nesting depth of " + maxNestingDepth));
}
}

@Test
public void testMaxNestingDepthIsRespectedWithValidXML() {
final String perfectlyFineXML = "<Test>\n" +
cleydyr marked this conversation as resolved.
Show resolved Hide resolved
" <employee>\n" +
" <name>sonoo</name>\n" +
" <salary>56000</salary>\n" +
" <married>true</married>\n" +
" </employee>\n" +
"</Test>\n";

final int maxNestingDepth = 1;

try {
XML.toJSONObject(perfectlyFineXML, XMLParserConfiguration.ORIGINAL.withMaxNestingDepth(maxNestingDepth));

fail("Expecting a JSONException");
} catch (JSONException e) {
assertTrue("Wrong throwable thrown: not expecting message <" + e.getMessage() + ">",
e.getMessage().startsWith("Maximum nesting depth of " + maxNestingDepth));
}
}

@Test
public void testMaxNestingDepthWithValidFittingXML() {
final String perfectlyFineXML = "<Test>\n" +
" <employee>\n" +
" <name>sonoo</name>\n" +
" <salary>56000</salary>\n" +
" <married>true</married>\n" +
" </employee>\n" +
"</Test>\n";

final int maxNestingDepth = 3;

try {
XML.toJSONObject(perfectlyFineXML, XMLParserConfiguration.ORIGINAL.withMaxNestingDepth(maxNestingDepth));
} catch (JSONException e) {
e.printStackTrace();
fail("XML document should be parsed as its maximum depth fits the maxNestingDepth " +
"parameter of the XMLParserConfiguration used");
}
}
cleydyr marked this conversation as resolved.
Show resolved Hide resolved
}


Expand Down