If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

sqlite3MemMalloc better check for int overflow

RESOLVED INVALID

Status

()

Toolkit
Storage
RESOLVED INVALID
9 years ago
9 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Unassigned)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate])

http://mxr.mozilla.org/mozilla-central/source/db/sqlite3/src/sqlite3.c#13304

static void *sqlite3MemMalloc(int nByte){
...
  nReserve = (nByte+7)&~7;
  totalSize = nReserve + sizeof(*pHdr) + sizeof(int) +
               mem.nBacktrace*sizeof(void*) + mem.nTitle;

  p = malloc(totalSize);

so |nReserve| and |totalSize| can overflow in several ways.

overflowing in the guts of memory allocator is not good.

breakpoint in sqlite3MemMalloc is hit frequently with usual browsing.
Whiteboard: [sg:investigate]
Component: General → SQL
Product: Firefox → Core
Component: SQL → Storage
Product: Core → Toolkit
QA Contact: general → storage
Shawn - do we ever patch our local version of sqlite, or just upstream patches?
We never patch sqlite because it makes it much harder to upgrade.  Additionally, it means we couldn't support system sqlite, which makes linux distros hate me.

Comment 3

9 years ago
SQLite is architected such that the memory allocator will never be called with
an "nByte" parameter larger than SQLITE_MAX_LENGTH+e (where e is a small
number, approximately 10).  SQLITE_MAX_LENGTH is 1,000,000,000 by default.  So
there really is no chance of an overflow within sqlite3MemMalloc().

Furthermore, the code snippet above where the alleged integer overflow occurs
is within the "memsys2" memory allocator, which is normally disabled.  Memsys2
is used only for testing and debugging.  You should be using memsys1 in FF.  If
not, please change to it, because it would give you a substantial speed boost. 
Memsys2 is enabled by the -DSQLITE_MEMDEBUG=1 compile-time option.

Additional information on memsys2:  http://www.sqlite.org/malloc.html#memdebug

See also: http://www.sqlite.org/limits.html
>will never be called with
>an "nByte" parameter larger than SQLITE_MAX_LENGTH+e (where e is a small
>number, approximately 10).  SQLITE_MAX_LENGTH is 1,000,000,000 by default.

quick grep for SQLITE_MAX_LENGTH shows some *signed* checks that will pass for suitable *negative* int/long

static void zeroblobFunc(
  i64 n;
  if( n>SQLITE_MAX_LENGTH ){
btw, i suggest to define constant max_length with trailing U, making it unsigned.

this gcc feature killed a lot of bugs...
is it easy to test my luck with memsys1 by passing it the random value |-1| ?

Comment 7

9 years ago
http://www.sqlite.org/cvstrac/chngview?cn=6298

The change above will be in SQLite version 3.6.11 which we are still on track to release this evening (EST - or early tomorrow UTC).

Can we put this issue to rest now?
I'm happy with it.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
INVALID, not WONTFIX, as the bug is in a third-party product.
Resolution: WONTFIX → INVALID
Group: core-security
You need to log in before you can comment on or make changes to this bug.