Skip to content

Commit e2cfb5a

Browse files
mechkoclaude
mechko
and
claude
committed
stleary#1071: reject invalid XML element names in XML.toString (CWE-91)
XML.toString emitted JSONObject keys verbatim as tag names, so a key containing '<', '>' or '/' broke out of its element and injected arbitrary sibling structure into the output. Per stleary#294/stleary#123 the agreed approach is to throw on invalid input rather than mangle it. - Add mustBeXmlName / isXmlNameStart / isXmlNameChar implementing the XML 1.0 (5th ed.) Name production, code-point aware. - Validate tagName at method entry and each key at the top of the key loop (skipping the cDataTagName sentinel). - Rewrite XMLTest.shouldHandleIllegalJSONNodeNames and XMLConfigurationTest.shouldHandleIllegalJSONNodeNames (previously documenting the pass-through behaviour) to assert the throw. - Add XMLTest.toStringRejectsElementInjectionInKey covering the stleary#1071 payload and an invalid caller-supplied tagName. - Add XMLTest.toStringAcceptsValidXmlNames covering hyphen/dot/ underscore/colon, Latin-1 letters, and the cDataTagName sentinel. Fixes stleary#1071. Also resolves the long-standing well-formedness question in stleary#166 / stleary#294 / stleary#308. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 1795e8c commit e2cfb5a

3 files changed

Lines changed: 159 additions & 40 deletions

File tree

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,75 @@ public static void noSpace(String string) throws JSONException {
232232
}
233233
}
234234

