You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
228 lines
6.1 KiB
228 lines
6.1 KiB
commit 5c47ae80738d0985babf06a023b3845169682064 |
|
Author: Tomas Korbar <tkorbar@redhat.com> |
|
Date: Mon Mar 14 10:22:37 2022 +0100 |
|
|
|
Protect against malicious namespace declarations |
|
|
|
diff --git a/lib/xmlparse.c b/lib/xmlparse.c |
|
index 5c3f573..901abbf 100644 |
|
--- a/lib/xmlparse.c |
|
+++ b/lib/xmlparse.c |
|
@@ -638,8 +638,7 @@ XML_ParserCreate(const XML_Char *encodingName) { |
|
|
|
XML_Parser XMLCALL |
|
XML_ParserCreateNS(const XML_Char *encodingName, XML_Char nsSep) { |
|
- XML_Char tmp[2]; |
|
- *tmp = nsSep; |
|
+ XML_Char tmp[2] = {nsSep, 0}; |
|
return XML_ParserCreate_MM(encodingName, NULL, tmp); |
|
} |
|
|
|
@@ -1253,8 +1252,7 @@ XML_ExternalEntityParserCreate(XML_Parser oldParser, const XML_Char *context, |
|
would be otherwise. |
|
*/ |
|
if (parser->m_ns) { |
|
- XML_Char tmp[2]; |
|
- *tmp = parser->m_namespaceSeparator; |
|
+ XML_Char tmp[2] = {parser->m_namespaceSeparator, 0}; |
|
parser = parserCreate(encodingName, &parser->m_mem, tmp, newDtd); |
|
} else { |
|
parser = parserCreate(encodingName, &parser->m_mem, NULL, newDtd); |
|
@@ -3526,6 +3524,117 @@ storeAtts(XML_Parser parser, const ENCODING *enc, const char *attStr, |
|
return XML_ERROR_NONE; |
|
} |
|
|
|
+static XML_Bool |
|
+is_rfc3986_uri_char(XML_Char candidate) { |
|
+ // For the RFC 3986 ANBF grammar see |
|
+ // https://datatracker.ietf.org/doc/html/rfc3986#appendix-A |
|
+ |
|
+ switch (candidate) { |
|
+ // From rule "ALPHA" (uppercase half) |
|
+ case 'A': |
|
+ case 'B': |
|
+ case 'C': |
|
+ case 'D': |
|
+ case 'E': |
|
+ case 'F': |
|
+ case 'G': |
|
+ case 'H': |
|
+ case 'I': |
|
+ case 'J': |
|
+ case 'K': |
|
+ case 'L': |
|
+ case 'M': |
|
+ case 'N': |
|
+ case 'O': |
|
+ case 'P': |
|
+ case 'Q': |
|
+ case 'R': |
|
+ case 'S': |
|
+ case 'T': |
|
+ case 'U': |
|
+ case 'V': |
|
+ case 'W': |
|
+ case 'X': |
|
+ case 'Y': |
|
+ case 'Z': |
|
+ |
|
+ // From rule "ALPHA" (lowercase half) |
|
+ case 'a': |
|
+ case 'b': |
|
+ case 'c': |
|
+ case 'd': |
|
+ case 'e': |
|
+ case 'f': |
|
+ case 'g': |
|
+ case 'h': |
|
+ case 'i': |
|
+ case 'j': |
|
+ case 'k': |
|
+ case 'l': |
|
+ case 'm': |
|
+ case 'n': |
|
+ case 'o': |
|
+ case 'p': |
|
+ case 'q': |
|
+ case 'r': |
|
+ case 's': |
|
+ case 't': |
|
+ case 'u': |
|
+ case 'v': |
|
+ case 'w': |
|
+ case 'x': |
|
+ case 'y': |
|
+ case 'z': |
|
+ |
|
+ // From rule "DIGIT" |
|
+ case '0': |
|
+ case '1': |
|
+ case '2': |
|
+ case '3': |
|
+ case '4': |
|
+ case '5': |
|
+ case '6': |
|
+ case '7': |
|
+ case '8': |
|
+ case '9': |
|
+ |
|
+ // From rule "pct-encoded" |
|
+ case '%': |
|
+ |
|
+ // From rule "unreserved" |
|
+ case '-': |
|
+ case '.': |
|
+ case '_': |
|
+ case '~': |
|
+ |
|
+ // From rule "gen-delims" |
|
+ case ':': |
|
+ case '/': |
|
+ case '?': |
|
+ case '#': |
|
+ case '[': |
|
+ case ']': |
|
+ case '@': |
|
+ |
|
+ // From rule "sub-delims" |
|
+ case '!': |
|
+ case '$': |
|
+ case '&': |
|
+ case '\'': |
|
+ case '(': |
|
+ case ')': |
|
+ case '*': |
|
+ case '+': |
|
+ case ',': |
|
+ case ';': |
|
+ case '=': |
|
+ return XML_TRUE; |
|
+ |
|
+ default: |
|
+ return XML_FALSE; |
|
+ } |
|
+} |
|
+ |
|
/* addBinding() overwrites the value of prefix->binding without checking. |
|
Therefore one must keep track of the old value outside of addBinding(). |
|
*/ |
|
@@ -3581,6 +3690,29 @@ addBinding(XML_Parser parser, PREFIX *prefix, const ATTRIBUTE_ID *attId, |
|
if (! mustBeXML && isXMLNS |
|
&& (len > xmlnsLen || uri[len] != xmlnsNamespace[len])) |
|
isXMLNS = XML_FALSE; |
|
+ |
|
+ // NOTE: While Expat does not validate namespace URIs against RFC 3986 |
|
+ // today (and is not REQUIRED to do so with regard to the XML 1.0 |
|
+ // namespaces specification) we have to at least make sure, that |
|
+ // the application on top of Expat (that is likely splitting expanded |
|
+ // element names ("qualified names") of form |
|
+ // "[uri sep] local [sep prefix] '\0'" back into 1, 2 or 3 pieces |
|
+ // in its element handler code) cannot be confused by an attacker |
|
+ // putting additional namespace separator characters into namespace |
|
+ // declarations. That would be ambiguous and not to be expected. |
|
+ // |
|
+ // While the HTML API docs of function XML_ParserCreateNS have been |
|
+ // advising against use of a namespace separator character that can |
|
+ // appear in a URI for >20 years now, some widespread applications |
|
+ // are using URI characters (':' (colon) in particular) for a |
|
+ // namespace separator, in practice. To keep these applications |
|
+ // functional, we only reject namespaces URIs containing the |
|
+ // application-chosen namespace separator if the chosen separator |
|
+ // is a non-URI character with regard to RFC 3986. |
|
+ if (parser->m_ns && (uri[len] == parser->m_namespaceSeparator) |
|
+ && ! is_rfc3986_uri_char(uri[len])) { |
|
+ return XML_ERROR_SYNTAX; |
|
+ } |
|
} |
|
isXML = isXML && len == xmlLen; |
|
isXMLNS = isXMLNS && len == xmlnsLen; |
|
diff --git a/tests/runtests.c b/tests/runtests.c |
|
index f03e008..40172d2 100644 |
|
--- a/tests/runtests.c |
|
+++ b/tests/runtests.c |
|
@@ -7233,6 +7233,37 @@ START_TEST(test_ns_double_colon_doctype) { |
|
} |
|
END_TEST |
|
|
|
+START_TEST(test_ns_separator_in_uri) { |
|
+ struct test_case { |
|
+ enum XML_Status expectedStatus; |
|
+ const char *doc; |
|
+ XML_Char namesep; |
|
+ }; |
|
+ struct test_case cases[] = { |
|
+ {XML_STATUS_OK, "<doc xmlns='one_two' />", XCS('\n')}, |
|
+ {XML_STATUS_ERROR, "<doc xmlns='one
two' />", XCS('\n')}, |
|
+ {XML_STATUS_OK, "<doc xmlns='one:two' />", XCS(':')}, |
|
+ }; |
|
+ |
|
+ size_t i = 0; |
|
+ size_t failCount = 0; |
|
+ for (; i < sizeof(cases) / sizeof(cases[0]); i++) { |
|
+ XML_Parser parser = XML_ParserCreateNS(NULL, cases[i].namesep); |
|
+ XML_SetElementHandler(parser, dummy_start_element, dummy_end_element); |
|
+ if (XML_Parse(parser, cases[i].doc, (int)strlen(cases[i].doc), |
|
+ /*isFinal*/ XML_TRUE) |
|
+ != cases[i].expectedStatus) { |
|
+ failCount++; |
|
+ } |
|
+ XML_ParserFree(parser); |
|
+ } |
|
+ |
|
+ if (failCount) { |
|
+ fail("Namespace separator handling is broken"); |
|
+ } |
|
+} |
|
+END_TEST |
|
+ |
|
/* Control variable; the number of times duff_allocator() will successfully |
|
* allocate */ |
|
#define ALLOC_ALWAYS_SUCCEED (-1) |
|
@@ -11527,6 +11558,7 @@ make_suite(void) { |
|
tcase_add_test(tc_namespace, test_ns_utf16_doctype); |
|
tcase_add_test(tc_namespace, test_ns_invalid_doctype); |
|
tcase_add_test(tc_namespace, test_ns_double_colon_doctype); |
|
+ tcase_add_test(tc_namespace, test_ns_separator_in_uri); |
|
|
|
suite_add_tcase(s, tc_misc); |
|
tcase_add_checked_fixture(tc_misc, NULL, basic_teardown);
|
|
|