sqlite: add permission model checks to DatabaseSync#62957
sqlite: add permission model checks to DatabaseSync#62957mcollina wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62957 +/- ##
==========================================
- Coverage 89.65% 89.65% -0.01%
==========================================
Files 676 706 +30
Lines 206578 219401 +12823
Branches 39562 42073 +2511
==========================================
+ Hits 185216 196709 +11493
- Misses 13475 14577 +1102
- Partials 7887 8115 +228
🚀 New features to boost your workflow:
|
Add permission model enforcement to DatabaseSync::Open(). File-backed databases now check kFileSystemRead (readOnly) or kFileSystemWrite (read-write) before calling sqlite3_open_v2. In-memory databases (:memory:) are exempt. Refs: https://hackerone.com/reports/3686625
8ffad93 to
877530d
Compare
RafaelGSS
left a comment
There was a problem hiding this comment.
As long as it doesn't open precedence for reports targeting SQLite on permission model, I'm +1.
There was a problem hiding this comment.
Hmm, I think this needs to take into account https://sqlite.org/uri.html
- A database can be opened read-only with
mode=roorimmutable=1. - Another way to create an in-memory database is with
mode=memory.
Doing more string processing on the database path sounds a bit hacky though.
Edit: looks like we have not enabled SQLITE_USE_URI=1, but it would be good to leave a comment here that this needs to be updated when we do at the very least.
As long as it doesn't open precedence for reports targeting SQLite on permission model, I'm +1.
Passing a custom VFS to SQLite that can really prevent reads/writes might be a more comprehensive solution worth considering...
https://sqlite.org/vfs.html sounds really cool! I will take a look. |
Add permission model enforcement to
DatabaseSync::Open().kFileSystemRead(readOnly) orkFileSystemWrite(read-write) before callingsqlite3_open_v2.:memory:) are exempt.Known limitation:
db.exec()withATTACH DATABASESQL can still access arbitrary files at runtime. This would require parsing SQL to interceptATTACHstatements. Should we address that in this PR or a follow-up?