Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
nielsdos committed Feb 5, 2025
1 parent de86b70 commit 779f6f7
Show file tree
Hide file tree
Showing 21 changed files with 310 additions and 70 deletions.
41 changes: 34 additions & 7 deletions ext/libxml/image_svg.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,34 @@ static int php_libxml_svg_stream_read(void *context, char *buffer, int len)

/* Sanity check that the input only contains characters valid for a dimension (numbers with units, e.g. 5cm).
* This also protects the user against injecting XSS.
* Only accept [0-9a-zA-Z] */
static bool php_libxml_valid_dimension(const xmlChar *input)
* Only accept [0-9]+[a-zA-Z]* */
static bool php_libxml_parse_dimension(const xmlChar *input, const xmlChar **unit_position)
{
if (*input == '\0') {
if (!(*input >= '0' && *input <= '9')) {
return false;
}

input++;

while (*input) {
if (!(*input >= '0' && *input <= '9')) {
if ((*input >= 'a' && *input <= 'z') || (*input >= 'A' && *input <= 'Z')) {
break;
}
return false;
}
input++;
}

*unit_position = input;

while (*input) {
if (!((*input >= '0' && *input <= '9') || (*input >= 'a' && *input <= 'z') || (*input >= 'A' && *input <= 'Z'))) {
if (!((*input >= 'a' && *input <= 'z') || (*input >= 'A' && *input <= 'Z'))) {
return false;
}
input++;
}

return true;
}

Expand Down Expand Up @@ -93,7 +109,10 @@ zend_result php_libxml_svg_image_handle(php_stream *stream, struct php_gfxinfo *

xmlChar *width = xmlTextReaderGetAttribute(reader, BAD_CAST "width");
xmlChar *height = xmlTextReaderGetAttribute(reader, BAD_CAST "height");
if (!width || !height || !php_libxml_valid_dimension(width) || !php_libxml_valid_dimension(height)) {
const xmlChar *width_unit_position, *height_unit_position;
if (!width || !height
|| !php_libxml_parse_dimension(width, &width_unit_position)
|| !php_libxml_parse_dimension(height, &height_unit_position)) {
xmlFree(width);
xmlFree(height);
break;
Expand All @@ -102,8 +121,16 @@ zend_result php_libxml_svg_image_handle(php_stream *stream, struct php_gfxinfo *
is_svg = true;
if (result) {
*result = ecalloc(1, sizeof(**result));
(*result)->width_str = zend_string_init((const char *) width, xmlStrlen(width), false);
(*result)->height_str = zend_string_init((const char *) height, xmlStrlen(height), false);
(*result)->width = ZEND_STRTOL((const char *) width, NULL, 10);
(*result)->height = ZEND_STRTOL((const char *) height, NULL, 10);
if (*width_unit_position) {
(*result)->width_unit = zend_string_init((const char*) width_unit_position,
xmlStrlen(width_unit_position), false);
}
if (*height_unit_position) {
(*result)->height_unit = zend_string_init((const char*) height_unit_position,
xmlStrlen(height_unit_position), false);
}
}

xmlFree(width);
Expand Down
56 changes: 48 additions & 8 deletions ext/libxml/tests/image/getimagesize.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ $inputs = [
<x:svg width='4cm' height="8cm" xmlns:x="http://www.w3.org/2000/svg">
XML,
"<SVG width='1' height=\"1\"/>",
"<SVG width='1px' height=\"1\"/>",
"<SVG width='1' height=\"1mm\"/>",
"<SVG width='1' height=\"cm\"/>",
"<SVG width='' height=\"\"/>",
"<SVG width=\"é\" height='1'/>",
"<foo width='1' height=\"1\"/>",
Expand All @@ -28,31 +31,68 @@ foreach ($inputs as $input) {
--EXPECTF--
bool(false)
bool(false)
array(5) {
array(6) {
[0]=>
string(3) "4cm"
int(4)
[1]=>
string(3) "8cm"
int(8)
[2]=>
int(%d)
[3]=>
string(24) "width="4cm" height="8cm""
["mime"]=>
string(13) "image/svg+xml"
["width_unit"]=>
string(2) "cm"
["height_unit"]=>
string(2) "cm"
}
array(5) {
array(7) {
[0]=>
string(1) "1"
int(1)
[1]=>
string(1) "1"
int(1)
[2]=>
int(%d)
[3]=>
string(20) "width="1" height="1""
["mime"]=>
string(13) "image/svg+xml"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}
array(7) {
[0]=>
int(1)
[1]=>
int(1)
[2]=>
int(20)
[3]=>
string(20) "width="1" height="1""
["mime"]=>
string(13) "image/svg+xml"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}
array(6) {
[0]=>
int(1)
[1]=>
int(1)
[2]=>
int(20)
["mime"]=>
string(13) "image/svg+xml"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "mm"
}
bool(false)
bool(false)
bool(false)
bool(false)
bool(false)
26 changes: 16 additions & 10 deletions ext/standard/image.c
Original file line number Diff line number Diff line change
Expand Up @@ -1543,17 +1543,11 @@ static void php_getimagesize_from_stream(php_stream *stream, char *input, zval *

if (result) {
array_init(return_value);
if (result->width_str) {
add_index_str(return_value, 0, result->width_str);
add_index_str(return_value, 1, result->height_str);
} else {
add_index_long(return_value, 0, result->width);
add_index_long(return_value, 1, result->height);
}
add_index_long(return_value, 0, result->width);
add_index_long(return_value, 1, result->height);
add_index_long(return_value, 2, itype);
if (result->width_str) {
add_index_str(return_value, 3, zend_strpprintf_unchecked(0, "width=\"%S\" height=\"%S\"", result->width_str, result->height_str));
} else {
if ((!result->width_unit || zend_string_equals_literal(result->width_unit, "px"))
&& (!result->height_unit || zend_string_equals_literal(result->height_unit, "px"))) {
char temp[MAX_LENGTH_OF_LONG * 2 + sizeof("width=\"\" height=\"\"")];
snprintf(temp, sizeof(temp), "width=\"%d\" height=\"%d\"", result->width, result->height);
add_index_string(return_value, 3, temp);
Expand All @@ -1566,6 +1560,18 @@ static void php_getimagesize_from_stream(php_stream *stream, char *input, zval *
add_assoc_long(return_value, "channels", result->channels);
}
add_assoc_string(return_value, "mime", mime_type ? mime_type : php_image_type_to_mime_type(itype));

if (result->width_unit) {
add_assoc_str(return_value, "width_unit", result->width_unit);
} else {
add_assoc_string(return_value, "width_unit", "px");
}
if (result->height_unit) {
add_assoc_str(return_value, "height_unit", result->height_unit);
} else {
add_assoc_string(return_value, "height_unit", "px");
}

efree(result);
} else {
RETURN_FALSE;
Expand Down
4 changes: 2 additions & 2 deletions ext/standard/php_image.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ PHPAPI bool php_is_image_avif(php_stream *stream);
struct php_gfxinfo {
unsigned int width;
unsigned int height;
zend_string *width_str;
zend_string *height_str;
zend_string *width_unit;
zend_string *height_unit;
unsigned int bits;
unsigned int channels;
};
Expand Down
6 changes: 5 additions & 1 deletion ext/standard/tests/image/bug13213.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ var_dump(GetImageSize(__DIR__.'/bug13213.jpg'));
?>
--EXPECTF--
Warning: getimagesize(): Corrupt JPEG data: 2 extraneous bytes before marker in %s%ebug13213.php on line %d
array(7) {
array(9) {
[0]=>
int(1)
[1]=>
Expand All @@ -21,4 +21,8 @@ array(7) {
int(3)
["mime"]=>
string(10) "image/jpeg"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}
6 changes: 5 additions & 1 deletion ext/standard/tests/image/bug70052.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ var_dump(getimagesize(__DIR__ . '/bug70052_2.wbmp'));
?>
--EXPECT--
bool(false)
array(5) {
array(7) {
[0]=>
int(3)
[1]=>
Expand All @@ -18,4 +18,8 @@ array(5) {
string(20) "width="3" height="3""
["mime"]=>
string(18) "image/vnd.wap.wbmp"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}
6 changes: 5 additions & 1 deletion ext/standard/tests/image/bug71848.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ var_dump(getimagesize(__DIR__ . '/bug71848.jpg', $info));
var_dump(array_keys($info));
?>
--EXPECT--
array(7) {
array(9) {
[0]=>
int(8)
[1]=>
Expand All @@ -21,6 +21,10 @@ array(7) {
int(3)
["mime"]=>
string(10) "image/jpeg"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}
array(2) {
[0]=>
Expand Down
6 changes: 5 additions & 1 deletion ext/standard/tests/image/bug72278.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ var_dump(getimagesize(FILENAME));
?>
--EXPECTF--
Warning: getimagesize(): Corrupt JPEG data: 3 extraneous bytes before marker in %s%ebug72278.php on line %d
array(7) {
array(9) {
[0]=>
int(300)
[1]=>
Expand All @@ -23,4 +23,8 @@ array(7) {
int(3)
["mime"]=>
string(10) "image/jpeg"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}
7 changes: 5 additions & 2 deletions ext/standard/tests/image/bug75708.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ var_dump(getimagesize('fs://bug75708.jpg', $info));

?>
--EXPECT--
array(7) {
array(9) {
[0]=>
int(10)
[1]=>
Expand All @@ -56,5 +56,8 @@ array(7) {
int(3)
["mime"]=>
string(10) "image/jpeg"
["width_unit"]=>
string(2) "px"
["height_unit"]=>
string(2) "px"
}

Loading

0 comments on commit 779f6f7

Please sign in to comment.