philippK-de / Collabtive

Collabtive is web based project management software
https://collabtive.o-dyn.de
GNU General Public License v3.0
215 stars 131 forks source link

Mitigate XSS attack on user profile page #120

Closed mdwheele closed 8 years ago

mdwheele commented 8 years ago

Came across this disclosure on Reddit today and figured I'd send some help while I had time.

See: https://devwerks.net/blog/16/how-not-to-use-html-purifier

Completely ignoring the snarky and passive aggressive title of their post, there was enough information in there to mitigate this specific attack. I am unfamiliar with this project so this may not be the "best way" to implement, but it does mitigate this attack while not impacting end-users at all, as far as I can tell.

If there are any other templates that output two variables in the same local space, they will need a similar treatment.

Hope all is well!

Screenie of output after mitigation:

image

philippK-de commented 8 years ago

thank you. there was some work already done on the userprofile for 3.0 to improve against XSS. Generally speaking this seems like an edge-case,i am not aware. I will investigate for more occurences of this vector.

mdwheele commented 8 years ago

Generally speaking this seems like an edge-case,i am not aware.

I feel like that's accurate. There isn't any real "blanket" protection that I can think of that would have caught this kind of targeted attack.

Thanks!

kelunik commented 8 years ago

@mdwheele @philippK-de This vulnerability is not caused by outputting two variables on the same line. This vulnerability is caused by the combination of two things: Escaping on input instead of output and using MySQL in non-strict mode.

MySQL in non-strict mode silently truncates too long strings. As zip is defined as being VARCHAR(16), it's just truncated and makes the purification useless.

I can only strongly advise to go trough the whole source and escape / purify all variables on output that contain user input or don't need HTML. As you only use purification and no escaping, you don't risk any double escaping. You can then remove the purification on input.

Another mitigation is to turn on MySQL strict mode, so things like that just error.

philippK-de commented 6 years ago

https://github.com/philippK-de/Collabtive/commit/b1bf8d5dd5b8052099bab738efd4e95c46c8e70a