shess@, looks like you handled the relevant sqlite roll at https://chromium.googlesource.com/chromium/src/+/0270407c0555655d204235d1fa39c86d453cb809

Could you look into this?

shess: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

shess: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

michaeln/pwnall could you please help triage/fix this security bug in sqlite. shess has left.

michaeln@: I got this, unless you particularly want it :)

This still repros with SQLite 3.20.1, which we're about to import.

=================================================================
==10171==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x63400001dcc0 at pc 0x00000058aa53 bp 0x7ffde6e0fd50 sp 0x7ffde6e0fd48
READ of size 4 at 0x63400001dcc0 thread T0
    #0 0x58aa52 in displayP4 third_party/sqlite/amalgamation/sqlite3.c:73535:35
    #1 0x58aa52 in sqlite3VdbeList third_party/sqlite/amalgamation/sqlite3.c:73898
    #2 0x58aa52 in sqlite3Step third_party/sqlite/amalgamation/sqlite3.c:77495
    #3 0x58aa52 in sqlite3_step third_party/sqlite/amalgamation/sqlite3.c:77564
    #4 0x5a29e8 in sqlite3_exec third_party/sqlite/amalgamation/sqlite3.c:112078:12
    #5 0x4ef31e in LLVMFuzzerTestOneInput third_party/sqlite/fuzz/ossfuzz.c:75:3
    #6 0x50c9a7 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) third_party/libFuzzer/src/FuzzerLoop.cpp:463:13
    #7 0x4f0766 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) third_party/libFuzzer/src/FuzzerDriver.cpp:273:6
    #8 0x4fb193 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) third_party/libFuzzer/src/FuzzerDriver.cpp:689:9
    #9 0x520db8 in main third_party/libFuzzer/src/FuzzerMain.cpp:20:10
    #10 0x7fe37c215509 in __libc_start_main (/lib64/libc.so.6+0x20509)

0x63400001dcc0 is located 0 bytes to the right of 120000-byte region [0x634000000800,0x63400001dcc0)
allocated by thread T0 here:
    #0 0x4c2c63 in __interceptor_malloc (/home/pwnall/chromium/src/out/libfuzzer/sqlite3_ossfuzz_fuzzer+0x4c2c63)
    #1 0x91d2dd in sqlite3MemMalloc third_party/sqlite/amalgamation/sqlite3.c:21340:7
    #2 0x55ee40 in mallocWithAlarm third_party/sqlite/amalgamation/sqlite3.c:25024:7
    #3 0x55ee40 in sqlite3Malloc third_party/sqlite/amalgamation/sqlite3.c:25054
    #4 0x5b2687 in setupLookaside third_party/sqlite/amalgamation/sqlite3.c:142457:14
    #5 0x5baa35 in openDatabase third_party/sqlite/amalgamation/sqlite3.c:144864:3
    #6 0x4ef1d9 in LLVMFuzzerTestOneInput third_party/sqlite/fuzz/ossfuzz.c:52:8
    #7 0x50c9a7 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) third_party/libFuzzer/src/FuzzerLoop.cpp:463:13
    #8 0x4f0766 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) third_party/libFuzzer/src/FuzzerDriver.cpp:273:6
    #9 0x4fb193 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) third_party/libFuzzer/src/FuzzerDriver.cpp:689:9
    #10 0x520db8 in main third_party/libFuzzer/src/FuzzerMain.cpp:20:10
    #11 0x7fe37c215509 in __libc_start_main (/lib64/libc.so.6+0x20509)

SUMMARY: AddressSanitizer: heap-buffer-overflow third_party/sqlite/amalgamation/sqlite3.c:73535:35 in displayP4
Shadow bytes around the buggy address:
  0x0c687fffbb40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c687fffbb50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c687fffbb60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c687fffbb70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c687fffbb80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c687fffbb90: 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa
  0x0c687fffbba0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c687fffbbb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c687fffbbc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c687fffbbd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c687fffbbe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==10171==ABORTING

The ASAN build fails with the input below. The odd character between "EXPLAIN" and "PRAGMA" in the clusterfuzz test is unnecessary.

CREATE TABLE t AS SELECT zeroblob(1219381); CREATE TABLE s AS SELECT 3; EXPLAIN PRAGMA integrity_check;

The build seems to fail with any zeroblob argument >= 1219381 (0x129b35), but does not fail with 1219380 or below.

I remapped the call stack to the original source code.

1) third_party/sqlite/src/src/vdbeaux.c:1378 - displayP4
-- the crash is at the sqlite3XPrintf instruction under the P4_INTARRAY branch
-- i must be bigger than the size of ai[i]
-- pOp is an opcode (struct Op)
-- pOp->p4type must equal P4_INTARRAY
-- ai is pOp->p4.ai
-- comments suggest that the first element is the number of elements to follow

