CMS Made Simple Security

[sjg]

Well, we've had a few security issues noted on BugTraq, which tells me two things:

• We're hitting the Big Time, in terms of usage
• We need to make sure we're doing things right

I'm going to do a preliminary audit of the codebase. Here's what I plan to do. I invite feedback, suggestions, criticism, etc.

• I'm creating a list of every reference to $_GET and $_POST in the core and core modules, and checking that they're not used in a way that could be exploited through a parameter-injection attack.
• I'm going through every call to $db->Execute or $db->SelectLimit, and making sure all query strings are properly parameterized to avoid SQL-injection attacks.
• I'll be looking at every admin page to confirm that it calls check_login() to prevent any unprivileged users from gaining access.

I suspect that this will be a fairly time-consuming and tedious task, and I also think I'll find few, if any, problems.

I think there may need to be similar audits on the use of the $param hash, but I'm not sure exactly what will need to be done there.

In any case, I'll report back here. Again, I invite suggestions!

Thanks,
___Samuel___

[Ted]

This is good news.  I wish I had time to do the same, but it's SO limited these days...

I agree that I think we're starting to hit a certain level of adoption where it's becoming very important to do a secuirty audit.  If anything is found, some rules have to be put in place for the future.  Or, at least some common gotchas to look for.

Yesterday I was stressing about this pretty badly. But then it's been pointed out to me on many occasions in the last 24 hours that it could be worse.  At least it's an app that people are using and worth taking the time to look at every line of code...

We're also on the eve of some very big (and positive) changes and it's best to get this out of way before it could get any more out of hand.

Oh, and Samuel...  you're the man! 

[calguy1000]

Hi SjG

I'm going through every call to $db->Execute or $db->SelectLimit, and making sure all query strings are properly parameterized to avoid SQL-injection attacks.

A Question.... what do you mean by parameterized?

Also, a suggestion

maybe modifying the Skeleton module with comments in the appropriate locations indicating how things should be done and why with respect to security would really help.  it would help me.

[sjg]

what do you mean by parameterized?

Well, I guess I really mean "properly escaped," although my prefered way of doing that is through parameterization.

For example, this is potentially dangerous:

Code: Select all

$query= "SELECT * from ".cms_db_prefix()."table WHERE field=$var1";
$result = $db->Execute($query);

because $var1 could, say, contain the value "1;delete from cms_users;"

Then the Execute statement would actually execute two SQL statements, do the select, and blow away the user table.

The safer way to do it would be to parameterize the query:

Code: Select all

$query= "SELECT * from ".cms_db_prefix()."table WHERE field=?";
$result = $db->Execute($query,array($var1));

In that case, ADODB escapes $var1, and it should theoretically be safe. At least in the Java world, this approach is better than the equivalent in-line escaping:

Code: Select all

$query= "SELECT * from ".cms_db_prefix()."table WHERE field=".$db->qstr($var1);
$result = $db->Execute($query);

because the database can cache the query. I don't know if that holds true in PHP, or if ADODB does anything intelligent with parameterized queries.

Thanks for the suggestion on the Skeleton module. I think it would be a good idea to add some text on the subject of security. I'll add that to my list.

[sjg]

OK. Here's my status report for today.

I've been reading over the entire codebase. I've looked at many files, but not all of them. The following lists what I have not yet looked at, or code that was too complex for me to understand, or code that I thought was an external library and therefore someone else's problem. I know. Bad attitude. Sue me. Actually, don't. Anyway, these have not been checked:

