-
☆ A M B ☆
- 24,524 Posts
Dunno. Either one should do the job; the htmlspecialchars doesn’t remove them, just encodes them, so I suppose it might be better. I think it all depends on the purpose of the string being sanitized. I was working under the presumption that the two values should not have those tags in any case. Six of one and half a dozen of the other, I would guess. But I’m no expert on these things.
Interesting. Seems like some sanitizing does indeed need to be done on the manager search var and the search highlighting plugin. Personally I would strip_tags to be sure that no code can be injected, but as Susan says it’s probably six of one, half dozen of the other.
So for the manager search var injection to work you do need an active manager session, which certainly makes me think that it would be difficult to implement in the real world (I gather that you would need to sidejack the session first). I do see the javascript code in the page source when I try the exploit URLs, but it doesn’t execute as far as I can tell. Still, just the ability to inject code like this would make me think that it wouldn’t be too difficult to toy with it until you found some code that does execute and do something useful.
But regardless that one seems easy to fix and not a very big risk given how difficult it would be to actually take advantage of.
The search highlighting plugin vulnerability could be a little bit more serious as far as I can tell because it doesn’t require a manager session to work. I can’t get it to inject code on my installations, however, so perhaps it doesn’t actually work as advertised. I have patched my highlight plugins with strip_tags just in case, however (and they work fine that way, although if you have a nerd site where people might actually search for tags then this might not be ideal).
So that seems like an easy patch that’s worth doing and discussing so that resource developers know to watch out for this possibility with user input.
I haven’t looked into the index-ajax.php issues yet because in general I don’t upload that file unless I need it (and I very rarely need it). It also seems like a matter of sanitizing posted input, but then again it may be overstated. Regardless it doesn’t appear to be a difficult thing to patch if indeed there is a problem there.
As far as the general fear of pirating manager sessions goes, I imagine that this could be accomplished by sidejacking. My understanding of how this works is somewhat limited, but I believe that it’s only a real concern when you are logging in via an insecure network (usually unencrypted wireless hotspots). Other people on that network can intercept your session cookie and use it for as long as you remain logged in.
This is a real and serious issue, but unfortunately it’s generalized to pretty much all web apps that allow for user logins so I think that it’s more of a user education issue than a treatable security breach. Connecting to the manager via SSL would likely protect you against sidejacking even over insecure networks (Gmail sidejacking over SSL was recently demonstrated, but I think that was only possible given some specific knowledge of how the Gmail system works and the https URLs that it uses), but of course the best solution is not to use them. Given that sidejacking is a problem that affects so many web apps, I expect that there will be some countermeasures introduced in the next few months that MODx might want to consider implementing (although I can’t see a way to protect against this from the script side while still using sessions).
Anyway... the long and the short of what I can make of this announcement is that it doesn’t worry me that much. There is something to them, but nothing that I personally find all that worrying. There’s no such thing as absolute security, but MODx is as secure as any web app that I’ve ever used and I’m always encouraged by how quickly and proactively the team responds when there are issues that need to be dealt with.
I had the code! days before the MODx0.9.6.1pach release and i thought it doesn’t make sense any more and deleted, Interesting part is that how i found the script!, I was checking the latest visitors log from the Cpanel and found some crazy link reference from yahoo free pages called geocities, So i browsed the url and found the code. Just now checked the log visit to see whether i got the second chance to see the crazy referrer or not but there was nothing. BTW: i have upgraded the same exploited site of mine to patch release!, so it’s better to check the latest log archive and have the crazy looking URL’s checked.
-
☆ A M B ☆
- 1,056 Posts
Quote from: rthrash at Feb 09, 2008, 09:00 AM
The current installer in the trunk has those lines as the following (added by Pixelchutes on Jan 23):
$searched = htmlspecialchars(urldecode($_REQUEST['searched'])); // pixelchutes
$highlight = htmlspecialchars(urldecode($_REQUEST['highlight'])); // pixelchutes
What’s the best course of action in this case?
Given the purpose of the plugin (highlighting specific words within the content), I think that Susan’s suggestion for
strip_tags makes for a better choice here. I’ll commit to SVN.
-
MODX Staff
- 12,272 Posts
Thank you Megu. The pending release codebase has been patched, but we’ll update the announcement to note this fix.
Ryan Thrash, MODX Co-Founder
Follow me on Twitter at @rthrash or catch my occasional unofficial thoughts at thrash.me
My take on this would be: Yes - if you patch the search highlighting plugin as you describe (or copy from the latest version in the repository) or disable it then you are safe from this possible vulnerability.
The example that you give for a potential exploit is interesting. I hadn’t thought of potential phishing-style attacks using injected JavaScript in this way. That method would also work with the Manager bugs, I believe.
Say for example someone posts a bizarre Jot comment to your site and you review it while logged into the manager. Within this comment is a URL that exploits one of the manager input validation bugs to redirect you to a phishing site that appears to be the MODx manager but is actually a hacker site. If it were good enough to fool you (which would be easiest if it could be inserted into your manager frameset somehow), then you might enter or otherwise disclose info that could be used against you by hackers.
This is a rather remote possibility, I admit, but not impossible as far as I can tell. I haven’t been able to get injected JavaScript to execute within the manager using these methods, but that doesn’t necessarily mean it can’t be done. So although these are unlikely real-world attack vectors in my opinion, I’d say it’s worth patching them in future releases anyway.
Not that I’m going to run off and patch my sites or anything, since I don’t see much likelihood of this attack ever actually being implemented.
Dear all,
Thank you, rthrash and ZAP.
We thank your fast response very much.
I also updated announcement against Japanese users.
Sincerely yours,
MEGU
I got the security notice by email... but which file do I have to patch around line 52? Could the filename be included in future announcements, please.
Thanks,
Peter
Quote from: posword at Feb 14, 2008, 06:39 PM
I got the security notice by email... but which file do I have to patch around line 52? Could the filename be included in future announcements, please.
The code that you need to patch is actually not in a file. It’s the Search Highlighting plugin, which you can edit via the Manager in Resources -> Plugins. That code is saved into the database, not stored in a file.