Bug #6154
closed
Notices are displaying raw html (interpreted by the browser), potential XSS vector
0%
Description
The text of a notice is displayed as passed, presumably to allow for links in the notice text. At least one place that files notices -- the AutoConfigBackup package -- is passing through raw HTML from a server response, which breaks the display and is a potential XSS vector.
Fixing ACB may be good enough, though it still seems a bit dangerous, maybe only allow certain tags like "a" to show through, and strip out any script/onload/etc? Needs some thought.
Updated by Anonymous almost 10 years ago
It seems like in addition to fixing ACB, it would be good to sanitize notices as you suggest. A sanitiize_text() function might be useful elsewhere and could simple zap strings such as script, onload, onclick, onchange etc.
Updated by Anonymous almost 10 years ago
- Priority changed from High to Normal
Lowered priority since JimP has taken care of the immediate problem.
Updated by Chris Buechler almost 10 years ago
ACB is good now. But head.inc probably should sanitize $id and $notice unconditionally. Any reason we couldn't always run those two through htmlspecialchars? I don't see anything using links within $notice, and even if they are that's probably not a great idea. That'd prevent notices from breaking the GUI in addition to security benefits.
Updated by Jim Pingle almost 10 years ago
Changing ACB directly was the quickest way to get a fix for everyone without updating pfSense itself, but looking through the source at uses of file_notice in deeper detail, doing it for all notices may be fine. Initially I thought I saw some place that relied on using HTML inside the message, forming a link or similar, but it looks like maybe all of that has shifted to using the $url parameter when filing a notice.
So we could probably do with an htmlspecialchars() on $notice['notice'] at source:src/usr/local/www/head.inc#L671 and for ACB on RELENG_2_3 we could revert or change the htmlspecialchars() it does to avoid having it done twice.
As for id, as long as htmlspecialchars() is run only on $notice['id'] at source:src/usr/local/www/head.inc#L668 that should be fine.
I suppose it could also be done for both in source:src/etc/inc/notices.inc#L93 and source:src/etc/inc/notices.inc#L94 as well when filing the notice rather than on display.
Updated by Chris Buechler almost 10 years ago
I'd rather store it safely, as there's less chance it'll become a security issue in our code, or outside of it where people are shipping notices elsewhere. For instance, discussed here.
https://thehackerblog.com/poisoning-the-well-compromising-godaddy-customer-support-with-blind-xss/
Pushed change to run all 5 through htmlentities before storing. That doesn't appear to hurt any current usage of notices.
Updated by Chris Buechler almost 10 years ago
- Status changed from Assigned to Feedback
- Assignee changed from Anonymous to Chris Buechler
added sanitation of the output in head.inc as well