Security checklist for developers

From MediaWiki.org
Jump to: navigation, search

This document is provided as a supplement to Security for developers. This is a list of common development tasks, and the security measures that need to be taken.

Security checklist[edit]

If you are working with ... have you ...

Cookies[edit]

  • reduced reviewer anxiety by using $wgRequest instead of $_COOKIE?
    • fetched cookies using $wgRequest->getCookie(...)?
    • set cookies using $wgRequest->response()->setcookie(...)?
# Attempt to fetch the UserID cookie value. Note: The 
# value returned isn't trusted and is forced to be an int.
$sId = intval( $wgRequest->getCookie( 'UserID' ) );

Dynamic code generation[edit]

Avoid using functions like eval() and create_function(), as well as the /e pattern modifier for preg_replace(). While powerful and convenient, these features are inherently insecure:[1][2]

  • it's easier to put arbitrary strings into text processed by a regular expressions, which – when combined with the /e pattern modifier – can lead to code injection attacks.
  • it is harder to read and maintain code that is part of a string.
  • static analysis tools won't catch warnings and errors in the code.
  • opcode caches (like APC) can't cache code mixed into strings.
  • create_function() sometimes has garbage-collection issues.
  • A loop which has a create_function() inside will create a new function on each iteration.

Sometimes you really do need these features (obviously eval.php needs to run eval() ;) but in most cases, we'd rather see the function broken out and referred as a callback.

For future code that runs only under PHP 5.3 and later, note that inline lambda functions will make it easier to make your callback inline while retaining the benefits of code that's written in native syntax instead of strings.

  • Anything external that is used in part of regex should be escaped with preg_quote( $externalStr, $delimiter ). It puts a backslash in front of every character that is part of the regular expression syntax, and escapes also the delimiter given as second parameter:
$str = preg_replace( "!" . preg_quote( $externalStr, '!' ) . "!", $replacement, $str );

External programs[edit]

// escape any naughty characters
$cmd = wfEscapeShellArg( $cmd ) . ' --version';  
$err = wfShellExec( $cmd, $retval ); // run $cmd

Forms[edit]

GET data[edit]

  • reduced reviewer anxiety by using $wgRequest instead of $_GET?
# Check if the action parameter is set to 'purge'
if ( $wgRequest->getVal( 'action' ) == 'purge' ) {
    ...

Global variables[edit]

# ensure that the script can't be executed outside of MediaWiki
if ( !defined( 'MEDIAWIKI' ) ) {
    die( 'Not a valid entry point.' );
}

Output (API, CSS, JavaScript, HTML, XML, etc.)[edit]

Any content that MediaWiki generates can be a vector for XSS attacks.

  • used the Html and Xml helper classes?
# rawElement() escapes all attribute values 
# (which, in this case, is provided by $myClass)
echo Html::rawElement( 'p', array( 'class' => $myClass  ) );
  • reduced reviewer anxiety by using ResourceLoader to deliver CSS and JavaScript resources?

User provided CSS[edit]

User provided CSS (Say for use in a style attribute) needs to be sanitized to prevent XSS, as well as to disallow insertion of tracking images (via background-image), etc

  • Use the Sanitizer::checkCss method for any css received from a user, possibly along with the Html class.
# let $CSSFromUser be the user's CSS.
echo Html::rawElement( 'p', array( 'style' => Sanitizer::checkCss( $CSSFromUser ) ) );
  • For CSS provided by the extension (and not the user), this is not needed (and will remove some valid things like background-image:). However, extension provided CSS should go in stylesheets loaded by ResourceLoader, and not in style attributes.

POST data[edit]

  • reduced reviewer anxiety by using $wgRequest instead of $_POST
  • Always validate that any POST data received is what you expect it to be
# Check if the action parameter is set to 'render'
if ( $wgRequest->getVal( 'action' ) == 'render' ) {
    ...

Query strings[edit]

Sessions[edit]

Any user input: no isset![edit]

Any isset() is frowned upon. Use strong comparison like === or !==. An example from SimpleAntiSpam:

	if ( !( $_POST['name'] === "" && $_POST[$bottrop_field_name] === $bottrop_field_value ) ) {
		return( "Stuff" );
	}

See also Register globals#Sanitize custom global variables before use and Manual:Coding conventions/PHP#Pitfalls.

Reviewer anxiety[edit]

  • Clearly added comments to explain unexpected or odd parts of your code?
# $wgRequest isn't yet available.  Forced to use $_GET instead. 
if ( $_GET['setupTestSuite'] !== null ) {
 	$setupTestSuiteName = $_GET['setupTestSuite'];
 	...

SQL queries[edit]


See also[edit]

References[edit]

Coding conventionsManual:Coding conventions
General All languagesManual:Coding conventions#Code structure · Development policyDevelopment policy · Security for developersSecurity for developers · Pre-commit checklistManual:Pre-commit checklist · Performance guidelinesPerformance guidelines(draft) · Style guideDesign/Living style guide · Accessibility guide for developersAccessibility guide for developers(draft)
PHP Code conventionsManual:Coding conventions/PHP · PHPUnit test conventionsManual:PHP unit testing/Writing unit tests#Test_conventions · Security checklist for developersSecurity checklist for developers
JavaScript Code conventionsManual:Coding conventions/JavaScript · Learning JavaScriptLearning JavaScript
CSS Code conventionsManual:Coding conventions/CSS
Database Code conventionsManual:Coding conventions/Database · Database policyDevelopment policy#Database policy
Python Code conventionsManual:Coding conventions/Python
Ruby Code conventionsManual:Coding conventions/Ruby
Selenium/Cucumber Code conventionsManual:Coding conventions/Selenium
Java Code conventionsManual:Coding conventions/Java
API client code Standards for API client librariesAPI:Client code/Gold standard