lib/filemanager/*
lib/smarty/*
lib/xmlrpc.functions.php
lib/adodb/*
modules/*

Assumptions

As I looked at the codebase, I had to make some assumptions. I don't have the time to go into excruciating detail and follow the exact flow of every piece of code. As I mentioned before, I'm looking for specific things which are known to be risky or are things that I think could be risky. This is a "first cut" audit, and should not be considered authoritative (I don't consider myself qualified to do an authoritative audit, especially in just a few days). My assumptions, listed below, are all open to discussion and evaluation:

1. If you install a malicious modules, all security bets are off.
2. If you're an Admin, you're not necessarily any more trustworthy than anyone else. The privileges you are granted by the "root" admin determine how much trust you're given. But we can't trust admins not to try to escalate their privileges.
3. It's not possible to hijack or place items into the $_SESSION unless you're already trusted enough to be doing code-like things (e.g., user tags or installing modules).
4. The session id stuff works, and session-replay won't get you anywhere. This is probably a bad assumption.


Things I patched:

I made a bunch of minor changes to database queries, where unescaped GET values were being passed directly to the database. In fact, I also escaped values that only in wildly implausible situations could get replaced by something evil. I figure better safe than sorry.

I made a bunch of probably unncessary changes where I sanitized output messages (or at least removed "<" characters from them) so that people couldn't craft trick URLs that could lead to cross-site information leaking scripts.

Things That Made Me Feel Insecure For No Good Reason:

These are things I didn't change, but thought should be discussed.

/admin/themes/default/login.php -- line 39
The user's POSTed password is used to prepopulate the password input field. While I don't think this is much risk, I don't see it providing much value either, because you only would have a value if the username or password is incorrect upon login. I think it should probably be removed.

/lib/page.functions.php -- line 810
This is a strange query, which I don't quite understand. What's with those backticks? Anyway, it's conceivable to me that someone could construct a post that got to this code with evil values in username or password, and do a sql-injection attack.

The following files all, under certain conditions, do a redirect that has values in the URL that can be set by the incoming request (e.g., a POST value). What that means is that a malicious person could craft a form that would then post those values on to another redirected URL (for a logged in admin). There may be a good XSS exploit here:
admin/addcssassoc.php
admin/addtemplateassoc.php
admin/deletecssassoc.php
admin/deletetemplateassoc.php
admin/listmodules.php

Whew. Doesn't seem like much. I think we're on the right track...
___Samuel___

[Nevermind]

speaking of externel libs lets get back to reason 10.2 was released they patched the wrong file they should have patched images.php it is used to upload images and uses headers to confirm file type therefore php can be used in images

[martin42]

It's reassuring that the current CMSMS design allows you to restrict the public webserver so that it can't see most of the CMSMS scripts anyway.
On my site (which admittedly has fairly low functionality!) the only files that seem to be needed are:-

    /index.php
    /stylesheet.php
    /uploads
    /images

No other files need be readable by the webserver, though lots of others need to be readable by PHP (notably not /admin though). All good!
If using CSSMenu, then the files CSSMenu.js and arrow.gif will need to be moved into /images (say), and the CSSMenu code adjusted accordingly.

It would be great if future versions of CMSMS could consider separating more clearly the stuff that needs to be accessible by:

1. the public web server instance
2. the public PHP instance
3. the admin's web server instance
4. the admin's PHP instance

But maybe this is just too much of a pain to implement!  But as a minimum, it would be nice to avoid mixing things up, so that separation into public-visible + admin-visible stuff remains possible in future releases.

[martin42]

Stepping back and thinking some more, a more radical approach would be possible for simpler websites!

If truly dynamic functionality isn't needed on a particular website, then maybe the CMSMS authoring functionality could (using a config.php option setting) simply update static content files in a specified folder. 

In such cases, the public web site view would not need to be PHP-enabled. It would only need to be able to see the static content.

Possibly, in this case, the menu list could come from a Javascript include file so that CMSMS would not need to  update all the static pages every time a new bit of content is added, and so that the web clients wouldn't need to download the menu list inside every content page as at present.

Sorry if this is too off-topic!

[Pierre M.]

Many thanks sjg for the security audit.
Martin42, please see http://forum.cmsmadesimple.org/index.ph ... 678.0.html my previous post about serving static content : I think a little diff could avoid the audit of all the caching code.
It is a very good idea to harden CMSms.

PM

[Pierre M.]

Many thanks sjg for the security audit.
Martin42, please see http://forum.cmsmadesimple.org/index.ph ... 678.0.html my previous post about serving static content : I think a little diff could avoid the audit of all the caching code.
It is a very good idea to harden CMSms.

PM

[iNSiPiD]

I think once theses directories are properly segregated that adding them to a default .htaccess file would be a great thing.

Most users don't need/want to think about this and shouldn't need to. It's a simple enough thing to implement, no?