In 1.6x the inc/datahanders/users.php sanitized all the user input with htmlspeciachars_uni(). In 1.8x a lot of the sanitizing is removed.
I'm not sure if this was done because it was expected on the output for the sanitizing to occur but those of us who upgraded from a 1.6x forum and had custom pages and plugins that didn't sanitize that output now face security risks.
Example is usertitle.
In 1.6x:
$this->user_update_data['usertitle'] = $db->escape_string(htmlspecialchars_uni($user['usertitle']));
In 1.8x:
$this->user_update_data['usertitle'] = $db->escape_string($user['usertitle']);
I'm not sure how or why sanitizing was removed but I think it should immediately be added back.
You should compare it against a 1.6.16 version and you'll see what I'm talking about.
I may end up doing a PR at Github for this because I want it fixed before the next update. But can it be explained before I do why it was removed, was it intentional?
(2018-04-26, 06:13 PM)labrocca Wrote: [ -> ]But can it be explained before I do why it was removed, was it intentional?
Escaping values more than once would result in the HTML entities (that cause the original characters to be displayed without having an effect on the HTML structure) being broken down further, producing corrupted text.
Escaping on output is better because:
- the original content is kept intact making it easier to interpret or convert data (e.g. if someone decided to allow specific HTML tags),
- there are fewer points of failure (otherwise e.g. we'd need to make sure it's escaped properly in the ACP, Mod CP, and frontend for save & edit actions + plugins),
- the filtering works for all past & future content immediately after it's implemented,
- the application is protected regardless of how the data was saved or modified (limited access to the database, perhaps as a result of unrelated vulnerability, shouldn't mean the XSS protection can be also bypassed).
I was wondering this but was there any documentation to admins that plugins and custom pages might be effected? To me this is a big change which creates XSS exploits on legacy code and the community should have been informed.
Quote:there are fewer points of failure (otherwise e.g. we'd need to make sure it's escaped properly in the ACP, Mod CP, and frontend for save & edit actions + plugins),
Actually escaping in user handler seems the least amount of failure.
My main issue is that now previously written custom code is vulnerable to XSS because of the change.
Quote:the filtering works for all past & future content immediately after it's implemented,
What does that mean? Please explain.
Quote:the application is protected regardless of how the data was saved or modified (limited access to the database, perhaps as a result of unrelated vulnerability, shouldn't mean the XSS protection can be also bypassed).
Again, if you had a custom page or plugin that showed usertitle for example, you didn't need to sanitize it because user handler already did it. Now it doesn't. Now all that legacy 1.6x code is vulnerable. I'd like to know if that's going to be fixed. I'd also like to see any documentation about this change that was provided to the community. Because it's a HUGE alteration that puts every MyBB forum with custom code at risk. And maybe this documentation has more information I would need to know about.
If you sanitized in 1.6x and don't sanitize in 1.8x I think admins need to be informed. And I think it's a bad change and should be undone.
(2018-04-26, 08:21 PM)labrocca Wrote: [ -> ]Quote:the filtering works for all past & future content immediately after it's implemented,
What does that mean? Please explain.
I believe what Devilshakerz means is that if escaping is done on output, all previous data that is already in the database (regardless of whether it is/was escaped) will be escaped no matter what. It's widely regarded that best practice is to store data in the form that it was provided, and do filtering/escaping when making use of that data - it could be that data doesn't want to be escaped or needs to be escaped in a different way depending on the context that the data is used in (eg: in plain text output, escaping using HTML entities makes no sense and causes garbled text).
I don't believe that this change was documented, though the original release of 1.8.0 was too long ago now for me to give a definitive answer either way off the top of my head and most of the people who were on the team or making decisions at that time have since moved on.
Quote:It's widely regarded that best practice is to store data in the form that it was provided, and do filtering/escaping when making use of that data
C'mon bud. You guys made a change that opened up everyone to XSS in their custom pages and plugins and never notified anyone they should check their code. If MyBB did tell admins then I'd like to see that.
This was IMPORTANT enough that a sanitizing change should have required the team to notify admins and plugin authors. I have a feeling that a lot of sites are now vulnerable. I know that every plugin I had that showed usertitle is now vulnerable in 1.8x even if the plugin actually works.
And I suppose if you don't revert the change I'll make my adjustments to my own code but my main gripe is the lack of notification to a security standard alteration. Something like that should be announced.
Hindsight 10/10, it's hard to announce what affects which plugins in detail.
1.8.0 had many many changes, it was a major update after all. 1.9.0 will have many changes.
Plugin developers are supposed to run their own tests, not just change a compatibility line.
That said I'm rather sure you had to sanitize all sorts of output yourself back in 1.6 (and 1.4) too.
Looking at the code it's still in there actually in another code path:
"usertitle" => $db->escape_string(htmlspecialchars_uni($user['usertitle'])),
(but not for usernames etc. etc. etc.)
probably makes no difference in this case but should be removed anyway
looking at the blame, the htmlspecialchars was removed in a changeset unrelated
https://github.com/mybb/mybb/issues/900
so there was no one out to harm people, was just removed for consistency with other values/fields I guess *shrugs*
There's something similar going on with private message folder names in private.php:
$foldername = $db->escape_string(htmlspecialchars_uni($val));
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.
I do agree that such a change should have been announced, and that it should be consistent (eg: should always be escaped on output, not input, or always escaped on input - a mix of the two makes no sense). As frostschutz said, hindsight is great, but unfortunately this change wasn't announced and I can hardly jump in a time machine to go back and announce it for the 1.8.0 release now.
I'm not opposed to applying the PR you provided, but as I said on GitHub we need to make sure it doesn't lead to double escaping.
You can't rely on database content to be already sanitized. So "already sanitized from MyBB" would mean by some MyBB API (like if MyBB's function.php had a format_usertitle()) or one of MyBB's public variables. Then you can rely on it - have to rely on it - as otherwise you'd double-sanitize which is &amp. But not if you query the data yourself (unless there's a database query API that spews out pre-sanitized values).
It can't work because if you put data sanitized into a database, it breaks queries and searches. Even if you encode the query string too, there is more than one way to encode a string with HTML entities and the implementation may be subject to change over time. So the strings might not match even if what they represent is identical.
And then you still have to consider people tampering with their database in phpmy/adminer. If you can make MyBB produce illegal output it's bad, even if it was done by database tampering.
Does the new template engine for MyBB 1.9 have any built-in sanitization feature? Most of the time (say, if you are not parsing bbcode, or well put templates inside templates I guess), the template should be the sole authority on HTML structure and all $vars should just be content/strings so the template engine could handle sanitization centrally rather than that garden variety of htmlspecialchars strewed all over everywhere.
Documentation of specific issues aside, changes like this in minor point releases should be expected - plugins, themes or custom modifications for 1.6 or earlier branches in general don't go together with 1.8 code.
Even if the double escaping problem is taken care of while restoring escaping on input, it would still go against best practices and be another exception to the rule (there are some places where HTML is being escaped before saving, as you point out). The goal is to have better code with fewer inconsistencies (and indeed, better documentation) so problems like yours don't happen.
(2018-04-27, 07:48 PM)frostschutz Wrote: [ -> ]Does the new template engine for MyBB 1.9 have any built-in sanitization feature? Most of the time (say, if you are not parsing bbcode, or well put templates inside templates I guess), the template should be the sole authority on HTML structure and all $vars should just be content/strings so the template engine could handle sanitization centrally rather than that garden variety of htmlspecialchars strewed all over everywhere.
All variables will be escaped by default, unless the
raw filter is applied.