Closed Bug 405920 Opened 12 years ago Closed 3 years ago

audit all places queries for sql injection potential

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dietrich, Unassigned)

References

Details

(Keywords: sec-audit, Whiteboard: [sg:audit])

Attachments

(1 file, 6 obsolete files)

* 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.
Blocks: 375898
Since this is a dependency of bug 375898, marking P2 so it stays on our radar for Fx3 triage/tracking.
Flags: blocking-firefox3?
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
Whiteboard: [sg:investigate]
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 beta4 → Firefox 3
Assignee: nobody → ondrej
Status: NEW → ASSIGNED
not sure if I have cycles to help with this - I'm a wee bit swamped.
Depends on: 422145
Attached file Script for analysis of SQL statements (obsolete) —
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.
Attachment #309194 - Flags: review?(dietrich)
I yet have to make analysis of the results produced by this tool for the first time.
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.
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.
what do you mean by %q is non-standard?  It's being consumed by the sprintf, right?
(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?

ah - gotcha.  So, that statement should probably be fixed (and clearly isn't being unit tested).
Blocks: 423402
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.
Attachment #309194 - Attachment is obsolete: true
Attachment #309195 - Attachment is obsolete: true
Attachment #309376 - Attachment is obsolete: true
Attachment #310003 - Flags: review?(dietrich)
Attachment #309194 - Flags: review?(dietrich)
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.
Attachment #310003 - Flags: review?(sdwilsh)
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.
Attachment #310003 - Flags: review?(dietrich) → review+
Attached patch Patch without the log file (obsolete) — Splinter Review
Attachment #310003 - Attachment is obsolete: true
Attachment #311018 - Flags: review?(sdwilsh)
Attachment #310003 - Flags: review?(sdwilsh)
Blocks: 424408
I have filed the tinderbox follow up bug 424408.
Whiteboard: [sg:investigate] → [sg:investigate][needs review sdwilsh]
Renoming for blocking.  I don't think this should block anymore - issues found by the script were filed as follow-up bugs.
Flags: blocking-firefox3+ → blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3-
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.
Attachment #311018 - Flags: review?(sdwilsh)
Attached patch Support for .js audit added (obsolete) — Splinter Review
(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?
Attachment #312784 - Flags: review?(sdwilsh)
Attachment #311018 - Attachment is obsolete: true
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.
Attachment #312784 - Flags: review?(sdwilsh) → review?(benjamin)
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?
Attachment #312784 - Flags: review?(benjamin) → review-
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.
Attachment #312784 - Attachment is obsolete: true
Attachment #312951 - Flags: review?(benjamin)
Comment on attachment 312951 [details] [diff] [review]
Short description added to the script

I'm not going to review this.
Attachment #312951 - Flags: review?(benjamin)
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
And the bug for script is bug 436342.
Target Milestone: Firefox 3 → ---
Assignee: ondrej → nobody
Status: ASSIGNED → NEW
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Depends on: 436342
Whiteboard: [sg:investigate][needs review sdwilsh] → [sg:audit]
Keywords: sec-audit
Priority: P2 → --
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.