XSS Fix

[goehle]

Added html character encoding to values captured from the path to address http://bugs.webwork.maa.org/show_bug.cgi?id=3376

Test by trying a link like
your-webwork-server.com/webwork2/your-course-name/%3Ca%20onmouseover=%22alert%28%27EVIL%20JAVASCRIPT%27%29%22%3EEVIL%20LINK

You should get a bunch of annoying popups before the patch and none after.

[dpvc]

I'm not sure this is the right place for this. It should be done when the data is put into an HTML page, not when the parameters are read from the page URL or input form. That data may not be used in an HTML setting, and if you encode it now, whatever part of the code that actually processes it will get HTML-encoded values rather than the proper values that it expects. The encoding needs to be done when the values are used in a page.

I don't think there is a simple, one-location fix for this issue.

[goehle]

I see your point about the html encoding possibly interfering with something which doesn't expect it, but currently the only things that are passed via the url are course ids, set ids, user ids, achievement ids, and problem ids and none of those allow the characters encoded by HTML::Entities. So this shouldn't have an impact on valid url parameters. My vote is that we make this a "policy" moving forward. (Actually in a separate more experimental pull request I've replaced all the ID's with numbers and in that case I would be even stricter about what would be allowed as a url parameter.)

Its not completely a matter of laziness on my part. The issue with fixing it the right way is that even if I did track down every place that a user id, set id, course id, achievement id, or problem id is printed and encode there, the next time someone submits a pull request that prints one of those values without encoding, it will create an XSS hole.

[goehle]

Actually, as an alternative I could just run the checkKeyfields method from DB.pm on the url args and cause the page to die if the url parameters dont check out.

[dpvc]

I don't know the URLPath interface well enough to know what gets passed, and I'm not up on the status of what is allowed in course IDs and other such data that is passed as part of the URL, so I'll take your word for that. But don't file names for things like images, pdf files, and such also go through that? Are there limitations on those?

If the IDs that get passed actually do have character limitations, I'd much rather see you filter out the illegal characters rather than HTML encode them. That way you would always get a valid ID rather then IDs potentially screwed up with HTML entities.

I didn't mean to suggest it was laziness, and I know that it would be very difficult to catch all the places that this could happen. But I think your solution can also cause problems in the future. For example, if I write a page that correctly encodes the data being put in the page, then if you have already pre-encoded it, the results will be double encoded. I don't know what values you may have already pre-encoded. This kind of thing has already happened when we changed the warning message facility when output from things like pretty-print were used in diagnostic messages.

I continue to feel that HTML escaping is the responsibility of the person creating the output. That does mean we need to be thinking about it, and looking for it as new code is submitted. Having some utilities to make it easier to do the escaping would be a help with that (as we would check if those are being used, for example). It needs to be part of the mindset of code contributors and reviewers.

While it is more work to make this happen, it is possible to do. But forcing encoding earlier makes it impossible for someone who is thinking about these ideas to handle the data correctly when there is a mix of encoded and un-encoded values.

Perhaps perl's taint facilities could be used to help with this stuff.

(I see you just commented while I was writing. I think your suggestion is similar to what I recommended above.)

[goehle]

This doesn't encode all of the html parameters, that wouldn't work for the reasons you mentioned. This encodes the url path parameters. Since those are all key fields of one type or another I can use the DB methods to validate them instead.

You make a good point about encoding in general. I'm not fond of unsafe strings and tend to think its an issue of figuring out which will be more grief in the long run, patching xss holes, or patching double encoding issues. On the other hand this kind of thinking is what led to the $ encoding bug.

[dpvc]

Since those are all key fields of one type or another I can use the DB methods to validate them instead.

That sounds like a better approach.

On the other hand this kind of thinking is what led to the $ encoding bug.

Yes, that is the kind of thing that I had in mind. I have certainly run into this sort of problem on several occasions while writing the MathObject or PGML libraries, and in some cases it was impossible to do the right thing because of things that were encoded too early.

[goehle]

I refactored the fix. Now instead of encoding the characters if you use an illegal character in the url path the system will croak. Test by using the same link above or just putting illegal set id characters in for a set id.

• Check that the system does not allow invalid characters in course ID, set ID and user ID places.
• Check that all of the usual pages still work.
• Check that db validation still works by trying to add a set or user with an invalid parameter.

[mgage]

Works ok so far. Just double checking that the error message being emitted by the croak() command is
translating HTML entities. We don't want the evil javaScript placed anywhere in the body of the error message HTML page.