With PHP CodeSniffer we have a good way to do testing for security vulnerabilities in code. I'm primarily concerned with XSS since it's the most popular vulnerability in Drupal code but SQL injection and others can be scanned for as well.

Here's a few ideas of what to search for:

  • Tracing user input to output without sanitizing or filtering
  • Finding callbacks that update the DB that are not access controlled
  • Incorrect use of dbtng opening up sql injection

These checks will require a better API for following function calls, parameters and assignments. I think the work done in Drupal_Sniffs_Semantics_FunctionCall is the direction needed, extending Sniffs to easily traverse through the token stack to meet these goals.

For example, for the token drupal_get_title in Drupal 6 if the argument is a variable we'd need to traverse back to the assignment of that variable and add an error if it's assigned from user input, like $_GET or $node->title, so long as the stack doesn't contain that variable being filtered or sanitized.

Some questions to start:

  1. What can be detected? Specific use cases are helpful as they can be turned into tests
  2. In what ways could an API help with writing security sniffs? What are the common things that need to be done?

Comments

coltrane’s picture

Status: Active » Needs review
StatusFileSize
new0 bytes

I created http://groups.drupal.org/node/268818 for discussion on common security mistakes.

The attached patch provides a helper class with registry functions for processing functions, variables and output (print and echo for example). The Drupal_Sniffs_Security_DrupalSetMessageSniff() class implements method processFunctionCall() to trace the arguments to user input.

Looking for input on this approach.

coltrane’s picture

StatusFileSize
new23.84 KB

(this time with full patch)

coltrane’s picture

Status: Needs review » Needs work

I'm continuing development on this so #2 is out of date.

Sniff helper functions like the following:

  • isBeingPrinted()
  • isBeingAssigned()
  • isBeingReturned()

are helpful for tracing variables like the following

$nextPtr = $variablePtr;
while (($nextPtr = $sniff->phpcsFile->findNext(PHP_CodeSniffer_Tokens::$emptyTokens, $nextPtr + 1, null, true, $sniff->tokens[$variablePtr]['content'])) !== false) {
// ... check if this variable is printed, re-assigned or returned
}
coltrane’s picture

Component: Code Sniffer » Coder Review

Placed in a sandbox for now: http://drupal.org/sandbox/coltrane/1921926

I'm interested in getting this merged into coder at some point though there are a few reasons it could live on as a separate project, or at least as a separate standard: lots of abstract functions for tracing variable assignment means more complicated code and it's less of a "coding standard".

coltrane’s picture

Component: Coder Review » Coder Sniffer

component should not have changed

klausi’s picture

Status: Needs work » Postponed

Thanks for your work on this, I'm going to experiment with it by using it for project applications in pareview.sh.

Let's mature this in the sandbox for now and decide later if we want to integrate it into coder.

psynaptic’s picture

Issue summary: View changes

Is it time to revisit this? It looks great and would be a terrible shame for this work to be lost to time.

klausi’s picture

Status: Postponed » Closed (won't fix)

Coder 7.x is frozen now and will not receive updates. Coder 8.x-2.x can be used to check code for any Drupal version, Coder 8.x-2.x also supports the phpcbf command to automatically fix conding standard errors. Please check if this issue is still relevant and reopen against that version if necessary.