2) third_party/sqlite/src/src/vdbeaux.c:1741 - sqlite3VdbeList
-- the crash is at the displayP4 call

3) third_party/sqlite/src/src/vdbeapi.c:623 - sqlite3Step
-- the crash is at the sqlite3VdbeList(p) call inside "if( p->explain ){"
-- p is a struct Vdbe*

4) third_party/sqlite/src/src/vdbeapi.c:692 - sqlite3_step
-- the crash is at the sqlite3Step(v) call
-- v is a struct Vdbe*
-- v is reinterpret-cast of pStmt argument, which is sqlite3_stmt *

5) third_party/sqlite/src/src/legacy.c:69 - sqlite3_exec
-- the crash is at the sqlite3_step(pStmt) call near the start of the "while( 1 ){" loop
-- pStmt is set by a sqlite3_prepare_v2 statement on line 53

I also remapped the allocation call stack.

1) third_party/sqlite/src/src/mem1.c:132 - sqlite3MemMalloc
-- the SQLITE_MALLOC call inside the #ifdef SQLITE_MALLOCSIZE (not in the #else)

2) third_party/sqlite/src/src/malloc.c:250 -- mallocWithAlarm
-- the alloc is in the sqlite3GlobalConfig.m.xalloc(nFull) right before "#ifdef SQLITE_ENABLE_MEMORY_MANAGEMENT"

3) third_party/sqlite/src/src/malloc.c:280 - sqlite3Malloc
-- the alloc is in mallocWithAlarm inside "}else if( sqlite3GlobalConfig.bMemstat ){"

4) third_party/sqlite/src/src/main.c:688 -- setupLookaside
-- the alloc is in sqlite3Malloc labeled /* IMP: R-61949-35727 */
-- the pBuf argument is 0, no pre-allocated buffer

5) third_party/sqlite/src/src/main.c:3095 -- openDatabase
-- the alloc is in setupLookaside

Here's another large ZEROBLOB related bug.
https://bugs.chromium.org/p/chromium/issues/detail?id=759526

Errata for #14: "struct Op" is actually an alias for "struct VdbeOp" defined in third_party/sqlite/src/src/vdbe.h

Local arguments around the crash site:

pOp->opcode: 142  (Op_IntegrityCk)
pOp->p4Type: -15  (P4_INTARRAY)  // stores root page numbers of all tables in db
pOp->p1: 2
pOp->p2: 3
pOp->p3: 1  // One less than the maximum number of allowed errors
pOp->p5: 0  // Check done on main db file, not an auxiliary file
n (pOp->p4.ai[0]): 301

The opcode appears to be put together at third_party/sqlite/src/src/pragma.c:1555
"sqlite3VdbeAddOp4(v, OP_IntegrityCk, 2, cnt, 1, (char*)aRoot,P4_INTARRAY);"

The bug seems to be in the loop in pragma.c lines 1540-1547. The loop seems to allocate a zero-terminated array. However, displayP4 in vdbeaux.c expects a length-prefixed array.

A (correct?) example of setting up a P4_INTARRAY is in third_party/sqlite/src/src/select.c lines 2940-2949 (starting at "aPermute = sqlite3DbMallocRawNN"). In this example, the array size is in the first element. "aPermute" is passed as a P4_INTARRAY data on line 3127.

The interpreter code for Op_IntegrityCk is in third_party/sqlite/src/src/vdbe.c lines 5269-5681. The code there would have to be modified to read the array in the correct format as well.


Possible patch below:

diff --git a/third_party/sqlite/src/src/pragma.c b/third_party/sqlite/src/src/pragma.c
index cc4a5eeb3c35..e5a7dad2c17e 100644
--- a/third_party/sqlite/src/src/pragma.c
+++ b/third_party/sqlite/src/src/pragma.c
@@ -1537,7 +1537,8 @@ void sqlite3Pragma(
       }
       aRoot = sqlite3DbMallocRawNN(db, sizeof(int)*(cnt+1));
       if( aRoot==0 ) break;
