CVE-2020-4035: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: In WatermelonDB (NPM package "@nozbe/watermelondb") before versions 0.15.1 and 0.16.2, a maliciously crafted record ID can exploit a SQL Injection vulnerability in iOS adapter implementation and cause the app to delete all or selected records from the database, generally causing the app to become unusable. This may happen in apps that don't validate IDs (valid IDs are `/^[a-zA-Z0-9_-.]+$/`) and use Watermelon Sync or low-level `database.adapter.destroyDeletedRecords` method. The integrity risk is low due to the fact that maliciously deleted records won't synchronize, so logout-login will restore all data, although some local changes may be lost if the malicious deletion causes the sync process to fail to proceed to push stage. No way to breach confidentiality with this vulnerability is known. Full exploitation of SQL Injection is mitigated, because it's not possible to nest an insert/update query inside a delete query in SQLite, and it's not possible to pass a semicolon-separated second query. There's also no known practicable way to breach confidentiality by selectively deleting records, because those records will not be synchronized. It's theoretically possible that selective record deletion could cause an app to behave insecurely if lack of a record is used to make security decisions by the app. This is patched in versions 0.15.1, 0.16.2, and 0.16.1-fix
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 5.9
Published: 2020-06-03T17:15Z
Repository: https://github.com/Nozbe/WatermelonDB
Language: JavaScript
Files changed: native/ios/WatermelonDB/DatabaseDriver.swift, src/adapters/__tests__/commonTests.js
Commit date: 2020-06-01 12:31:24 +0200
Commit message: Fix destroyDeletedRecords vulnerability

Diff: diff --git a/native/ios/WatermelonDB/DatabaseDriver.swift b/native/ios/WatermelonDB/DatabaseDriver.swift
index fe00a03a..4be8015e 100644
--- a/native/ios/WatermelonDB/DatabaseDriver.swift
+++ b/native/ios/WatermelonDB/DatabaseDriver.swift
@@ -126,12 +126,12 @@ class DatabaseDriver {
         }
     }
 
     func destroyDeletedRecords(table: Database.TableName, records: [RecordId]) throws {
         // TODO: What's the behavior if record doesn't exist or isn't actually deleted?
-        let recordIds = records.map { id in "'\(id)'" }.joined(separator: ",")
-        try database.execute("delete from \(table) where id in (\(recordIds))")
+        let recordPlaceholders = records.map { _ in "?" }.joined(separator: ",")
+        try database.execute("delete from \(table) where id in (\(recordPlaceholders))", records)
     }
 
 // MARK: - LocalStorage
 
     func getLocal(key: String) throws -> String? {
diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js
index 69ee9c3a..ad0c23a2 100644
--- a/src/adapters/__tests__/commonTests.js
+++ b/src/adapters/__tests__/commonTests.js
@@ -437,10 +437,35 @@ export default () => [
       expectSortedEqual(await adapter.query(taskQuery()), ['t4'])
       expect(await adapter.find('tasks', 't1')).toBeNull()
       expect(await adapter.find('tasks', 't2')).toBeNull()
     },
   ],
+  [
+    'destroyDeletedRecords can handle unsafe strings',
+    async adapter => {
+      const m1 = mockTaskRaw({ id: 't1', text1: 'bar1', order: 1 })
+      const m2 = mockTaskRaw({ id: 't2', text1: 'bar2', order: 2 })
+      const m3 = mockTaskRaw({ id: 't3', text1: 'bar3', order: 3 })
+      await adapter.batch([
+        ['create', 'tasks', m1],
+        ['create', 'tasks', m2],
+        ['create', 'tasks', m3],
+      ])
+      await adapter.batch([
+        ['markAsDeleted', 'tasks', m1.id],
+        ['markAsDeleted', 'tasks', m2.id],
+        ['markAsDeleted', 'tasks', m3.id],
+      ])
+
+      await adapter.destroyDeletedRecords('tasks', ['\') or 1=1 --'])
+      expectSortedEqual(await adapter.getDeletedRecords('tasks'), ['t1', 't2', 't3'])
+      expectSortedEqual(await adapter.query(taskQuery()), [])
+
+      await adapter.destroyDeletedRecords('tasks', ['\'); insert into tasks (id) values (\'t4\') --'])
+      expectSortedEqual(await adapter.query(taskQuery()), [])
+    },
+  ],
   [
     'can run mixed batches',
     async _adapter => {
       let adapter = _adapter
       const m1 = mockTaskRaw({ id: 't1', text1: 'bar' })

Vulnerable code:         let recordIds = records.map { id in "'\(id)'" }.joined(separator: ",")
        try database.execute("delete from \(table) where id in (\(recordIds))")
Fixed code:         let recordPlaceholders = records.map { _ in "?" }.joined(separator: ",")
        try database.execute("delete from \(table) where id in (\(recordPlaceholders))", records)
  [
    'destroyDeletedRecords can handle unsafe strings',
    async adapter => {
      const m1 = mockTaskRaw({ id: 't1', text1: 'bar1', order: 1 })
      const m2 = mockTaskRaw({ id: 't2', text1: 'bar2', order: 2 })
      const m3 = mockTaskRaw({ id: 't3', text1: 'bar3', order: 3 })
      await adapter.batch([
        ['create', 'tasks', m1],
        ['create', 'tasks', m2],
        ['create', 'tasks', m3],
      ])
      await adapter.batch([
        ['markAsDeleted', 'tasks', m1.id],
        ['markAsDeleted', 'tasks', m2.id],
        ['markAsDeleted', 'tasks', m3.id],
      ])

      await adapter.destroyDeletedRecords('tasks', ['\') or 1=1 --'])
      expectSortedEqual(await adapter.getDeletedRecords('tasks'), ['t1', 't2', 't3'])
      expectSortedEqual(await adapter.query(taskQuery()), [])

      await adapter.destroyDeletedRecords('tasks', ['\'); insert into tasks (id) values (\'t4\') --'])
      expectSortedEqual(await adapter.query(taskQuery()), [])
    },
  ],