diff --git a/src/main/java/org/json/XML.java b/src/main/java/org/json/XML.java index 32475876c..5191cdf97 100644 --- a/src/main/java/org/json/XML.java +++ b/src/main/java/org/json/XML.java @@ -232,6 +232,81 @@ public static void noSpace(String string) throws JSONException { } } + /** + * Throw an exception if the string is not a valid XML 1.0 {@code Name} + * (element name). Used by {@link #toString(Object)} to reject JSON keys that + * would otherwise be emitted verbatim between {@code <} and {@code >} and + * could break out of the tag context (element injection, CWE-91; + * see issue #1071 and #294). + * + * @param string the candidate element name + * @throws JSONException if {@code string} is null, empty, or contains a + * character outside the XML 1.0 Name production + */ + static void mustBeXmlName(String string) throws JSONException { + if (string == null || string.isEmpty()) { + throw new JSONException("'" + string + + "' is not a valid XML element name."); + } + int cp = string.codePointAt(0); + if (!isXmlNameStart(cp)) { + throw new JSONException("'" + string + + "' is not a valid XML element name."); + } + for (int i = Character.charCount(cp); i < string.length(); i += Character.charCount(cp)) { + cp = string.codePointAt(i); + if (!isXmlNameChar(cp)) { + throw new JSONException("'" + string + + "' is not a valid XML element name."); + } + } + } + + private static boolean inRange(int cp, int lo, int hi) { + return cp >= lo && cp <= hi; + } + + /** + * XML 1.0 (5th ed.) {@code NameStartChar} production. + * + * @param cp a Unicode code point + * @return true if {@code cp} may start an XML Name + */ + private static boolean isXmlNameStart(int cp) { + return cp == ':' + || cp == '_' + || inRange(cp, 'A', 'Z') + || inRange(cp, 'a', 'z') + || inRange(cp, 0xC0, 0xD6) + || inRange(cp, 0xD8, 0xF6) + || inRange(cp, 0xF8, 0x2FF) + || inRange(cp, 0x370, 0x37D) + || inRange(cp, 0x37F, 0x1FFF) + || inRange(cp, 0x200C, 0x200D) + || inRange(cp, 0x2070, 0x218F) + || inRange(cp, 0x2C00, 0x2FEF) + || inRange(cp, 0x3001, 0xD7FF) + || inRange(cp, 0xF900, 0xFDCF) + || inRange(cp, 0xFDF0, 0xFFFD) + || inRange(cp, 0x10000, 0xEFFFF); + } + + /** + * XML 1.0 (5th ed.) {@code NameChar} production. + * + * @param cp a Unicode code point + * @return true if {@code cp} may appear after the first character of an XML Name + */ + private static boolean isXmlNameChar(int cp) { + return isXmlNameStart(cp) + || cp == '-' + || cp == '.' + || cp == 0xB7 + || inRange(cp, '0', '9') + || inRange(cp, 0x0300, 0x036F) + || inRange(cp, 0x203F, 0x2040); + } + /** * Scan the content following the named tag, attaching it to the context. * @@ -968,6 +1043,10 @@ private static String toString(final Object object, final String tagName, final JSONObject jo; String string; + if (tagName != null) { + mustBeXmlName(tagName); + } + if (object instanceof JSONObject) { // Emit @@ -986,6 +1065,9 @@ private static String toString(final Object object, final String tagName, final // don't use the new entrySet accessor to maintain Android Support jo = (JSONObject) object; for (final String key : jo.keySet()) { + if (!key.equals(config.getcDataTagName())) { + mustBeXmlName(key); + } Object value = jo.opt(key); if (value == null) { value = ""; diff --git a/src/test/java/org/json/junit/XMLConfigurationTest.java b/src/test/java/org/json/junit/XMLConfigurationTest.java index e8ff3b60c..1a1a2fd58 100755 --- a/src/test/java/org/json/junit/XMLConfigurationTest.java +++ b/src/test/java/org/json/junit/XMLConfigurationTest.java @@ -506,32 +506,31 @@ public void shouldHandleNestedArraytoString() { /** - * Possible bug: - * Illegal node-names must be converted to legal XML-node-names. - * The given example shows 2 nodes which are valid for JSON, but not for XML. - * Therefore illegal arguments should be converted to e.g. an underscore (_). + * JSON keys that are not valid XML 1.0 Names are rejected by + * XML.toString rather than being emitted as (malformed) tag names. + * See issues #166, #294, #308 and #1071. */ @Test public void shouldHandleIllegalJSONNodeNames() { - JSONObject inputJSON = new JSONObject(); - inputJSON.append("123IllegalNode", "someValue1"); - inputJSON.append("Illegal@node", "someValue2"); - - String result = XML.toString(inputJSON, null, - XMLParserConfiguration.KEEP_STRINGS); - - /* - * This is invalid XML. Names should not begin with digits or contain - * certain values, including '@'. One possible solution is to replace - * illegal chars with '_', in which case the expected output would be: - * <___IllegalNode>someValue1someValue2 - */ - String expected = "<123IllegalNode>someValue1someValue2"; - - assertEquals("Length", expected.length(), result.length()); - assertTrue("123IllegalNode", result.contains("<123IllegalNode>someValue1")); - assertTrue("Illegal@node", result.contains("someValue2")); + // Name may not start with a digit + try { + XML.toString(new JSONObject().append("123IllegalNode", "someValue1"), + null, XMLParserConfiguration.KEEP_STRINGS); + fail("expected JSONException for digit-leading element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("123IllegalNode")); + } + // Name may not contain '@' + try { + XML.toString(new JSONObject().append("Illegal@node", "someValue2"), + null, XMLParserConfiguration.KEEP_STRINGS); + fail("expected JSONException for '@' in element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("Illegal@node")); + } } /** diff --git a/src/test/java/org/json/junit/XMLTest.java b/src/test/java/org/json/junit/XMLTest.java index 589536fd2..e67b9de59 100644 --- a/src/test/java/org/json/junit/XMLTest.java +++ b/src/test/java/org/json/junit/XMLTest.java @@ -538,31 +538,75 @@ public void shouldHandleNestedArraytoString() { /** - * Possible bug: - * Illegal node-names must be converted to legal XML-node-names. - * The given example shows 2 nodes which are valid for JSON, but not for XML. - * Therefore illegal arguments should be converted to e.g. an underscore (_). + * JSON keys that are not valid XML 1.0 Names are rejected by + * XML.toString rather than being emitted as (malformed) tag names. + * See issues #166, #294, #308 and #1071. */ @Test public void shouldHandleIllegalJSONNodeNames() { - JSONObject inputJSON = new JSONObject(); - inputJSON.append("123IllegalNode", "someValue1"); - inputJSON.append("Illegal@node", "someValue2"); + // Name may not start with a digit + try { + XML.toString(new JSONObject().append("123IllegalNode", "someValue1")); + fail("expected JSONException for digit-leading element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("123IllegalNode")); + } + // Name may not contain '@' + try { + XML.toString(new JSONObject().append("Illegal@node", "someValue2")); + fail("expected JSONException for '@' in element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("Illegal@node")); + } + } - String result = XML.toString(inputJSON); + /** + * A JSON key containing XML metacharacters must not be emitted as a raw + * tag name, since doing so allows the key to break out of its element and + * inject sibling structure into the output (CWE-91, issue #1071). + */ + @Test + public void toStringRejectsElementInjectionInKey() + { + JSONObject jo = new JSONObject( + "{\"a/>evil' are outside the XML Name production + } - /* - * This is invalid XML. Names should not begin with digits or contain - * certain values, including '@'. One possible solution is to replace - * illegal chars with '_', in which case the expected output would be: - * <___IllegalNode>someValue1someValue2 - */ - String expected = "<123IllegalNode>someValue1someValue2"; + // caller-supplied tagName is validated too + try { + XML.toString(new JSONObject(), "bad tag"); + fail("expected JSONException for tagName containing whitespace"); + } catch (JSONException expected) { + // expected: space is outside the XML Name production + } + } - assertEquals("length",expected.length(), result.length()); - assertTrue("123IllegalNode",result.contains("<123IllegalNode>someValue1")); - assertTrue("Illegal@node",result.contains("someValue2")); + /** + * Keys that are valid XML 1.0 Names continue to serialise unchanged. + */ + @Test + public void toStringAcceptsValidXmlNames() + { + JSONObject jo = new JSONObject(); + jo.put("simple", "a"); + jo.put("with-hyphen.dot_underscore", "b"); + jo.put("ns:qualified", "c"); + jo.put("élément", "d"); // Latin-1 letters + jo.put("content", "e"); // cDataTagName sentinel, emitted as text + String xml = XML.toString(jo, "root"); + assertTrue(xml.contains("a")); + assertTrue(xml.contains("b")); + assertTrue(xml.contains("c")); + assertTrue(xml.contains("<élément>d")); + assertTrue("cDataTagName still emitted as text content", xml.contains(">e<")); } /**