-      for(cnt=0, x=sqliteHashFirst(pTbls); x; x=sqliteHashNext(x)){
+      aRoot[0] = cnt;
+      for(cnt=1, x=sqliteHashFirst(pTbls); x; x=sqliteHashNext(x)){
         Table *pTab = sqliteHashData(x);
         Index *pIdx;
         if( HasRowid(pTab) ) aRoot[cnt++] = pTab->tnum;
@@ -1545,7 +1546,6 @@ void sqlite3Pragma(
           aRoot[cnt++] = pIdx->tnum;
         }
       }
-      aRoot[cnt] = 0;
 
       /* Make sure sufficient number of registers have been allocated */
       pParse->nMem = MAX( pParse->nMem, 8+mxIdx );
diff --git a/third_party/sqlite/src/src/vdbe.c b/third_party/sqlite/src/src/vdbe.c
index 22f8682cf5bd..492824a473bb 100644
--- a/third_party/sqlite/src/src/vdbe.c
+++ b/third_party/sqlite/src/src/vdbe.c
@@ -5656,7 +5656,7 @@ case OP_IntegrityCk: {
   nRoot = pOp->p2;
   aRoot = pOp->p4.ai;
   assert( nRoot>0 );
-  assert( aRoot[nRoot]==0 );
+  assert( aRoot[0]==nRoot );
   assert( pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor) );
   pnErr = &aMem[pOp->p3];
   assert( (pnErr->flags & MEM_Int)!=0 );
@@ -5664,7 +5664,7 @@ case OP_IntegrityCk: {
   pIn1 = &aMem[pOp->p1];
   assert( pOp->p5<db->nDb );
   assert( DbMaskTest(p->btreeMask, pOp->p5) );
-  z = sqlite3BtreeIntegrityCheck(db->aDb[pOp->p5].pBt, aRoot, nRoot,
+  z = sqlite3BtreeIntegrityCheck(db->aDb[pOp->p5].pBt, aRoot + 1, nRoot,
                                  (int)pnErr->u.i+1, &nErr);
   sqlite3VdbeMemSetNull(pIn1);
   if( nErr==0 ){

Hi, Richard!

We found a security vulnerability in SQLite. Running an "EXPLAIN PRAGMA integrity_check;" query can result in invalid memory accesses. The line numbers below should match the SQLite 3.20.1 release.

The query is turned into a VDBE opcode in src/pragma.c at line 1555, which calls sqlite3VdbeAddOp4 with OP_IntegrityCk. The call passes the "aRoot" array as a P4_INTARRAY. The array is prepared at lines 1540-1547, as a zero-terminated array.

However, displayP4 in src/vdbeaux.c expects that all P4_INTARRAYs are length-prefixed arrays. The relevant code is in src/vdbeaux.c at lines 1372-1383.

I found another case where a P4_INTARRAY is prepared, at src/select.c, in lines lines 2940-2949 (starting at "aPermute = sqlite3DbMallocRawNN"). In this example, the array size is in the first element. "aPermute" is passed as a P4_INTARRAY data on line 3127, via a sqlite3VdbeAddOp4 call with OP_Permutation.

I'm guessing that the correct fix is to set up the array OP_IntegrityCk as a length-prefixed array. In that case, the interpreter code for Op_IntegrityCk would have to be modified to read the array in the correct format as well. This code is in third_party/sqlite/src/src/vdbe.c lines 5269-5681.

I've put together a proposed patch, below. Feel free to use it as a starting point, if it helps, or throw it away completely, if you'd rather fix this issue in a different way. The commit hashes are most likely useless, and the line numbers should match the source code for SQLite 3.20.1. 

diff --git a/third_party/sqlite/src/src/pragma.c b/third_party/sqlite/src/src/pragma.c
index cc4a5eeb3c35..e5a7dad2c17e 100644
--- a/third_party/sqlite/src/src/pragma.c
+++ b/third_party/sqlite/src/src/pragma.c
@@ -1537,7 +1537,8 @@ void sqlite3Pragma(
       }
       aRoot = sqlite3DbMallocRawNN(db, sizeof(int)*(cnt+1));
       if( aRoot==0 ) break;
-      for(cnt=0, x=sqliteHashFirst(pTbls); x; x=sqliteHashNext(x)){
+      aRoot[0] = cnt;
+      for(cnt=1, x=sqliteHashFirst(pTbls); x; x=sqliteHashNext(x)){
         Table *pTab = sqliteHashData(x);
         Index *pIdx;
         if( HasRowid(pTab) ) aRoot[cnt++] = pTab->tnum;
@@ -1545,7 +1546,6 @@ void sqlite3Pragma(
           aRoot[cnt++] = pIdx->tnum;
         }
       }
-      aRoot[cnt] = 0;
 
       /* Make sure sufficient number of registers have been allocated */
       pParse->nMem = MAX( pParse->nMem, 8+mxIdx );
diff --git a/third_party/sqlite/src/src/vdbe.c b/third_party/sqlite/src/src/vdbe.c
index 22f8682cf5bd..492824a473bb 100644
--- a/third_party/sqlite/src/src/vdbe.c
+++ b/third_party/sqlite/src/src/vdbe.c
@@ -5656,7 +5656,7 @@ case OP_IntegrityCk: {
   nRoot = pOp->p2;
   aRoot = pOp->p4.ai;
   assert( nRoot>0 );
-  assert( aRoot[nRoot]==0 );
+  assert( aRoot[0]==nRoot );
   assert( pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor) );
   pnErr = &aMem[pOp->p3];
   assert( (pnErr->flags & MEM_Int)!=0 );
@@ -5664,7 +5664,7 @@ case OP_IntegrityCk: {
   pIn1 = &aMem[pOp->p1];
   assert( pOp->p5<db->nDb );
   assert( DbMaskTest(p->btreeMask, pOp->p5) );
-  z = sqlite3BtreeIntegrityCheck(db->aDb[pOp->p5].pBt, aRoot, nRoot,
+  z = sqlite3BtreeIntegrityCheck(db->aDb[pOp->p5].pBt, aRoot + 1, nRoot,
                                  (int)pnErr->u.i+1, &nErr);
   sqlite3VdbeMemSetNull(pIn1);
   if( nErr==0 ){

What do you think is the right way to fix this problem?

Thank you very much!

+cmumford@, jsbell@ for context, as one of you will be reviewing the patch(es) that come out of this.

I independently came up with a patch similar to that in Comment 19.  I'm testing it now...

Why did we (the SQLite developers) not get notification of this bug sooner?  I thought we were suppose to be getting all OSS-discovered problems?  But this is the first I've seen of this issue....

Process glitch on our end - the bug got assigned to someone who is no longer active on the project, and since access was restricted it didn't show up for the new owners.

Official bug fix here: https://sqlite.org/src/info/adc12c83dda8ba93

Let me know if you need a back-port to some earlier version of SQLite.

Thank you very much for the quick turnaround, Dr. Hipp!

I'll ask for help if I have trouble backporting the fix on my own -- we're on 3.20.1 now, and the applying patch looks fairly straightforward. Many thanks for the generous offer!

I turned https://sqlite.org/src/info/adc12c83dda8ba93 into CL https://crrev.com/c/676477 which is under review.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a88af4713bfdd24669cdf1c9e500e6bc5126946e

commit a88af4713bfdd24669cdf1c9e500e6bc5126946e
Author: Victor Costan <pwnall@chromium.org>
Date: Thu Sep 21 21:53:39 2017

sqlite: Fix invalid memory access in "EXPLAIN PRAGMA integrity_check".

Bug:  730379 
Change-Id: If3a5091481a9de983caece1cbc955714ba51f481
Reviewed-on: https://chromium-review.googlesource.com/676477
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503575}
[modify] https://crrev.com/a88af4713bfdd24669cdf1c9e500e6bc5126946e/third_party/sqlite/amalgamation/sqlite3.c
[add] https://crrev.com/a88af4713bfdd24669cdf1c9e500e6bc5126946e/third_party/sqlite/patches/0012-Fix-EXPLAIN-PRAGMA-integrity_check-output.patch
[modify] https://crrev.com/a88af4713bfdd24669cdf1c9e500e6bc5126946e/third_party/sqlite/src/src/pragma.c
[modify] https://crrev.com/a88af4713bfdd24669cdf1c9e500e6bc5126946e/third_party/sqlite/src/src/vdbe.c
[modify] https://crrev.com/a88af4713bfdd24669cdf1c9e500e6bc5126946e/third_party/sqlite/src/src/vdbeaux.c
[modify] https://crrev.com/a88af4713bfdd24669cdf1c9e500e6bc5126946e/third_party/sqlite/src/test/pragma4.test

awhalley@: Adding you to weigh in on whether we need to think about merging this.

IMHO we don't need a merge, because the invalid memory accesses are reads, and controlling the address seems fairly difficult. At the same time, all my security background is around building defenses, and I don't have enough experience to know which vulnerabilities are more likely to get weaponized. Please advise?

Additional context that might be relevant: I just did a minor SQLite upgrade before this (3.20 RC to 3.20.1), which wasn't the best sequencing, in retrospect. We'd have to merge the upgrade as well, or develop a separate patch.

ClusterFuzz has detected this issue as fixed in range 503528:503598.

Detailed report: https://clusterfuzz.com/testcase?key=6430022067027968

Fuzzer: libFuzzer_sqlite3_ossfuzz_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x63400001dcc0
Crash State:
  displayP4
  sqlite3VdbeList
  sqlite3Step
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=458147:458197
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=503528:503598

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6430022067027968

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6430022067027968 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot