Skip to content

Fixed bug #74371 strip_tags altering attributes #3570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fixed bug #74371 strip_tags altering attributes #3570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a65e254
Fixed bug #74371 strip_tags altering attributes
htesligte Oct 2, 2018
5226052
removed unnecessary break, added some more tests
htesligte Oct 3, 2018
7b85f23
Changed strip_tags function to escape < and >
htesligte Oct 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ext/filter/sanitizing_filters.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ void php_filter_string(PHP_INPUT_FILTER_PARAM_DECL)
{
size_t new_len;
unsigned char enc[256] = {0};
zend_string* newval;

if (!Z_REFCOUNTED_P(value)) {
ZVAL_STRINGL(value, Z_STRVAL_P(value), Z_STRLEN_P(value));
Expand All @@ -206,10 +207,11 @@ void php_filter_string(PHP_INPUT_FILTER_PARAM_DECL)
php_filter_encode_html(value, enc);

/* strip tags, implicitly also removes \0 chars */
new_len = php_strip_tags_ex(Z_STRVAL_P(value), Z_STRLEN_P(value), NULL, NULL, 0, 1);
Z_STRLEN_P(value) = new_len;
newval = php_strip_tags_ex(Z_STRVAL_P(value), Z_STRLEN_P(value), NULL, NULL, 0, 1);
zval_ptr_dtor(value);
ZVAL_STR(value, newval);

if (new_len == 0) {
if (ZSTR_LEN(newval) == 0) {
zval_ptr_dtor(value);
if (flags & FILTER_FLAG_EMPTY_STRING_NULL) {
ZVAL_NULL(value);
Expand Down
13 changes: 6 additions & 7 deletions ext/standard/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,8 @@ PHPAPI PHP_FUNCTION(fgetss)
zend_long bytes = 0;
size_t len = 0;
size_t actual_len, retval_len;
char *buf = NULL, *retval;
char *buf = NULL, *str;
zend_string* retval;
php_stream *stream;
char *allowed_tags=NULL;
size_t allowed_tags_len=0;
Expand All @@ -1125,18 +1126,16 @@ PHPAPI PHP_FUNCTION(fgetss)
memset(buf, 0, len + 1);
}

if ((retval = php_stream_get_line(stream, buf, len, &actual_len)) == NULL) {
if ((str = php_stream_get_line(stream, buf, len, &actual_len)) == NULL) {
if (buf != NULL) {
efree(buf);
}
RETURN_FALSE;
}

retval_len = php_strip_tags(retval, actual_len, &stream->fgetss_state, allowed_tags, allowed_tags_len);

// TODO: avoid reallocation ???
RETVAL_STRINGL(retval, retval_len);
efree(retval);
retval = php_strip_tags(str, actual_len, &stream->fgetss_state, allowed_tags, allowed_tags_len);
efree(str);
RETURN_NEW_STR(retval);
}
/* }}} */

Expand Down
4 changes: 2 additions & 2 deletions ext/standard/php_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ PHPAPI char *php_stristr(char *s, char *t, size_t s_len, size_t t_len);
PHPAPI zend_string *php_str_to_str(const char *haystack, size_t length, const char *needle,
size_t needle_len, const char *str, size_t str_len);
PHPAPI zend_string *php_trim(zend_string *str, char *what, size_t what_len, int mode);
PHPAPI size_t php_strip_tags(char *rbuf, size_t len, uint8_t *state, const char *allow, size_t allow_len);
PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const char *allow, size_t allow_len, zend_bool allow_tag_spaces);
PHPAPI zend_string *php_strip_tags(char *rbuf, size_t len, uint8_t *state, const char *allow, size_t allow_len);
PHPAPI zend_string *php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const char *allow, size_t allow_len, zend_bool allow_tag_spaces);
PHPAPI void php_implode(const zend_string *delim, zval *arr, zval *return_value);
PHPAPI void php_explode(const zend_string *delim, zend_string *str, zval *return_value, zend_long limit);

Expand Down
53 changes: 47 additions & 6 deletions ext/standard/string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4778,6 +4778,7 @@ PHP_FUNCTION(nl2br)
PHP_FUNCTION(strip_tags)
{
zend_string *buf;
zend_string *newbuf;
zend_string *str;
zval *allow=NULL;
const char *allowed_tags=NULL;
Expand All @@ -4797,8 +4798,10 @@ PHP_FUNCTION(strip_tags)
}

buf = zend_string_init(ZSTR_VAL(str), ZSTR_LEN(str), 0);
ZSTR_LEN(buf) = php_strip_tags_ex(ZSTR_VAL(buf), ZSTR_LEN(str), NULL, allowed_tags, allowed_tags_len, 0);
RETURN_NEW_STR(buf);
newbuf = php_strip_tags_ex(ZSTR_VAL(buf), ZSTR_LEN(str), NULL, allowed_tags, allowed_tags_len, 0);
zend_string_release(buf);

RETURN_NEW_STR(newbuf);
}
/* }}} */

Expand Down Expand Up @@ -5002,7 +5005,7 @@ int php_tag_find(char *tag, size_t len, const char *set) {
}
/* }}} */

PHPAPI size_t php_strip_tags(char *rbuf, size_t len, uint8_t *stateptr, const char *allow, size_t allow_len) /* {{{ */
PHPAPI zend_string *php_strip_tags(char *rbuf, size_t len, uint8_t *stateptr, const char *allow, size_t allow_len) /* {{{ */
{
return php_strip_tags_ex(rbuf, len, stateptr, allow, allow_len, 0);
}
Expand All @@ -5028,9 +5031,9 @@ PHPAPI size_t php_strip_tags(char *rbuf, size_t len, uint8_t *stateptr, const ch
swm: Added ability to strip <?xml tags without assuming it PHP
code.
*/
PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const char *allow, size_t allow_len, zend_bool allow_tag_spaces)
PHPAPI zend_string *php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const char *allow, size_t allow_len, zend_bool allow_tag_spaces)
{
char *tbuf, *tp, *rp, c, lc;
char *tbuf, *tp, *rp, c, lc, *reallocp;
const char *buf, *p, *end;
int br, depth=0, in_q = 0;
uint8_t state = 0;
Expand Down Expand Up @@ -5122,6 +5125,23 @@ PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const
case '\0':
break;
case '<':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be crazy to encode < and > rather than merely allow them? XHTML and HTML4/5 agree that < > are acceptable forms of <> in attributes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that would be the responsibility of strip_tags. So I think if the documentation promises that attributes won't be altered, the function should just allow them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As strip_tags is commonly misused as a security mechanism, I think it is best to err on the side of caution here and encode < and >. This will also limit the collateral damage if the attribute handling is in some way incorrect.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, before I go crazy on this, the return value of php_strip_tags_ex is the new length, and the string is not expected to grow. So I'm a bit uncertain here, change the return value to a new string, which would mean touching all locations where strip_tags is used (in sanitizing_filters.c, file.c and filters.c). Or do you see an alternative way to approach this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm not sure if the direction I went is the smartest solution, but I just figured let's go a certain route and see what you think of this. My alternative solution was to modify the rbuf parameter to char**.

if (in_q && allow) {
// resize return buffer to store escaped > (&gt;)
pos = rp - rbuf;
rbuf = erealloc(rbuf, len += 3); // there was room for 1 <, add 3 more chars
rp = rbuf + pos;

// resize temporary buffer if necessary
if ((tp - tbuf) + 4 >= PHP_TAG_BUF_SIZE) {
pos = tp - tbuf;
tbuf = erealloc(tbuf, pos + PHP_TAG_BUF_SIZE + 1); // add room for &lt;
tp = tbuf + pos;
}

// store escaped <
strcpy(tp, "&lt;");
tp += 4;
}
if (in_q) {
break;
}
Expand All @@ -5135,6 +5155,23 @@ PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const
depth--;
break;
}
if (in_q && allow) {
// resize return buffer to store escaped > (&gt;)
pos = rp - rbuf;
rbuf = erealloc(rbuf, len += 3); // there was room for 1 <, add 3 more chars
rp = rbuf + pos;

// resize temporary buffer
if ((tp - tbuf) + 4 >= PHP_TAG_BUF_SIZE) {
pos = tp - tbuf;
tbuf = erealloc(tbuf, pos + PHP_TAG_BUF_SIZE + 1); // add room for &gt;
tp = tbuf + pos;
}

// store escaped >
strcpy(tp, "&gt;");
tp += 4;
}
if (in_q) {
break;
}
Expand Down Expand Up @@ -5196,6 +5233,7 @@ PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const
if (allow) {
if (tp - tbuf >= PHP_TAG_BUF_SIZE) {
pos = tp - tbuf;

tbuf = erealloc(tbuf, (tp - tbuf) + PHP_TAG_BUF_SIZE + 1);
tp = tbuf + pos;
}
Expand Down Expand Up @@ -5355,7 +5393,10 @@ PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const
if (stateptr)
*stateptr = state;

return (size_t)(rp - rbuf);
// copy string to new buffer
size_t new_len = (size_t)(rp-rbuf);

return zend_string_init(rbuf, (size_t)(rp - rbuf), 0);
}
/* }}} */

Expand Down
16 changes: 16 additions & 0 deletions ext/standard/tests/strings/bug74371.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
--TEST--
Bug #74371: strip_tags altering attributes
--FILE--
<?php

echo strip_tags('<img src="example.jpg" alt=":> :<">', '<img>') . PHP_EOL;
echo strip_tags('<img src="example.jpg" alt="\':> :<">', '<img>') . PHP_EOL;
echo strip_tags('<img src="example.jpg" alt=:><script>alert(0)</script>', '<img>') . PHP_EOL;
echo strip_tags('<img alt=< />', '<img>') . PHP_EOL;

?>
--EXPECT--
<img src="example.jpg" alt=":&gt; :&lt;">
<img src="example.jpg" alt="':&gt; :&lt;">
<img src="example.jpg" alt=:>alert(0)
<img alt=< />