235+
/**
236+
* Throw an exception if the string is not a valid XML 1.0 {@code Name}
237+
* (element name). Used by {@link #toString(Object)} to reject JSON keys that
238+
* would otherwise be emitted verbatim between {@code <} and {@code >} and
239+
* could break out of the tag context (element injection, CWE-91;
240+
* see issue #1071 and #294).
241+
*
242+
* @param string the candidate element name
243+
* @throws JSONException if {@code string} is null, empty, or contains a
244+
* character outside the XML 1.0 Name production
245+
*/
246+
static void mustBeXmlName(String string) throws JSONException {
247+
if (string == null || string.isEmpty()) {
248+
throw new JSONException("'" + string
249+
+ "' is not a valid XML element name.");
250+
}
251+
int cp = string.codePointAt(0);
252+
if (!isXmlNameStart(cp)) {
253+
throw new JSONException("'" + string
254+
+ "' is not a valid XML element name.");
255+
}
256+
for (int i = Character.charCount(cp); i < string.length(); i += Character.charCount(cp)) {
257+
cp = string.codePointAt(i);
258+
if (!isXmlNameChar(cp)) {
259+
throw new JSONException("'" + string
260+
+ "' is not a valid XML element name.");
261+
}
262+
}
263+
}
264+
265+
/**
266+
* XML 1.0 (5th ed.) {@code NameStartChar} production.
267+
*
268+
* @param cp a Unicode code point
269+
* @return true if {@code cp} may start an XML Name
270+
*/
271+
private static boolean isXmlNameStart(int cp) {
272+
return cp == ':' || cp == '_'
273+
|| (cp >= 'A' && cp <= 'Z')
274+
|| (cp >= 'a' && cp <= 'z')
275+
|| (cp >= 0xC0 && cp <= 0xD6)
276+
|| (cp >= 0xD8 && cp <= 0xF6)
277+
|| (cp >= 0xF8 && cp <= 0x2FF)
278+
|| (cp >= 0x370 && cp <= 0x37D)
279+
|| (cp >= 0x37F && cp <= 0x1FFF)
280+
|| (cp >= 0x200C && cp <= 0x200D)
281+
|| (cp >= 0x2070 && cp <= 0x218F)
282+
|| (cp >= 0x2C00 && cp <= 0x2FEF)
283+
|| (cp >= 0x3001 && cp <= 0xD7FF)
284+
|| (cp >= 0xF900 && cp <= 0xFDCF)
285+
|| (cp >= 0xFDF0 && cp <= 0xFFFD)
286+
|| (cp >= 0x10000 && cp <= 0xEFFFF);
287+
}
288+
289+
/**
290+
* XML 1.0 (5th ed.) {@code NameChar} production.
291+
*
292+
* @param cp a Unicode code point
293+
* @return true if {@code cp} may appear after the first character of an XML Name
294+
*/
295+
private static boolean isXmlNameChar(int cp) {
296+
return isXmlNameStart(cp)
297+
|| cp == '-' || cp == '.'
298+
|| (cp >= '0' && cp <= '9')
299+
|| cp == 0xB7
300+
|| (cp >= 0x0300 && cp <= 0x036F)
301+
|| (cp >= 0x203F && cp <= 0x2040);
302+
}
303+
235304
/**
236305
* Scan the content following the named tag, attaching it to the context.
237306
*
@@ -968,6 +1037,10 @@ private static String toString(final Object object, final String tagName, final
9681037
JSONObject jo;
9691038
String string;
9701039

1040+
if (tagName != null) {
1041+
mustBeXmlName(tagName);
1042+
}
1043+
9711044
if (object instanceof JSONObject) {
9721045

9731046
// Emit <tagName>
@@ -986,6 +1059,9 @@ private static String toString(final Object object, final String tagName, final
9861059
// don't use the new entrySet accessor to maintain Android Support
9871060
jo = (JSONObject) object;
9881061
for (final String key : jo.keySet()) {
1062+
if (!key.equals(config.getcDataTagName())) {
1063+
mustBeXmlName(key);
1064+
}
9891065
Object value = jo.opt(key);
9901066
if (value == null) {
9911067
value = "";

‎src/test/java/org/json/junit/XMLConfigurationTest.java‎

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -506,32 +506,31 @@ public void shouldHandleNestedArraytoString() {
506506

507507

508508
/**
509-
* Possible bug:
510-
* Illegal node-names must be converted to legal XML-node-names.
511-
* The given example shows 2 nodes which are valid for JSON, but not for XML.
512-
* Therefore illegal arguments should be converted to e.g. an underscore (_).
509+
* JSON keys that are not valid XML 1.0 Names are rejected by
510+
* XML.toString rather than being emitted as (malformed) tag names.
511+
* See issues #166, #294, #308 and #1071.
513512
*/
514513
@Test
515514
public void shouldHandleIllegalJSONNodeNames()
516515
{
517-
JSONObject inputJSON = new JSONObject();
518-
inputJSON.append("123IllegalNode", "someValue1");
519-
inputJSON.append("Illegal@node", "someValue2");
520-
521-
String result = XML.toString(inputJSON, null,
522-
XMLParserConfiguration.KEEP_STRINGS);
523-
524-
/*
525-
* This is invalid XML. Names should not begin with digits or contain
526-
* certain values, including '@'. One possible solution is to replace
527-
* illegal chars with '_', in which case the expected output would be:
528-
* <___IllegalNode>someValue1</___IllegalNode><Illegal_node>someValue2</Illegal_node>
529-
*/
530-
String expected = "<123IllegalNode>someValue1</123IllegalNode><Illegal@node>someValue2</Illegal@node>";
531-
532-
assertEquals("Length", expected.length(), result.length());
533-
assertTrue("123IllegalNode", result.contains("<123IllegalNode>someValue1</123IllegalNode>"));
534-
assertTrue("Illegal@node", result.contains("<Illegal@node>someValue2</Illegal@node>"));
516+
// Name may not start with a digit
517+
try {
518+
XML.toString(new JSONObject().append("123IllegalNode", "someValue1"),
519+
null, XMLParserConfiguration.KEEP_STRINGS);
520+
fail("expected JSONException for digit-leading element name");
521+
} catch (JSONException expected) {
522+
assertTrue("message names the offending key",
523+
expected.getMessage().contains("123IllegalNode"));
524+
}
525+
// Name may not contain '@'
526+
try {
527+
XML.toString(new JSONObject().append("Illegal@node", "someValue2"),
528+
null, XMLParserConfiguration.KEEP_STRINGS);
529+
fail("expected JSONException for '@' in element name");
530+
} catch (JSONException expected) {
531+
assertTrue("message names the offending key",
532+
expected.getMessage().contains("Illegal@node"));
533+
}
535534
}
536535

