We launched new forums in March 2019—join us there. In a hurry for help with your website? Get Help Now!
    • 28042 ☆ 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.
      Studying MODX in the desert - http://sottwell.com
      Tips and Tricks from the MODX Forums and Slack Channels - http://modxcookbook.com
      Join the Slack Community - http://modx.org
      • 33372
      • 1,611 Posts
      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.
        "Things are not what they appear to be; nor are they otherwise." - Buddha

        "Well, gee, Buddha - that wasn't very helpful..." - ZAP

        Useful MODx links: documentation | wiki | forum guidelines | bugs & requests | info you should include with your post | commercial support options
        • 20289
        • 958 Posts
        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.
          [img]http://i10.tinypic.com/52c4eir.gif[/img][/td]
          [td][Wiki] [Persian support forum]
          [SVN] [RTL SVN Branch] [bugs] [FishEye+Crucible] [Learn MODx!] | [My Google Code]
          [font=tahoma][برای دسترسی به راهنمای فارسی به [url=http://www.modxcms.ir]
          • 23491 ☆ 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.
            Mike Reid - www.pixelchutes.com
            MODx Ambassador / Contributor
            [Module] MultiMedia Manager / [Module] SiteSearch / [Snippet] DocPassword / [Plugin] EditArea / We support FoxyCart
            ________________________________
            Where every pixel matters.
            • 19033
            • 892 Posts
            Dear all,

            Is patch which will correct by you those vulnerabilities offered?
            Especially, the patch for "Search Highlighting" should be offered, we (Japanese community)think.
            Because this vulnerabilities(XSS) influences even the user who visits the Web site.

            For instance, the direct mail of the shopping site has been sent.
            http://shopping-siteA.com/?searched=test&highlight="><IMG%20SRC="javascript:location.href='http://sham-site.com/';


            User might trust this URL because user sees only the domain.
            And, the password might be input to the imitation site.

            We announced the Japanese users two methods.
            http://modxcms.com/forums/index.php/topic,21302.msg139848.html#msg139848 (in Japanese).

            One is the correction of a code of "Search Highlighting".
            $searched = strip_tags(urldecode($_GET['searched']));
            $highlight = strip_tags(urldecode($_GET['highlight']));


            Another, disables Search-Highlighting-plugin.
              • 25663 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
                • 33372
                • 1,611 Posts
                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.
                  "Things are not what they appear to be; nor are they otherwise." - Buddha

                  "Well, gee, Buddha - that wasn&#39;t very helpful..." - ZAP

                  Useful MODx links: documentation | wiki | forum guidelines | bugs & requests | info you should include with your post | commercial support options
                  • 19033
                  • 892 Posts
                  Dear all,

                  Thank you, rthrash and ZAP.
                  We thank your fast response very much.

                  I also updated announcement against Japanese users.

                  Sincerely yours,
                  MEGU
                    • 23754
                    • 13 Posts
                    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
                      • 33372
                      • 1,611 Posts
                      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.
                        "Things are not what they appear to be; nor are they otherwise." - Buddha

                        "Well, gee, Buddha - that wasn&#39;t very helpful..." - ZAP

                        Useful MODx links: documentation | wiki | forum guidelines | bugs & requests | info you should include with your post | commercial support options