Question about the recent XSS security fix
| Project: | Webform |
| Version: | 6.x-2.7 |
| Component: | Code |
| Category: | support request |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | closed |
Jump to:
I was looking at the code change, and trying to ascertain why it was made.
The old code was: check_markup($data['value']['0'], FILTER_HTML_STRIP, FALSE);
The new code is: check_plain($data['value']['0']);
AFAIK, provided that standard input formats exist (or more specifically, format 1 exists and has the HTML filter assigned), then the original code would have dealt with any XSS issues by calling filter_xss() when running the HTML filter.
I'm presuming that the change was therefore made to ensure safe output when people have modified or removed the default input formats.
Is that correct, or is there more to it than that? (I'm especially keen to know if filter_xss() is not actually sufficient in some circumstances?)
Thanks in advance for your help.
#1
The problem that occurred with the old code was that the constant
FILTER_HTML_STRIPdoes not exist (it used to exist back in 4.7 or 4.6 versions of Drupal). Since this filter does not exist, and because the permission check was skipped (the FALSE parameter), the call to check_markup() was effectively doing nothing.We could have just as easily used filter_xss() solve the problem, but considering that the contents of textareas are never rendered as HTML when viewing/editing/emailing/downloading submissions, check_plain() made more sense to keep consistency in the display.
#2
Well filter.module does still define that constant (as value 1) in Drupal 5 and 6. It doesn't look as though it's intended to be used as a format ID, however? (which is maybe what you were saying).
So it seems like the old call to check_markup() would have tried to execute all the filters for format 1 (or, I now notice, the default format if format 1 does not exists), and thus filter_xss() would have been called provided that the HTML filter was enabled for that format.
But explicitly calling filter_xss() is obviously more robust, and your reasoning for using check_plain() instead makes sense.
Thank you for the clarification!