Closed
Bug 405920
Opened 17 years ago
Closed 8 years ago
audit all places queries for sql injection potential
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: dietrich, Unassigned)
References
Details
(Keywords: sec-audit, Whiteboard: [sg:audit])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
* 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•17 years ago
|
||
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
Updated•17 years ago
|
Whiteboard: [sg:investigate]
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Target Milestone: Firefox 3 beta4 → Firefox 3
Updated•17 years ago
|
Assignee: nobody → ondrej
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 3•17 years ago
|
||
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)
Comment 4•17 years ago
|
||
I yet have to make analysis of the results produced by this tool for the first time.
Comment 5•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
what do you mean by %q is non-standard? It's being consumed by the sprintf, right?
Comment 8•17 years ago
|
||
(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•17 years ago
|
||
ah - gotcha. So, that statement should probably be fixed (and clearly isn't being unit tested).
Comment 10•17 years ago
|
||
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 11•17 years ago
|
||
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)
Reporter | ||
Comment 12•17 years ago
|
||
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+
Comment 13•17 years ago
|
||
Attachment #310003 -
Attachment is obsolete: true
Attachment #311018 -
Flags: review?(sdwilsh)
Attachment #310003 -
Flags: review?(sdwilsh)
Reporter | ||
Updated•17 years ago
|
Whiteboard: [sg:investigate] → [sg:investigate][needs review sdwilsh]
Comment 15•17 years ago
|
||
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?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3-
Comment 16•17 years ago
|
||
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)
Comment 17•17 years ago
|
||
(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)
Updated•17 years ago
|
Attachment #311018 -
Attachment is obsolete: true
Comment 18•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #312784 -
Flags: review?(sdwilsh) → review?(benjamin)
Comment 19•17 years ago
|
||
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-
Comment 20•17 years ago
|
||
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 21•17 years ago
|
||
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)
Comment 22•17 years ago
|
||
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
Reporter | ||
Updated•16 years ago
|
Target Milestone: Firefox 3 → ---
Updated•16 years ago
|
Assignee: ondrej → nobody
Updated•15 years ago
|
Status: ASSIGNED → NEW
Comment 24•15 years ago
|
||
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
Updated•15 years ago
|
Depends on: 436342
Whiteboard: [sg:investigate][needs review sdwilsh] → [sg:audit]
Updated•9 years ago
|
Priority: P2 → --
Comment 25•8 years ago
|
||
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: 8 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•