Closed Bug 351458 Opened 18 years ago Closed 13 years ago

Potential SQL injection in /netwerk/cache/src/nsDiskCacheDeviceSQL.cpp

Categories

(Core :: Networking: Cache, defect)

Product:
Core
Component:
Networking: Cache
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: thor, Assigned: mayhemer)

References

Details

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

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 http://lxr.mozilla.org/seamonkey/source/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp#1228

The problematic piece of code is 

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

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 http://lxr.mozilla.org/seamonkey/ident?i=EvictEntries

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
Did anyone take a look at this? It's still labelled UNCONFIRMED

nsDiskCacheDeviceSQL.cpp is not used in any Mozilla product.  It just happens to live in the source tree as an experimental disk cache implementation.
since this code is not shipping in gecko, i see no reason to keep this bug confidential.
Group: security
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? :)
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.
> 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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
This has been fixed as part of bug 442806, http://hg.mozilla.org/mozilla-central/rev/3e41c4b388c3.

Should we back port only the fix for this issue to 1.9.0?
If it's easy, probably worth it.
Was this patch backported?
(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)?
Depends on: 442806
Fixed by bug 442806.  Fix contained since 1.9.1.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.