Merge pull request #1072 from mechko/1071-xml-tostring-name-validation

#1071: reject invalid XML element names in XML.toString (CWE-91)

Author: stleary

src/main/java/org/json/XML.java

@@ -238,6 +238,28 @@ public static void noSpace(String string) throws JSONException {
         }
     }
 
+    /**
+     * Throw an exception if the string contains an XML metacharacter
+     * ({@code < > & " ' /}). 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).
+     *
+     * @param string the candidate element name
+     * @throws JSONException if {@code string} contains an XML metacharacter
+     */
+    static void noXmlMetachars(String string) throws JSONException {
+        int length = string.length();
+        for (int i = 0; i < length; i++) {
+            char c = string.charAt(i);
+            if (c == '<' || c == '>' || c == '&'
+                    || c == '"' || c == '\'' || c == '/') {
+                throw new JSONException("'" + string
+                        + "' contains an XML metacharacter and may not be used as an element name.");
+            }
+        }
+    }
+
     /**
      * Scan the content following the named tag, attaching it to the context.
      *
@@ -974,6 +996,10 @@ private static String toString(final Object object, final String tagName, final
         JSONObject jo;
         String string;
 
+        if (tagName != null) {
+            noXmlMetachars(tagName);
+        }
+
         if (object instanceof JSONObject) {
 
             // Emit <tagName>
@@ -992,6 +1018,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())) {
+                    noXmlMetachars(key);
+                }
                 Object value = jo.opt(key);
                 if (value == null) {
                     value = "";

src/test/java/org/json/junit/XMLTest.java

@@ -565,6 +565,42 @@ public void shouldHandleIllegalJSONNodeNames()
         assertTrue("Illegal@node",result.contains("<Illegal@node>someValue2</Illegal@node>"));
     }
 
+    /**
+     * 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/><injected>evil</injected><a\":\"\"}");
+        try {
+            XML.toString(jo, "root");
+            fail("expected JSONException for key containing XML metacharacters");
+        } catch (JSONException expected) {
+            // expected: '/', '<', '>' are rejected in element names
+        }
+
+        // caller-supplied tagName is checked too
+        try {
+            XML.toString(new JSONObject(), "bad<tag");
+            fail("expected JSONException for tagName containing '<'");
+        } catch (JSONException expected) {
+            // expected: '<' is rejected in element names
+        }
+
+        // each metacharacter is rejected individually
+        for (char c : new char[] {'<', '>', '&', '"', '\'', '/'}) {
+            try {
+                XML.toString(new JSONObject().put("a" + c + "b", "v"));
+                fail("expected JSONException for key containing '" + c + "'");
+            } catch (JSONException expected) {
+                // expected
+            }
+        }
+    }
+
     /**
      * JSONObject with NULL value, to XML.toString()
      */