Reject NUL bytes in the SimpleXMLElement constructor - #23069
Conversation
When dataIsURL is true, __construct parsed the path as a plain string and passed it to xmlReadFile, so an embedded NUL truncated the path. simplexml_load_file already rejects NULs via the path parameter type. Match that check before xmlReadFile. Closes phpGH-23069
49c4d81 to
3f8efc2
Compare
|
should target master (thus no more CHECK_NULL_PATH) |
There was a problem hiding this comment.
This should just use a P ZPP specifier.
There was a problem hiding this comment.
Done in b423d89, with lowercase p to keep the existing char*/size_t locals, matching simplexml_load_file() and asXML() in the same file.
It covers the non-URL form too, where the NUL truncates just as visibly: "<r/>\0evil" parsed as <r/> and dropped the rest, while the same string without the NUL is rejected. UPGRADING entry added for that.
3f8efc2 to
c6d000b
Compare
The first argument was parsed with the s specifier and handed to
xmlReadFile as a C string when dataIsURL is set, so
new SimpleXMLElement("/tmp/ok.xml\0evil", 0, true) quietly loaded
/tmp/ok.xml. simplexml_load_file() and SimpleXMLElement::asXML() already
declare their path argument with p; the constructor now does the same.
Closes phpGH-23069
The first argument was parsed with the s specifier, so a NUL byte
truncated it: with dataIsURL set xmlReadFile got a C string and
new SimpleXMLElement("/tmp/ok.xml\0evil", 0, true) quietly loaded
/tmp/ok.xml, and without it libxml stopped at the NUL and dropped the
rest of the document. simplexml_load_file() and
SimpleXMLElement::asXML() already declare their path argument with p;
the constructor now does the same.
Closes phpGH-23069
c6d000b to
b423d89
Compare
|
Retargeted and rebased onto master in b423d89. |
iliaal
changed the title
devnexen
left a comment
devnexen
left a comment
There was a problem hiding this comment.
UPGRADING is wrong about data mode. data_len goes to xmlReadMemory, so nothing truncated; on baseline "\0evil" and "evil" both throw Exception: String could not be parsed as XML. This changes Exception to ValueError, not silence to error.
And p can't key off $dataIsURL, so new SimpleXMLElement("\0evil") now throws while simplexml_load_string() still returns false.
With dataIsURL set the first argument goes to xmlReadFile as a C string,
so new SimpleXMLElement("/tmp/ok.xml\0evil", 0, true) truncates at the NUL
and quietly loads /tmp/ok.xml. simplexml_load_file() and
SimpleXMLElement::asXML() already declare their path argument as a path;
the constructor took a plain string, so the check never ran.
Closes phpGH-23069
b423d89 to
7b464e7
Compare
With dataIsURL set the first argument goes to xmlReadFile as a C string,
so new SimpleXMLElement("/tmp/ok.xml\0evil", 0, true) truncates at the NUL
and quietly loads /tmp/ok.xml. simplexml_load_file() and
SimpleXMLElement::asXML() already declare their path argument as a path;
the constructor took a plain string, so the check never ran.
Closes phpGH-23069
7b464e7 to
14a60a6
Compare
|
Ugh, the UPGRADING wording was wrong, rewritten in 14a60a6. We measured different things because it is libxml dependent: on 2.9.14 If that divergence is the blocker, the alternative is checking only when dataIsURL is set, the way ext/dom guards DOM_LOAD_FILE. |
With dataIsURL set the first argument goes to xmlReadFile as a C string,
so new SimpleXMLElement("/tmp/ok.xml\0evil", 0, true) truncates at the NUL
and quietly loads /tmp/ok.xml. simplexml_load_file() and
SimpleXMLElement::asXML() already declare their path argument as a path;
the constructor took a plain string, so the check never ran.
Closes phpGH-23069
14a60a6 to
fbe772c
Compare
3 participants
With dataIsURL set, SimpleXMLElement::__construct hands its first argument to xmlReadFile as a C string, so
new SimpleXMLElement("/tmp/ok.xml\0anything", 0, true)truncates at the NUL and quietly loads /tmp/ok.xml. simplexml_load_file() and SimpleXMLElement::asXML() already declare their path argument as a path; the constructor took a plain string, so the check never ran.Declaring it as a path applies to the non-URL form too. At default options current libxml already rejects an embedded NUL there, so that form gains a ValueError in place of a parse failure. With LIBXML_RECOVER, or on older libxml such as 2.9.14, the string was accepted and is now rejected.