676165 - Flag SQL statements with non-static strings

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Description

[Kris Maglione [:kmag]]

SQL injection is the bane of server-side web development, and now, alas, your client code can suffer too. Given that the preferred method of creating dynamic SQL statements is to use placeholders, I think the following methods should be flagged if they're called using non-static strings:

   createStatement
   createAsyncStatement
   executeSimpleSQL

They may as well be flagged as universal methods. The names are fairly unambiguous, and there are too many ways to get and store a database connection.

The message should be something along the lines of:

Dynamically creating SQL statements is a common cause of errors, including SQL injection vulnerabilities. Please use placeholders instead: https://developer.mozilla.org/en/storage#section_8

Comment 1

[Wil Clouser [:clouserw]]

jorgev for decisions/wording.  Please give to mbasta when finished.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

The warning message looks good, and I agree that we should flag this. I'm not sure how accurate we can be detecting non-static strings, but as long as we cover most cases, it should be a win.

Comment 3

[Wil Clouser [:clouserw]]

Matt, is this possible?

Comment 4

[Matt Basta [:basta]]

We already do something similar for functions like createElement.

From the docs, I gather that you create a database connection with:

    Services.storage.openDatabase()

Is that the only way?

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

The doc indicates 3 open functions:
https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIStorageService

You can get the service from the Services object, or getting the XPCOM service directly, using getService.

Comment 6

[Kris Maglione [:kmag]]

There are a few builtin services that return them from the DBConnection property.

I suspect it would be easiest and most effective to just implement these as universal methods. Most of the SQL code I see passes the relevant objects as function parameters.

Comment 7

[Matt Basta [:basta]]

I feel dirty implementing them as instance actions/properties, because as we add more and more of them, it becomes more and more likely that add-on developers will start seeing false positives because of unfortunately named functions.

Comment 8

[Kris Maglione [:kmag]]

I'd rather deal with that problem when we come to it. The names are sufficiently distinct I don't expect false positives to be nearly as much of an issue as false negatives from trying to be too smart about it.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...

Comment 10

[Kris Maglione [:kmag]]

https://github.com/mozilla/amo-validator/pull/256

Comment 11

[Andy McKay]

This pull request was merged.

https://github.com/mozilla/amo-validator/pull/256