405920 - audit all places queries for sql injection potential

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

* Enumerate and document all sources of data added into places.sqlite.
* Formalized audit of code path of input data, confirm it's using parameter binding instead of executing raw SQL.

Comment 1

[Brandon Sterne (:bsterne)]

Since this is a dependency of bug 375898, marking P2 so it stays on our radar for Fx3 triage/tracking.

Comment 2

[Shawn Wilsher :sdwilsh]

not sure if I have cycles to help with this - I'm a wee bit swamped.

Comment 3

[Ondrej Brablc]

Attachment: Script for analysis of SQL statements

This script extracts SQL statements from C++ source code. In case when the statement contains % or consists of multiple added strings (using +) or in case when it is not a constant string, it will be printed with the command line. -a parameter forces the program to print all SQL statements for deeper review.

The idea is that we should run this tool before each release and keep historical results. By comparing the current results with previous release we will see what SQL statements have been modified or added and the next audit should be very short.

Comment 4

[Ondrej Brablc]

Attachment: A shell script to analyze the whole tree

I yet have to make analysis of the results produced by this tool for the first time.

Comment 5

[Ondrej Brablc]

Attachment: List of all found sql stmts with highlighted potentially dangerous ones

I have fixed couple of bugs in the parser and added HTML output. This list contains all found statements and highlights potentially dangerous ones. My task is to get through all the highlighted and check whether they are safe. If you feel some files are missing or as module owner you know that the tool did not detect your queries properly, please let me know.

Comment 6

[Ondrej Brablc]

I have lost some time on this code, may be someone can help:

./netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
1257       PR_smprintf("DELETE FROM moz_cache WHERE ClientID=%q AND Flags=0;",
1258                   clientID);

%q is non standard, I have found that it is an sqlite extension, however this requires opening and closing quote:

char *zSQL = sqlite3_mprintf("INSERT INTO table VALUES('%q')", zText);

So I basically think, that the code above leads to invalid SQL statement.

Comment 7

[Shawn Wilsher :sdwilsh]

what do you mean by %q is non-standard?  It's being consumed by the sprintf, right?

Comment 8

[Ondrej Brablc]

(In reply to comment #7)
> what do you mean by %q is non-standard?  It's being consumed by the sprintf,
> right?

C99 says: If a conversion specification is invalid, the behavior is undefined. 
%q is not valid conversion specification of C99. In case of PR_smprintf, %q is ignored (as I have tested). So we have an invalid SQL statement. Do I make mistake somewhere, or should I file a bug?

Comment 9

[Shawn Wilsher :sdwilsh]

ah - gotcha.  So, that statement should probably be fixed (and clearly isn't being unit tested).

Comment 10

[Ondrej Brablc]

Attachment: Auditing scripts and results for the tree

I have finished audit of all the SQL statements. We have SQL injections problems from locales - addressed in bug 420354. And we have an invalid SQL statement to be fixed with bug 423402.

I'm not sure where and how to put created scripts and results for later comparison. So I decided to put them into storage/test/audit. Please let me know if there is better place.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 310003 [details] [diff] [review]
Auditing scripts and results for the tree

you'll need a storage peer to look at this too

maybe we could get this to run on checkin with make-check and go orange if something bad comes up?  that would be *really* cool actually.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 310003 [details] [diff] [review]
Auditing scripts and results for the tree

looks fine to me. however, the log shouldn't be checked in, is fine to just leave that attached to the bug.

integrating this into tinderbox would be great, but is definitely not a blocker. please file a followup bug for that.

given that the only risks found are in separate bugs, we can close this out after you get r+ on the audit script from a storage peer and this gets checked in.

Comment 13

[Ondrej Brablc]

Attachment: Patch without the log file

Comment 14

[Ondrej Brablc]

I have filed the tinderbox follow up bug 424408.

Comment 15

[Shawn Wilsher :sdwilsh]

Renoming for blocking.  I don't think this should block anymore - issues found by the script were filed as follow-up bugs.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 311018 [details] [diff] [review]
Patch without the log file

I actually don't know much of anything about perl - but I think this looks right.  If this were python, I'd by much better off :/

Any chance you could get this to look at JS too, or no?  I realize that that would be a lot more difficult, but I also know we keep moving more and more to JS.

Comment 17

[Ondrej Brablc]

Attachment: Support for .js audit added

(In reply to comment #16)
> (From update of attachment 311018 [details] [diff] [review])
> I actually don't know much of anything about perl - but I think this looks
> right.  If this were python, I'd by much better off :/

Shawn, if you do not feel comfortable doing review for perl file, can you suggest some other peer that can do it for storage module? I would not like to try one after another.

> Any chance you could get this to look at JS too, or no?  I realize that that
> would be a lot more difficult, but I also know we keep moving more and more to
> JS.

I have added simple analysis of .js files. There were only four .js files found outside of test directories:

./browser/components/search/nsSearchService.js
./extensions/sql/sqltest/sqltest.js
./toolkit/components/contentprefs/src/nsContentPrefService.js
./toolkit/mozapps/downloads/content/downloads.js

sqltest.js contains two vulnerable places, should I file a bug, or is this file part of some test too?

http://mxr.mozilla.org/firefox/source/extensions/sql/sqltest/sqltest.js#111
http://mxr.mozilla.org/firefox/source/extensions/sql/sqltest/sqltest.js#131

Are there more .js files that use SQL and that the tool has missed?

Comment 18

[Shawn Wilsher :sdwilsh]

Maybe bsmedberg for the perl stuff.

That might be correct for what is in js - I'm not 100% sure myself.

I'm unsure if the sqltest files are even maintained anymore.

Comment 19

[Benjamin Smedberg]

Comment on attachment 312784 [details] [diff] [review]
Support for .js audit added

I don't understand the purpose of this script: could you put a comment at the top explaining what the forms of input it's supposed to accept? Are you tokenizing C++? JS? something else? Could we re-use existing parsers (the JS engine, and GCC) to void custom tokenization?

Comment 20

[Ondrej Brablc]

Attachment: Short description added to the script

There has been an attempt to use DeHydra for this - see bug 422145. However, according to Taras Glek, it was not that easy due to some problems in parsing NS_LITERAL_CSTRING. So instead of doing it manually, I wrote the script for .CPP modules and recently extended it for .JS files. 

I did not check whether I can use JS engine to do the parsing. But as we do not have it for .CPP where is the majority of SQL code, I feel that it is better that both file types are handled in a single module. Moreover, there is a platform problem running DeHydra on Windows.

The attached patch adds some information to the usage docs in the module.

Comment 21

[Benjamin Smedberg]

Comment on attachment 312951 [details] [diff] [review]
Short description added to the script

I'm not going to review this.

Comment 22

[Shawn Wilsher :sdwilsh]

Note: taras made a dehydra script which will start to do some of this:
http://hg.mozilla.org/users/tglek_mozilla.com/index.cgi/dehydra-mq/file/e935d5f002ae/sql.diff

Comment 23

[Shawn Wilsher :sdwilsh]

And the bug for script is bug 436342.

Comment 24

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 25

[Marco Bonardo [:mak]]

not going to happen, we'll rely on reviews and good practice/habits. We should never build statements from scratch unless we control 100% what is added. So far that just worked.