Last Comment Bug 351458 - Potential SQL injection in /netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
: Potential SQL injection in /netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
: Patrick McManus [:mcmanus]
Depends on: 442806
  Show dependency treegraph
Reported: 2006-09-05 12:51 PDT by Thor Larholm
Modified: 2011-11-30 08:53 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Description User image Thor Larholm 2006-09-05 12:51:46 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20060728 Firefox/
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20060728 Firefox/

Most of the SQL statements in nsDiskCacheDeviceSQL.cpp that accept some kind of input from the outside world are defined in the prepared structure, e.g.:

{ mStatement_DeleteEntry,     "DELETE FROM moz_cache WHERE ClientID = ? AND Key = ?;" }

These prepared statements are all executed with statement->ExecuteStep after having their paramters bound with BindDataParameter/BindCStringParameter/etc, which properly guards against SQL injections.

Aside from these, there are 5 calls to mDB->ExecuteSimpleSQL - of which one accepts input and does not have its input properly bound. See

The problematic piece of code is 

    deleteCmd =
      PR_smprintf("DELETE FROM moz_cache WHERE ClientID=%q AND Flags=0;",

PR_smprintf simply adds quotes around the input. If you can control clientID and have it contain quotes you can inject and execute arbitrary SQL.

I've written "Potential SQL injection" in the summary as I haven't looked deeply into all the places where nsDiskCacheDevice::EvictEntries is called. See

If you can successfully inject SQL you can alter the content of the disk and memory cache as well as read/write files to the local system through ATTACH/CREATE.

Reproducible: Always

Steps to Reproduce:
1. Provide EvictEntries with a clientID char that contains quotes
2. Wait for mDB->ExecuteSimpleSQL
3. SQL Injection
Comment 1 User image Thor Larholm 2006-09-06 02:17:53 PDT
Did anyone take a look at this? It's still labelled UNCONFIRMED
Comment 2 User image Darin Fisher 2006-09-06 09:25:22 PDT
nsDiskCacheDeviceSQL.cpp is not used in any Mozilla product.  It just happens to live in the source tree as an experimental disk cache implementation.
Comment 3 User image timeless 2006-09-06 18:35:54 PDT
since this code is not shipping in gecko, i see no reason to keep this bug confidential.
Comment 4 User image Thor Larholm 2006-09-07 00:48:32 PDT
Indeed, the default configure contains NECKO_DISK_CACHE=1 instead of NECKO_DISK_CACHE_SQL=1 so nsCacheService.cpp never gets to use the SQLite implementation of the cache. Even Firefox 2 doesn't use it, but still has the source in it's tree - any reason to even keeping it there? :)
Comment 5 User image timeless 2006-09-21 11:38:19 PDT
presumably someone was hoping to eventually switch to it. if they choose to do that, they'll be required to fix this before they turn it on. in the interim, anyone including you could post a patch for it. presumably darin would gladly review it.
Comment 6 User image Steffen Wilberg 2010-02-15 14:32:47 PST
> presumably someone was hoping to eventually switch to it. if they choose to do
> that, they'll be required to fix this before they turn it on.
The offline cache already uses nsDiskCacheDeviceSQL...
Comment 7 User image Honza Bambas (:mayhemer) 2010-02-15 14:49:45 PST
The ClientID is built from the URL of the manifest, that must be a valid URL and is defined in <html manifest=> tag.  So, there is a potential to exploit this.

Going to fix this as it should be quit simple.
Comment 8 User image Honza Bambas (:mayhemer) 2010-02-15 15:24:58 PST
This has been fixed as part of bug 442806,

Should we back port only the fix for this issue to 1.9.0?
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2010-02-15 16:05:26 PST
If it's easy, probably worth it.
Comment 10 User image Marco Castelluccio [:marco] 2011-11-18 13:45:08 PST
Was this patch backported?
Comment 11 User image Steffen Wilberg 2011-11-19 15:15:31 PST
(In reply to Marco Castelluccio from comment #10)
The fix from bug 442806 wasn't backported to Gecko 1.9.0, which is equivalent to Firefox 3.0, because that's way past end-of-life. Gecko 1.9.1/Firefox 3.5 contains the fix, but is past end-of-life as well. Firefox 3.6.24 is still supported if you have to run such an old browser.

Can we mark this bug as FIXED (by bug 442806, according to comment 8)?
Comment 12 User image Honza Bambas (:mayhemer) 2011-11-30 08:53:05 PST
Fixed by bug 442806.  Fix contained since 1.9.1.

Note You need to log in before you can comment on or make changes to this bug.