Thanks ahead of time.
Our IT Central folks have flagged our WeBWorK 2.12 server with a Cross-Site Scripting vulnerability. I used the Plain Vanilla WW2.12/Ubuntu 16.04 ISO to install our present WeBWorK server this past May, with some (obviously not enough) customization. What is needed to patch our server to remove this vulnerability?
We'll need more information from your IT department to track this down. Do they have the URL that displays the XSS vulnerability (that will tell us the offending page from WeBWorK) and if possible what kind of XSS vulnerability exists? If they can give us a command that triggers the XSS vulnerability that would be ideal.
Often these XSS vulnerabilities arise from an error message that returns a little too much information.
-- Mike
With the help of our IT Central folks here at the University, we were able to resolve this XXS issue. The solution involved removing the various error responses in the ...
/opt/webwork/webwork2/lib/Apache/WeBWorK.pm
... file. Basically, from the Vanilla 2.12 with Ubuntu 16.04 install (without too many other modifications), the following lines of code from the file above ...
<div style="text-align:left">
<h2>WeBWorK error</h2>
<p>An error occured while processing your request. For help, please send mail
to this site's webmaster $admin, including all of the following information as
well as what what you were doing when the error occured.</p>
<p>$time</p>
<h3>Warning messages</h3>
<ul>$warnings</ul>
<h3>Error messages</h3>
<blockquote style="color:red"><code>$exception</code></blockquote>
<h3>Call stack</h3>
<p>The information below can help locate the source of the problem.</p>
<ul>$backtrace</ul>
<h3>Request information</h3>
<table border="1">
<tr><td>Method</td><td>$method</td></tr>
<tr><td>URI</td><td>$uri</td></tr>
<tr><td>HTTP Headers</td><td>
<table width="90%">
$headers
</table>
</td></tr>
</table>
</div>
... were truncated to this ...
<div style="text-align:left">
<h2>WeBWorK error</h2>
<p>An error occured while processing your request. For help, please send mail
to this site's webmaster $admin, including all of the following information as
well as what what you were doing when the error occured.</p>
<p>$time</p>
</div>
These represent lines 233-256 in the original WeBWorK.pm file (your line #'s may vary). After a sudo service apache2 restart, this trouble-ticket, cross-talk vulnerability, was resolved.
Thanks to you and your IT department for your work on the XXS exploit. This is a good fix for now but I'd like to see a solution that still allows some error message to be returned. At the moment
For help, please send mail
For help, please send mail
to this site's webmaster $admin, including all of the following information as
well as what what you were doing when the error occurred.
As it stands there is no "information below" and the webmaster will not have any idea of what error occurred. Of course balancing providing useful debugging information while at the same time preventing XXS exploits and other safety issues has always been a problem. It will take a little thought and some work to see if we can provide both or at least reach an optimal compromise.
Dear Michael,
I am not sure whether this should belong to a new thread, but my department has encountered something of a similar nature when our university's central IT folks subjected our WeBWorK server to a vulnerability test.
One thing which they have flagged as a cross-site scripting vulnerability is the ability of the following URL:
<webwork host>/math1234/?status_message=<aUdIo%20SrC=x%20OnErRoR=alert(61212)>
to trigger an alert on the browser of a user with professor-level or above privilege. I suppose the security risk is that the "alert" function could potentially be replaced with something a lot more malicious.
Seeing that the argument to "status_message" is processed by the "message" routine under ContentGenerator.pm, I made the following change:
sub message {
my ($self) = @_;
print "\n<!-- BEGIN " . __PACKAGE__ . "::message -->\n";
# print $self->{status_message}
# if exists $self->{status_message};
if (exists $self->{status_message}) {
my $sterilized = $self->{status_message};
$sterilized =~ s/<|>/%/g;
print $sterilized;
}
print "<!-- END " . __PACKAGE__ . "::message -->\n";
return "";
}
Basically, it replaces all the pointy brackets of any potential html tags in the message argument. Perhaps not the most elegant solution... My question is: Would it be possible to avoid the vulnerability without touching the source code? If not, is the way I altered the code above safe from a security standpoint? Are there better ways still to handle this?Thank you very much!
-- P-S
Fixing this will almost certainly require editing the source code. There are other functions in ContentGenerator.pm that use the HTML::Scrubber package to remove any scripts from html messages. Take a look at the addmessage routine to see how it is implemented. Using the same code is probably the cleanest way to do this robustly.
If you do change the code, please submit a pull request back to the main repository on GitHub so it can be included in the next release.
If you do change the code, please submit a pull request back to the main repository on GitHub so it can be included in the next release.
Dear Danny,
Thank you very much for your reply. It appears that the current settings of $scrubber (as of WeBWorK 2.15) do not scrub html events like "onerror" and etc.
It seems there are all sorts of on* events in html, and I am not even sure where a comprehensive list can be found (I'd imagine it'd also depend on which browser we are talking about). Anyway, I went on the forum https://www.perlmonks.org/index.pl?node_id=251427 and a commenter has posted a pretty long list. It appears that adding the following lines after each definition of $scrubber (for example under 'sub warningOutput' in ContentGenerator.pm) does avert the URL attack referenced in my first post:
Does it look right? Thank you very much!
Best regards,
Ping-Shun
p.s. If it looks fine I will submit a pull request.
Thank you very much for your reply. It appears that the current settings of $scrubber (as of WeBWorK 2.15) do not scrub html events like "onerror" and etc.
It seems there are all sorts of on* events in html, and I am not even sure where a comprehensive list can be found (I'd imagine it'd also depend on which browser we are talking about). Anyway, I went on the forum https://www.perlmonks.org/index.pl?node_id=251427 and a commenter has posted a pretty long list. It appears that adding the following lines after each definition of $scrubber (for example under 'sub warningOutput' in ContentGenerator.pm) does avert the URL attack referenced in my first post:
$scrubber->default(
undef,
{
'*' => 1,
'onabort' => 0,
'onactivate' => 0,
'onafterprint' => 0,
'onafterupdate' => 0,
'onbeforeactivate' => 0,
'onbeforecopy' => 0,
'onbeforecut' => 0,
'onbeforedeactivate' => 0,
'onbeforeeditfocus' => 0,
'onbeforepaste' => 0,
'onbeforeprint' => 0,
'onbeforeunload' => 0,
'onbeforeupdate' => 0,
'onblur' => 0,
'onbounce' => 0,
'oncellchange' => 0,
'onchange' => 0,
'onclick' => 0,
'oncontextmenu' => 0,
'oncontrolselect' => 0,
'oncopy' => 0,
'oncut' => 0,
'ondataavailable' => 0,
'ondatasetchanged' => 0,
'ondatasetcomplete' => 0,
'ondblclick' => 0,
'ondeactivate' => 0,
'ondrag' => 0,
'ondragend' => 0,
'ondragenter' => 0,
'ondragleave' => 0,
'ondragover' => 0,
'ondragstart' => 0,
'ondrop' => 0,
'onerror' => 0,
'onerrorupdate' => 0,
'onfilterchange' => 0,
'onfinish' => 0,
'onfocus' => 0,
'onfocusin' => 0,
'onfocusout' => 0,
'onhelp' => 0,
'onkeydown' => 0,
'onkeypress' => 0,
'onkeyup' => 0,
'onlayoutcomplete' => 0,
'onload' => 0,
'onlosecapture' => 0,
'onmousedown' => 0,
'onmouseenter' => 0,
'onmouseleave' => 0,
'onmousemove' => 0,
'onmouseover' => 0,
'onmouseout' => 0,
'onmouseup' => 0,
'onmousewheel' => 0,
'onmove' => 0,
'onmoveend' => 0,
'onmovestart' => 0,
'onpaste' => 0,
'onpropertychange' => 0,
'onreadystatechange' => 0,
'onreset' => 0,
'onresize' => 0,
'onresizeend' => 0,
'onresizestart' => 0,
'onrowenter' => 0,
'onrowexit' => 0,
'onrowsdelete' => 0,
'onrowsinserted' => 0,
'onscroll' => 0,
'onselect' => 0,
'onselectionchange' => 0,
'onselectstart' => 0,
'onstart' => 0,
'onstop' => 0,
'onsubmit' => 0,
'onunload' => 0
}
);Does it look right? Thank you very much!
Best regards,
Ping-Shun
p.s. If it looks fine I will submit a pull request.
I'm not an expert on this, but if I understand the existing scrubber code correctly then it is scrubbing all scripts, which means that it doesn't have to worry about individual events because they would have to be contained in a script. Can someone confirm whether this is correct?
Dear Danny,
I am guessing that the URL sent by my IT department's scanner triggered an alert because "onerror" was contained in an <audio> tag with a bogus src attribute. So, no script tag was explicitly involved.
But perhaps there is a cleaner solution to including that long list of DOM events in the scrubber rules. It appears to me that the HTML element attributes that I ever encountered in system messages were mostly "class" attributes within <div>'s (e.g. ' class="ResultsWithoutErrors" ') If that's the case, wouldn't it be simpler to make scrubber allow only the 'class' attribute, or perhaps a few more harmless ones?
Thank you very much!
Best regards,
Ping-Shun
It was my (possibly flawed) understanding that the scrubber package was specifically designed to prevent things like XSS vulnerabilities, so I'm surprised that there isn't an easy way to scrub anything that looks like a script. At this point this is beyond my level of expertise, so I'm hoping someone else will join the conversation with more knowledgeable input.
Dear Danny,
Thanks for looking into it.
For what it's worth, on https://metacpan.org/pod/HTML::Scrubber, the example given there also has DOM event attributes hard coded in even after "script => 0" has been set.
Anyway, our quick solution here is to escape all HTML tags in $warnings (and $messages) with "HTML::Entities::encode_entities($warnings)". A total of maybe 4 lines were changed, in lib/Apache/WeBWorK.pm and lib/WeBWorK/ContentGenerator.pm.
These changes allowed the server to pass all XSS vulnerability tests given by our IT colleagues, but of course, all system messages and warnings are now a little ugly because the HTML tags are not rendered anymore. Functionally speaking, the full messages can still be seen, so that's "good enough" for us.
Alternatively, setting:
Thanks for looking into it.
For what it's worth, on https://metacpan.org/pod/HTML::Scrubber, the example given there also has DOM event attributes hard coded in even after "script => 0" has been set.
Anyway, our quick solution here is to escape all HTML tags in $warnings (and $messages) with "HTML::Entities::encode_entities($warnings)". A total of maybe 4 lines were changed, in lib/Apache/WeBWorK.pm and lib/WeBWorK/ContentGenerator.pm.
These changes allowed the server to pass all XSS vulnerability tests given by our IT colleagues, but of course, all system messages and warnings are now a little ugly because the HTML tags are not rendered anymore. Functionally speaking, the full messages can still be seen, so that's "good enough" for us.
Alternatively, setting:
$scrubber->default(
undef,
{
'*' => 0, # all attributes disabled by default
'class' => 1 # only class attributes are allowed
}
);
also seems to prevent scripts from executing, and the messages, at least the ones I come across, look as nice as before.
But this hasn't been fully tested yet.
Best regards,
Ping-Shun
Dear Ping-Shun,
It is nice of you to work on this, and for your IT staff to help improve the security aspects of WW. If you could share your code in a fork on GitHub, and eventually put it into a pull request, it should be quite easy to get the correct changes into the develop branch, and eventually into WW 2.16.
I agree with your conclusion that there is a need to scrub the input coming in from requests as a status_message value to avoid the XSS issues reported. Your suggestion of allowing only the class attribute seems to be a very good one. It could be that this does not need to be done to all possible uses of addmessage().
The main issue with XSS is probably with the few places where form provided values of status_message are read in and used to create the "new" status message. Those places use "$r->param("status_message");" (This seems to be intended to carry over status_message values passed on by some "prior" page.) It seems likely these are the cases which cause the the XSS issue you can trigger by providing a value of status_message in the URL. It could be that it suffices to apply stricter scrubbing rules only in these cases using a add_scrubbed_message() replacement for addmessage() in those settings. Your IT people could help test whether that is really sufficient.
Here are the cases where form data seems to be pulled in to be used in a status_message in webwork2/lib:
- lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor2.pm:383: $self->addmessage($r->param('status_message') ||''); # record status messages carried over if this is a redirect
- lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor3.pm:384: $self->addmessage($r->param('status_message') ||''); # record status messages carried over if this is a redirect
lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm:378: $self->addmessage($r->param('status_message') ||''); # record status messages carried over if this is a redirect
lib/WeBWorK/ContentGenerator/Instructor/AchievementEditor.pm:137: $self->addmessage($r->param('status_message') ||''); # record status messages carried over if this is a redirect
lib/WeBWorK/ContentGenerator/ProblemSet.pm:62: my $status_message = $r->param("status_message");
lib/WeBWorK/ContentGenerator/CourseAdmin.pm:64: my $status_message = $r->param("status_message");
lib/WeBWorK/ContentGenerator/ProblemSets.pm:147: my $status_message = $r->param("status_message");
lib/WeBWorK/ContentGenerator/Problem.pm:580: my $status_message = $r->param("status_message");
The "stronger" scrubbing could probably by default block all HTML tags except a short white-list including DIV, SPAN, P, HR, UL, and LI and maybe a few simple formatting tags (including CODE), as well as blocking all attributed except class as you suggested. Then, if additional tags should be white-listed - it can be done so later. Maybe something like:
my $scrubber = HTML::Scrubber->new(
default => 0, # No longer 1
allow => [ qw[ div span p b i u hr br ul li code ] ] ,
script => 0,
comment => 0
);
Passing the class attribute is About attributes to pass - I suspect that only class is really needed, as that would likely allow the CSS based formatting to continue to work.
- Most usages of addmessage() seem pretty simple and basically set stings which are often enveloped in one of CGI::div, CGI::span, and CGI::p where some cases set a class attribute. I suspect that those should not be causing any problem, as no client side data is included in those typical usages.
- There are also uses of addbadmessage() and addgoodmessage() which get passed to addmessage(). Several use CGI::br().
- There is one use of CGI::code in lib/WeBWorK/ContentGenerator/Hardcopy.pm and I found some other places where <CODE> and </CODE> are being used via addbadmessage in lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm and lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList2.pm
- lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm makes use of CGI::ul and CGI::li
- Some calls of the forms $self->addbadmessage($msg); $self->addgoodmessage($msg); $self->addgoodmessage($message); $self->addbadmessage($write_result); need to be checked. (I gave up on digging down that far.)
- There are 3 places where addmessage() is called on the output from $self->$actionHandler . Someone needs to check what sort of output those calls are generating, as they may be the most critical places where too much scrubbing might make trouble. I took a very quick look and it seems that they also basically are strings and DIVS with some use of the class attribute.
- lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm:214
- lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm:387
- lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList2.pm:404
Dear Nathan,
Thank you very much for your detailed overview of the situation.
I didn't even realize there were all these other places, like AchievementList.pm, that generate messages. I guess the issue is that if a revised scrubber is placed too downstream, or the white list is made too short, one ends up potentially taking away useful features.
Anyway, in my fork of webwork2, I made changes to the scrubber settings so that it allows only the 'class' attribute, but allows all html tags except script. You are welcome to have a look:
https://github.com/pschan-gh/webwork2/commit/d3ecb96c8393bd9d015b615e922e7bdd4c03c65a
These changes scrub messages pretty much right before they are presented to the webpage, so based on what you said, it's probably not the most ideal solution, since it might take away functionalities of other webwork features. So, perhaps right now it's at best something that serves to start a discussion. For testing purposes, I include below a few URL's that trigger unwanted alerts. They work only if the user logs on as a professor or admin. The changes I made to my fork appear to thwart these "attacks".
Thank you very much!
Best regards,
Ping-Shun
http://localhost/webwork2/TestingCourse/?status_message=%3CaUdIo%20SrC=x%20OnErRoR=alert(61212)%3E http://localhost/webwork2/TestingCourse/instructor/setmaker/?last_index=-1%22%3E%3CsVg%20OnLoAd=alert(16676)%3E
http://localhost/webwork2/TestingCourse/instructor/?selected_users!sort=lnfn%3CaUdIo%20SrC%3dx%20OnErRoR%3dalert%2817166%29%3E
http://localhost/webwork2/TestingCourse/instructor/sets2/?action=filter%3CaUdIo%20SrC=x%20OnErRoR=alert(26176)%3E
http://localhost/webwork2/TestingCourse/instructor/send_mail/?rows=15%22%20sTyLe=X:eX/**/pReSsIoN(alert(24226))%20%22