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 1 commit
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
2 changes: 2 additions & 0 deletions ext/standard/string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5123,6 +5123,7 @@ PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const
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) {
goto reg_char_1;
break;
}
if (isspace(*(p + 1)) && !allow_tag_spaces) {
Expand All @@ -5136,6 +5137,7 @@ PHPAPI size_t php_strip_tags_ex(char *rbuf, size_t len, uint8_t *stateptr, const
break;
}
if (in_q) {
goto reg_char_1;
break;
}

Expand Down
10 changes: 10 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,10 @@
--TEST--
Bug #74371: strip_tags altering attributes
--FILE--
<?php

echo strip_tags('<img src="example.jpg" alt=":> :<">', '<img>');
Copy link
Member

@sgolemon sgolemon Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Create some additional cases and try to break things.
<img alt=< />
<img alt='foo\' :<' />
etc...

Copy link
Author

Choose a reason for hiding this comment

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

Your second example is interesting, the entire tag gets removed (also in the current PHP version) because the in_q logic seems to get confused by the \'. From what I can find, escaping a quote using backslash is not allowed in XHTML/HTML (https://stackoverflow.com/a/34880976), but the documentation says that broken attributes are not removed.. What do you think, should we leave the current behaviour intact or try to fix that as well here?


?>
--EXPECT--
<img src="example.jpg" alt=":> :<">