[D7] Security review check list?

You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!

Posted by Ayesh on June 3, 2015 at 3:37pm
Last updated by naveenvalecha on Thu, 2016-02-18 09:21

First of all, I would like to thank every one of you for the welcome and support for me to become a git admin.

While there is a neat documentation for the review process itself, I think it would be easier if we have a check list of security reviews. Automated tests can detect printing $_GET vars, etc, but there is a pattern of security issues in many applications.

I'm collecting points to write a wiki page. So far, I have written down these. If you have any suggestions of a repeated security issue pattern, please share!

access callback set to TRUE for admin paths, and in general is a orange flag that something may be amiss
Accessing $_GET, $_POST $_REQUEST usually signals something is probably insecure.
Renderable arrays, with #markup property set to a variable.
print variable_get('something')
String concatenation in SQL queries instead of placeholders
db_query("SELECT * FROM {node}")... (node_access)
drupal_goto() with a variable (open redirect)
Lack of proper checks in a hook_file_download()
Directly outputting user-supplied entity values without sanitization.
Modifying data via a GET request w/o a token (CRSF)
Using $form_state['input'] instead of $form_state['values'] is a red flag

I will write an example case and a penetration test for each above. If you have any other suggestions or something you look out naturally, please let us know.

Comments

I was searching for something

Posted by Pravin Ajaaz on June 3, 2015 at 4:37pm

I was searching for something like this. This is good work. If other senior reviewers add things to this. It will be super informative. :)

drupal_goto()

Posted by chenderson on June 4, 2015 at 10:35am

@Ayesh thanks for this.

I have just been looking into drupal_goto() and its potential security issues and have a few questions around it (sorry if this is not the place to post).

In 7.35 their was a change which means external URLs can no longer be passed through the "destination" query parameter. From what I can see this would make using drupal_goto() with drupal_get_destination() safe.

I assume this is which you mention drupal_goto() with a variable specifically?

What would be the best practice when developing modules now? Should their still be checks to prevent the abuse of drupal_goto() with drupal_get_destination() or is it fair to assume people on older versions must take the risk if they choose not up update?

Vulnerable to open redirect

Posted by klausi on June 4, 2015 at 10:50am

Vulnerable to open redirect attacks:

<?php
drupal_goto($_GET['random_user_supplied_value']);
?>

Safe, because $_GET['destination'] is sanitized by Drupal core (7.35 and above):

<?php
drupal_goto($_GET['destination']);
?>

Safe against open redirect attacks because we only allow relative URL paths:

<?php
if (!url_is_external($_GET['random_user_supplied_value'])) {
  drupal_goto($_GET['random_user_supplied_value']);
}
else {
  drupal_goto('<front>');
}
?>