Security issue in CakePHP code documentation

I have been using CakePHP for a long time now and enjoy every second. It provides a productive, easy to use and well document platform for PHP application. The key advantages for me are – transparent OR mapping, a strong Model View Controller framework, and tons of extra utilities that make your life better.

I have came across a possible security issue in one of cakePHP code examples. This section of code is responsible to authorize or un-authorize clients access to a certain action (MVC flow)

action == 'delete') {
            if ($this->Auth->user('role') == 'admin') {
                return true;
            } else {
                return false;
            }
        }

        return true;
    }
?>

The major security rule this code is breaking is – never ever have ‘return true’ as a default for an authorization method.

Security driven operations should always return “unauthorized” as a default value. You should always take the white-list-approach to authorization methods, in other words you want to allow privileges to specific scenarios and block all other scenarios by default.

This is how this method should be implemented:

action == 'delete') {
            if ($this->Auth->user('role') == 'admin') {
                return true;
            }
        }
	if ($this->action == 'view') {
                return true;
        }
	...
        return false;
    }
?>

A good way to prove this is a safer implementation would be to think what will happen if we add a new action (let’s call it “edit”) and let’s say we forget to rewrite the isAuthorized method.

  • Original implementation: The ‘edit’ method would be accessible to any role in the system and the developer would have hard time discovering that he/she forgot to rewrite the isAuthorized method. This security hole might never be found until it is too late.
  • New implementation: The ‘edit’ method will not be accessible to any role and the developer would immediately know that there is something he forgot to do.

I have submitted the correction to the official CakePHP tutorial and hopefully they will amend the reference code shortly.

Share the love...Tweet about this on TwitterShare on LinkedInShare on Google+Share on Facebook

Amir Shevat

Amir Shevat is the global Startup Outreach lead in Google Developer Relations (g.co/launch). Previously, Amir Led Google Campus Tel Aviv and was the co founder of several startups.

You may also like...

5 Responses

  1. anonymous says:

    That’s not the case here though. It seems the intention of the example is to only delineate access when performing deletes and that the act of deleting should only be performed by “admin” users.

  2. Amir Shevat says:

    But the issue is that the isAuthorized method protects all methods in a controller – not only the delete function.

    Look at the code of the isAuthorized that handles the delete function. It checks if you are an admin in defaults to unauthorized if you are not admin – this should be the case for all functions

    Even if you only want to protect a single method right now you should explicitly allow access methods and return unauthorized by default. You simply cannot return authorized by default.

  3. anonymous says:

    So? That’s the point of the AuthComponent in the first place. The developer in the example has decided that the only action requiring escalated privileges (aside from already being logged in) is the delete function; they’ve decided that the others are available to authenticated users.

    Furthermore, why should it be the case for all functions? You’re applying your requirements on an example, you don’t know their needs or requirements, it’s just an example.

  4. Amir Shevat says:

    The whole point of examples is to demonstrate best practices, developers use the example to develop their application and rarely diverge from the example.
    My point in this post is that an authorization method that returns “authorised” by default is not best practice.

    Look at how auth->allow() works in cakePHP examples:

    $this->Auth->allow(‘foo’, ‘bar’, ‘baz’);

    It works in a white list fashion – you specify the methods you want to provide public access to, and all the rest are blocked by default.

    The isAuthorized methods should demonstrate the same consistency and use a white list to allow privileges to specific roles and block all the rest.

    Security examples should drive security best practices and be secure by default.

  5. anonymous says:

    Totally agree, developers will see this example and will probably write less secure code as a result. isAuthorized should be written like Auth->allow(…)

Leave a Reply

Your email address will not be published. Required fields are marked *