[Rejected] Sanitizing User Profiles
#7
Quote:Looking at the code it's still in there actually in another code path:

Actually you have to look further down for a second section for updating the usertitle. You'll see it was sanitized in 1.6x but is not in 1.8x. It seems to get santized only on account creation which imho doesn't make much sense to do it then but not updates.

Quote:There's something similar going on with private message folder names in private.php:

We've looked into it (my pentesting team) and we didn't find a vulnerability there. Plus a private folder would only show to your own member account. There is a css glitch though but again, that only effects your own PM page.

I can't tell MyBB Team what to do but I can create open discussion about policy which I hope might improve how things are done. I'm just a little peeved and disappointed that sanitizing was removed and I don't see anywhere that it was mentioned.

Now that my members know and it's been posted here, don't you think everyone is about to go check plugins for persistent XSS? Many 1.8x plugins were just minor updates to 1.6x and they worked, they may assume they are safe when they are not.

Quote:That said I'm rather sure you had to sanitize all sorts of output yourself back in 1.6 (and 1.4) too.

I did but I also relied on already sanitized code from MyBB. Because just as it was mentioned you may not want to double sanitize. Things like get_thread() and get_user() are already sanitized.

The only reason to unsanitize usertitles in datahandler was to possibly consider HTML usertitles which isn't supported in the code and would still require core edits by a plugin author (I checked the code myself to confirm that). You basically just moved the sanitizing to pages like member.php and functions_post.php. That doesn't make a lot of sense to me. You now have to sanitize in multiple places when you could have just done so in one file which will be effecive for every plugin too.

The argument that the database should hold the raw data I don't agree with. Mainly because it's a change from the previous standard that lowers security.

@frostschutz, do you support adding the sanitizing again in the users datahandler or do you think how it is currently should remain? You're a top plugin author and I respect your view.

If my github PR gets removed then so be it. I've already made my own code adjustments but I want to use branch. If MyBB agrees, I'll find every single instance of the usertitle fields and send a PR at Github. I don't see a reason to bother unless MyBB is on board with the change. Otherwise I'll accept the situation and make my own code adjustments. I hope though my argument about other plugins being possibly vulnerable has some weight.

Thank you all for the discussion and I do hope that at the very least a bit more communication happens about changes which effect security to plugin authors.
Reply


Messages In This Thread
Sanitizing User Profiles - by labrocca - 2018-04-26, 06:13 PM
RE: Sanitizing User Profiles - by Devilshakerz - 2018-04-26, 07:40 PM
RE: Sanitizing User Profiles - by labrocca - 2018-04-26, 08:21 PM
RE: Sanitizing User Profiles - by Euan T - 2018-04-26, 09:42 PM
RE: Sanitizing User Profiles - by labrocca - 2018-04-26, 10:20 PM
RE: Sanitizing User Profiles - by frostschutz - 2018-04-26, 11:01 PM
RE: Sanitizing User Profiles - by labrocca - 2018-04-27, 05:54 PM
RE: Sanitizing User Profiles - by frostschutz - 2018-04-27, 07:48 PM
RE: Sanitizing User Profiles - by Euan T - 2018-04-27, 06:52 PM
RE: Sanitizing User Profiles - by Devilshakerz - 2018-04-27, 08:12 PM
RE: Sanitizing User Profiles - by labrocca - 2018-04-27, 08:23 PM
RE: Sanitizing User Profiles - by Omar G. - 2018-04-28, 06:44 AM
RE: Sanitizing User Profiles - by labrocca - 2018-04-28, 02:41 PM

Forum Jump:


Users browsing this thread: 1 Guest(s)