0.9.6.2 security problem with reflect snippets

You’re missing the point entirely, I’m afraid. This is not an input sanitization issue. This code is never supposed to receive input from the user at all, and certainly not from the query string.

This file should not exist as a .php file on the server. It is intended to be copied into the database as a MODx snippet. The only way that any value should be given to the $reflect_base variable is as a parameter of this snippet, which is run in a limited (non-global) scope and controlled environment with plenty of hardening code surrounding it.

The root problem absolutely is register_globals being set to ON. That is the only way that this file or any other like it could be exploited. There’s no reason to harden this specific file to resist attacks on its own, since it’s only a reference file and should simply be deleted or renamed with a .txt extension.

It’s not supposed to, yet it did. Famous last words, I’m sorry to say.

I suggest you do a review of your methods, if code like this was in that file, I assume there’s more hidden all over the place.

It’s not just the sanitization question, it’s allowing code like this in at all, in the first place, which does nothing but hide errors.

So now one file was discovered, are there others? Do I have to check all the code to make sure this isn’t repeatable?

Again, relying on server settings that are not under your control rather than making your code secure, which is under your control, guarantees hacks for the life of your software.

This reminds me quite a bit of phpbb in around 2000-2003, when its holes were well known, but it was simply considered bad form to hack it, since it was so easy to do, and was also open source, which got it some relief. Same excuses, by the way, from the phpbb group, re its security issues.

Here’s a phpbb sample, this is the result of a few years of learning why you don’t do what you’re advocating, from phpbb:



Note a few things there? the root path is set explicitly, or using ./, not implicitly with something that could be null, this can’t be null. So it’s impossible to inject anything in there, period.

I’m not saying phpbb is a paragon of good programming or secure software, it isn’t, but they have learned, painfully, why you don’t rely on luck or implicit values for untested variables.

Quote from: lizardx at Dec 09, 2008, 09:42 PM

I suggest you do a review of your methods, if code like this was in that file, I assume there’s more hidden all over the place.

I agree about reviewing the method of including sample snippet code as .php files, since they should never be executed that way. But the solution is as simple as renaming any such files with a .txt extension. When this code is executed in its proper context, the code that you suggest would be redundant and unnecessary.

Quote from: lizardx at Dec 09, 2008, 09:42 PM

It’s not just the sanitization question, it’s allowing code like this in at all, in the first place, which does nothing but hide errors.

Ternary operators don’t "hide" anything; they just make your code more concise. Some people like them, some people don’t. But they’re no different than an if/else statement.

Quote from: lizardx at Dec 09, 2008, 09:42 PM

Again, relying on server settings that are not under your control rather than making your code secure, which is under your control, guarantees hacks for the life of your software.

Again, you’re missing the point here. This code is not a part of the MODx system (it’s not an file included by some other script), and it is not insecure when run in its proper context (as a snippet stored in the database). As long as it is not uploaded as an executable .php file, it is not a security risk as is.

So on that point we agree: It should not have been included in the MODx package with a .php extension. The solution is not, however, to harden this file so that it can withstand attacks in a context that it was never intended to be used and on an insecure server. The solution is as simple as renaming or deleting the file.

Note that I’m not saying that the file should be left as is; it should be renamed or deleted.

I don’t think that any major PHP software should ever be run with register_globals set to ON. There’s no realistic way to audit every line of code for possible XSS vulnerabilities in a system this complex given that quite a number of code libraries written by other teams are integrated into it and updated frequently. If there’s one possible exploit in one included file in the latest version of TinyMCE, for example (not to give them a bad name, of course), then your whole system can be compromised.

Leaving register_globals set to ON is as irresponsible as setting the permissions of PHP files to 777, and setting it to OFF protects you from this entire class of attacks.

So there are two proactive lessons here from my point of view: Don’t distribute code samples that are not meant to be executed as PHP files with a .php extension, and make the warnings about register_globals being set to ON much more dramatic so that people take them seriously (or perhaps even disable the system until it’s corrected).

I don’t know if it was posted but here’s another solution

put this in .htaccess

AddType x-mapp-php5 .php

This will force all php files to be treated as php5 which has register globals off by default

xpix I’ve seen many (including my shared server) where the host has turned register_globals on globally so this would not work. They originally did it to allow a smoother transition to PHP5 when they forced upgrade between 4 and 5. After a few months on PHP5 they globally turned register_globals off at my suggestion. I think there are hosts out there who would rather not deal with clients whose sites/apps break than having a secure server. If this is your host, find another.

That said it may work for many.

Cheers,

Jay

I just started with 1and1 and I guess I didn’t care about register_globals since I’m not coding for this.
Since modx is the only way for me I am glad that a warning was added in the manager.

Do you seriously suggest changing host just because of this?

10000% ... just like I would suggest you change day care if one left small bowls of anti-freeze laying around, razor blades all over the changing table, loaded pistols laying about, and an ample supply of matches and rusty nails tossed in for good measure. Oh, and the staff were all convicted child molesters.

Granted it’s not a guarantee something bad would happen, but I sure as hell wouldn’t send my child there.

If your host doesn’t care about security they don’t really care about you. Hosts have lax security for one of two reasons: They don’t know what they are doing; they don’t want to deal with telling clients to fix their apps.

In either case it’s time to walk away.

Sorry to bother you guys but I need to understand if by adding "AddType x-mapp-php5 .php" is everything ok or not?

You’re making it sound like is a big issues even with register globals off.