853091 - invalid reads in sqlite3 reported by ASAN

Status: RESOLVED WORKSFORME

(Toolkit :: Storage, defect)

Description

[Joe Drew (not getting mail)]

Compile Firefox with ASAN. Try to start it. It will crash immediately because of invalid reads inside sqlite3; in this case, it's in the memcmp() at sqlite3.c:27640, which compares pVfs->zName with "unix-excl". The pVfs->zName, though, is just "unix". 

There are other problems too, but I need ASAN to work so I've just turned it off on *sqlite3* functions.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't think this actually is a security problem... The contents of the memory beyond the '\0' will not matter in the final result of the comparison (since 0 is still less than everything in the reference string) so there's no risk of malicious code somehow altering the behavior of sqlite.

I don't know why  memcmp is used here, maybe there were performance concerns? A simple (and, admittedly, naive) fix here would be to use strncmp. That would bail out when it hit the '\0' and should prevent the ASAN warning.

CC'ing the head SQLite developer to make sure this is known and to get his thoughts on the above.

Comment 2

[Christian Holler (:decoder)]

This is already being tracked in the Chromium bugtracker and shouldn't be a security problem:

http://code.google.com/p/chromium/issues/detail?id=178677

Comment 3

[D. Richard Hipp]

I do not think this is a real issue, or if it is it is exceedingly obscure.  Nevertheless, we have changed several memcpy() calls to strncmp() or strcmp().  This occurred a few weeks ago and is in the 3.7.16 release of SQlite.  See http://www.sqlite.org/src/info/d73435587b for the specific change.

The problem is (I am told) that optimized versions of memcmp() will not necessarily stop at the first byte difference, and hence might read past the end of a buffer.  strcmp() and strncmp(), on the other hand, are guaranteed to stop reading at the first byte difference.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks! We have a bug open to upgrade our in-tree SQLite to 3.7.16 already (bug 853266). Shall we dup to that?

Comment 5

[Marco Bonardo [:mak]]

I may review the patch for the new version of SQLite on next Monday

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok. In the meantime I don't see any reason to keep this bug hidden.

Comment 7

[Christian Holler (:decoder)]

Agreed, this is already public in the Chromium Tracker.

Comment 8

[Marco Bonardo [:mak]]

Is this still an issue after we upgraded to SQLite 3.7.16.1?
fwiw now we are about to upgrade to 3.7.17 in bug 874171.