537536
/**

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

Lines changed: 62 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -538,31 +538,75 @@ public void shouldHandleNestedArraytoString() {
538538

539539

540540
/**
541-
* Possible bug:
542-
* Illegal node-names must be converted to legal XML-node-names.
543-
* The given example shows 2 nodes which are valid for JSON, but not for XML.
544-
* Therefore illegal arguments should be converted to e.g. an underscore (_).
541+
* JSON keys that are not valid XML 1.0 Names are rejected by
542+
* XML.toString rather than being emitted as (malformed) tag names.
543+
* See issues #166, #294, #308 and #1071.
545544
*/
546545
@Test
547546
public void shouldHandleIllegalJSONNodeNames()
548547
{
549-
JSONObject inputJSON = new JSONObject();
550-
inputJSON.append("123IllegalNode", "someValue1");
551-
inputJSON.append("Illegal@node", "someValue2");
548+
// Name may not start with a digit
549+
try {
550+
XML.toString(new JSONObject().append("123IllegalNode", "someValue1"));
551+
fail("expected JSONException for digit-leading element name");
552+
} catch (JSONException expected) {
553+
assertTrue("message names the offending key",
554+
expected.getMessage().contains("123IllegalNode"));
555+
}
556+
// Name may not contain '@'
557+
try {
558+
XML.toString(new JSONObject().append("Illegal@node", "someValue2"));
559+
fail("expected JSONException for '@' in element name");
560+
} catch (JSONException expected) {
561+
assertTrue("message names the offending key",
562+
expected.getMessage().contains("Illegal@node"));
563+
}
564+
}
552565

553-
String result = XML.toString(inputJSON);
566+
/**
567+
* A JSON key containing XML metacharacters must not be emitted as a raw
568+
* tag name, since doing so allows the key to break out of its element and
569+
* inject sibling structure into the output (CWE-91, issue #1071).
570+
*/
571+
@Test
572+
public void toStringRejectsElementInjectionInKey()
573+
{
574+
JSONObject jo = new JSONObject(
575+
"{\"a/><injected>evil</injected><a\":\"\"}");
576+
try {
577+
XML.toString(jo, "root");
578+
fail("expected JSONException for key containing XML metacharacters");
579+
} catch (JSONException expected) {
580+
// expected: '/', '<', '>' are outside the XML Name production
581+
}
554582

555-
/*
556-
* This is invalid XML. Names should not begin with digits or contain
557-
* certain values, including '@'. One possible solution is to replace
558-
* illegal chars with '_', in which case the expected output would be:
559-
* <___IllegalNode>someValue1</___IllegalNode><Illegal_node>someValue2</Illegal_node>
560-
*/
561-
String expected = "<123IllegalNode>someValue1</123IllegalNode><Illegal@node>someValue2</Illegal@node>";
583+
// caller-supplied tagName is validated too
584+
try {
585+
XML.toString(new JSONObject(), "bad tag");
586+
fail("expected JSONException for tagName containing whitespace");
587+
} catch (JSONException expected) {
588+
// expected: space is outside the XML Name production
589+
}
590+
}
562591

563-
assertEquals("length",expected.length(), result.length());
564-
assertTrue("123IllegalNode",result.contains("<123IllegalNode>someValue1</123IllegalNode>"));
565-
assertTrue("Illegal@node",result.contains("<Illegal@node>someValue2</Illegal@node>"));
592+
/**
593+
* Keys that are valid XML 1.0 Names continue to serialise unchanged.
594+
*/
595+
@Test
596+
public void toStringAcceptsValidXmlNames()
597+
{
598+
JSONObject jo = new JSONObject();
599+
jo.put("simple", "a");
600+
jo.put("with-hyphen.dot_underscore", "b");
601+
jo.put("ns:qualified", "c");
602+
jo.put("élément", "d"); // Latin-1 letters
603+
jo.put("content", "e"); // cDataTagName sentinel, emitted as text
604+
String xml = XML.toString(jo, "root");
605+
assertTrue(xml.contains("<simple>a</simple>"));
606+
assertTrue(xml.contains("<with-hyphen.dot_underscore>b</with-hyphen.dot_underscore>"));
607+
assertTrue(xml.contains("<ns:qualified>c</ns:qualified>"));
608+
assertTrue(xml.contains("<élément>d</élément>"));
609+
assertTrue("cDataTagName still emitted as text content", xml.contains(">e<"));
566610
}
567611

568612
/**

0 commit comments

Comments
 (0)