Head-Slapping Drupal Security Moments

Today I learned an important lesson in the realm of Drupal module development, namely a lesson in what I will refer to as: coder complacency, but this lesson could just as well be called a lesson in coder humility. Having earned my daily bread as a web developer for nearly 8 years now and having been a Drupal module developer for the better part of 5 years one could say I am comfortable around code. But, in code land – just as in many other aspects of life – with comfort can also come peril. When we are comfortable and complacent we drop our guard and lower our defenses.

For a coder, this can turn out to be a consistent, albeit natural, challenge. A good coder is a defensive coder and that, to me, means treating each line of code you write with focus and always have security and standards on your mind. Syntactical accuracy, especially for someone who has been coding as long as I, should be second nature but never overlooked. Sometimes, though as you may have inferred, even the most experienced of coders can make basic mistakes that cause them great grief. Our penance is proportional to the amount of time we have to spend hunting down our bug and the lesson is illuminated in all its glory when you realize that bug was something as sophomoric as accidentally using an assignment operator when you should have used a comparison operator.

Today's experience took that little bug one step further. Because of my syntactical oversight, I had inadvertently introduced a serious security flaw into my module. It's one thing to goof and use an assignment operator when you meant to be comparing, oh say, two numbers or if a variable is true or false. It's another thing when you meant to be comparing user ids. With global $user. Compared to a user id retrieved from the URL. Do you see where I'm going with this?

Well, let's see some code then. Here are the business requirements (theirs, not mine - for the record): Authenticated users can create a node. Once this node has been created, our module generates a unique URL and presents this to the node's author. Here's the catch: The node's author is never to have access to the URL again that was generated for them, however, the generated URL contains the author's user name and the authored node id to facilitate a menu callback in our module that displays a custom form based on the information contained in the URL.

Oh, it gets better.

Other authenticated users (any ol' authenticated user) can access the URL that was generated. Just not the original author. Still with me? Good. This will probably make more sense in code. So, first of all, I'll spare you all the code that leads up to the generation of the URL as it is irrelevant for this article (though I may revisit this module in another article). We are only concerned with the menu callback function that handles what happens when a user attempts to visit the generated URL. First our hook_menu():

<?php
function example_menu() {
  $items['some-path/%/%'] = array(
    'page callback' => 'example_show_formpage',
    'page arguments' => array(1,2),
    'access arguments' => array('access content'),
  );

  return $items;
}
?>

Pretty straightforward. We have wildcards in our path that we will use as arguments in our callback function "example_show_formpage". The first wildcard refers to the user id of the node's author (which isn't used for the comparison to global $user, but is used elsewhere in the function), and the second wildcard is the node id. Here's an abbreviated version of my callback function:

<?php
function example_show_formpage($uid = NULL, $nid = NULL) {
  if (empty($uid) || empty($nid) || !is_numeric($uid) || !is_numeric($nid)) {
    return FALSE;
  }

  global $user;
  $node = node_load($nid);

  // make sure the arguments passed validate
  // then check if the current user is this node's author
  if (($uid == $node->uid) && ($user->uid = $node->uid)) {
    // this is the node's author.
    // show them a message telling them they can't view this page.
  }
  else {
    // this is not the node's author. they can see and do stuff.
  }
}
?>

Did you spot the bug? Good. I didn't at first and it was a very surreal 15 minutes of repeatedly attempting to access the URL as an anonymous user (for testing) only to be suddenly logged in as that node's author. I cleared cache, I checked my hook_menu(), checked permissions, etc… until suddenly it sat there glaring at me right in the face. Yes, I assigned the currently logged in (or not logged in) user's uid to be equal to the node author's uid.

Bad mojo. Bad user-spoofing-from-the-url mojo.

Now, I will disclaim that this was spotted early on in this module's development. Re-factoring of anything hadn't occurred yet and I was just jumping in trying to rough out the business requirements. But I digress. Once the bug was spotted I did find myself amused (and relieved) at first followed quickly by a forehead-slapping moment when I realized I could have just negated my logic and accomplished the same thing and probably avoided such a silly – yet potentially costly – typo. Such is life.

Thankfully, all was resolved during testing (which is why we test!) and not something discovered by the client (or worse) post-launch. Whew! If I had to sum up the takeaway from my lesson today it would be to always check the basics first when you encounter those seemingly absurd or unexplainable bugs. When all other things seem to check out the answer may just be one of syntax.

Occam's razor FTW.

Until next time, happy coding!