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
#markupproperty 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
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()
@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
Vulnerable to open redirect attacks:
<?phpdrupal_goto($_GET['random_user_supplied_value']);
?>
Safe, because $_GET['destination'] is sanitized by Drupal core (7.35 and above):
<?phpdrupal_goto($_GET['destination']);
?>
Safe against open redirect attacks because we only allow relative URL paths:
<?phpif (!url_is_external($_GET['random_user_supplied_value'])) {
drupal_goto($_GET['random_user_supplied_value']);
}
else {
drupal_goto('<front>');
}
?>
Good idea to collect some
Good idea to collect some points about security reviews, can we do this as a wiki page so that everyone can edit and improve?
I wikified this node :)
I wikified this node :)
knaddison blog | Morris Animal Foundation
:)
Thank you!
I will update this node with some details.