Snippet security flaws

From MODx Wiki

By writing Snippets and Plugins you can freely adapt MODx to the functionality you need. However, be aware that your Snippet code may add potential security vulnerabilities to your website. This page aims to provide some tips on how to avoid common security flaws in those PHP scripts.

All input is evil. Never use any user input without having sanitized it before. In PHP user data may come through the variables $_GET, $_POST, $_COOKIE and $_REQUEST. Also some items of the $_SERVER array contain data the user may have altered (e.g. $_SERVER['HTTP_USER_AGENT'] and $_SERVER['HTTP_REFERER']). Even the widely used $_SERVER['PHP_SELF'] can be easily changed by the user and opens a vulnerability for a XSS (Cross Site Scripting) attack if not filtered properly.

Which filtering techniques to use depends on how the user submitted data is used in your script:

Contents 1 Browser display 2 Filenames 3 Emails 4 SQL

Browser display

If you're going to display any user submitted data to the browser, always use [htmlspecialchars] on it, to get rid of common XSS (Cross Site Scripting) attacks:

<?php
// $_GET['userdata'] = '<script>alert("XSS!");</script>';
echo htmlspecialchars($_GET['userdata'], ENT_QUOTES); // &lt;script&gt;alert(&quot;XSS!&quot;);&lt;/script&gt;
?>

Filenames

In case you're going to allow user input as part of a filename (you further open for reading or writing) thoroughly check that it won't be possible for an attacker to dive into the server's filesystem. Note that the following example is in no way secure, even it's more ore less popular. It allows to include any file, the webserver has read-access to (not limited to *.inc.php).

<?php
// DON'T USE THIS!
include './includes/' . $_GET['myinclude'] . '.inc.php';
?>

Better use [preg_replace] and strip all unwanted characters. In the example below, only lowercase letters and digits are allowed.

<?php
// Good practice
$myinclude = preg_replace('/[^a-z0-9]/', '', $_GET['myinclude']);
include './includes/' . $myinclude . '.inc.php';
?>

Or even better: Don't allow the user to change the filename at all.

Emails

A popular way to send Spam is to exploit form mail scripts that allow the attacker to add additional mail headers (e.g. thousands of BCC addresses). To avoid this, your mail calls should never allow user submitted CR/LF characters going into mail headers.

<?php
$recipient = 'nobody@foo.bar';
$subject = 'FormMail: '.$_POST['subject'];
$message = $_POST['message'];
$header = 'From: webmaster@foo.bar' . "\r\n" . 'Reply-To: ' . $_POST['mailsender'] . "\r\n":
if(strpos($_POST['subject'].$_POST['mailsender'],"\r")!==false) die('Possible header injection.');
if(strpos($_POST['subject'].$_POST['mailsender'],"\n")!==false) die('Possible header injection.');
mail($recipient, $subject, $message, $header);
?>

SQL

In MODx it's best practice to use prepared statements when interacting with MySQL via PHP. Here's one recap of the process: Preparing MySQL Statements

In the very least, you should prepare user data for SQL statements by applying $modx->db->escape. This way you can help avoid attacks called "SQL injection".

<?php
$sql = 'SELECT longtitle FROM site_content WHERE pagetitle = \'' . $modx->db->escape($_GET['q']) . '\' LIMIT 15';
$result = $modx->query($sql);
?>

But notice how this latter technique assembles the string from user-supplied variables. If someone hijacks your string, they can hijack your SQL statement, and you could be in big trouble. Refer to Preparing MySQL Statements for using prepared statements, which separate user-input from the statement.

For more security considerations specific to MODx, refer to the MODx Security page.