Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLite3: add DEFENSIVE config option for SQLite >= 3.26.0 #3709

Closed
wants to merge 1 commit into
base: master
from

Conversation

7 participants
@bohwaz
Copy link
Contributor

bohwaz commented Dec 16, 2018

This is a new option, available in SQLite starting from 3.26.0 that is meant to prevent any corruption of the database file that could come from arbitrary crafted SQL queries.

From the SQLite doc:

When the defensive flag is enabled, language features that allow ordinary SQL to deliberately corrupt the database file are disabled. The disabled features include but are not limited to the following:

  • The PRAGMA writable_schema=ON statement.
  • Writes to the sqlite_dbpage virtual table.
  • Direct writes to shadow tables.

https://www.sqlite.org/c3ref/c_dbconfig_defensive.html#sqlitedbconfigdefensive

Please note that this could potentially cause BC-breaks if code was writing to sqlite_dbpage, the schema or shadow tables. It is quite unlikely that any PHP code would do that kind of thing IMHO, and if it does I don't think it should be allowed by default.

! BUT another case where it might break things is if you had exported a backup of the DB with SQL statements that would rewrite to the shadow tables. This would be the case for example for database management tools, eg. PHPLiteAdmin. In that case it might be suitable to find a different solution, eg. adding an extra param to the open method to allow disabling the defensive mode. This is open to discussion, if anyone has a better idea I'll try to find some free time to do another PR :)

Also, it is likely that the recent RCE in SQLite is due to users being able to write to shadow tables. See https://news.ycombinator.com/item?id=18685296

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Dec 16, 2018

Thanks! That's really helpful; particularly the link to the Hacker News discussion. I'll have a closer look at this as soon as possible.

@cmb69 cmb69 self-requested a review Dec 16, 2018

@petk petk added the Enhancement label Dec 19, 2018

@cmb69
Copy link
Contributor

cmb69 left a comment

Well, it seems to me that the simplest and most secure solution would be to call sqlite3_config() in MINIT depending on a PHP_INI_SYSTEM option which defaults to enabling SQLITE_DBCONFIG_DEFENSIVE. This change is likely to work smoothly with most existing scripts, and only those few who need to disable SQLITE_DBCONFIG_DEFENSIVE would have to change the ini setting (and to make sure that they are not vulnerable).

We also should consider whether to apply this enhancement to older branches. At least having it in PHP-7.3 appears to be sensible.

@bohwaz

This comment has been minimized.

Copy link
Contributor Author

bohwaz commented Dec 24, 2018

Ah that's a good idea! I will try to find some time in January to do that.

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Dec 26, 2018

Thanks!

Anyhow, would anybody object to adding a new ini setting? Would that need discussion on internals?

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Dec 26, 2018

@cmb69 Ini setting sounds fine. If this is going to get backported due to security concerns, might want to post to internals.

@bohwaz

This comment has been minimized.

Copy link
Contributor Author

bohwaz commented Feb 7, 2019

@cmb69 OK I found some time to change that to be an INI setting, updated on this PR :)

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Feb 15, 2019

Thanks @bohwaz – LGTM! (And sorry for the late reply.) I've written mail to internals@ regarding PHP-7.2.

@Danack

This comment has been minimized.

Copy link
Contributor

Danack commented Feb 15, 2019

Hi - I'm confused by the PR, as I can't see where db_obj is going to be defined where it is used in the changed code: https://github.com/php/php-src/pull/3709/files#diff-d0d9591d093d64f6c62abd62271485baR2417

Also, reading the docs for the defensive flag, at https://www.sqlite.org/c3ref/c_dbconfig_defensive.html#sqlitedbconfigdefensive is says it is to be used by sqlite3_db_config().

Reading the docs for that https://www.sqlite.org/c3ref/db_config.html it says:

The sqlite3_db_config() interface is used to make configuration changes to a database connection. The interface is similar to sqlite3_config() except that the changes apply to a single database connection (specified in the first argument).

So this flag is settable per connection, rather than a module wide setting.

If that's the case I would recommend:

  • Move setting the defensive flag to inside the sqlite::open function.

  • Make it be on by default for new connections - probably by adding it to the default flags that are set for that function, so the default flags would be: SQLITE3_OPEN_READWRITE | SQLITE3_OPEN_CREATE | SQLITE3_OPEN_DEFENSIVE.

Apologies if this is all me just misreading the code....heads a bit fuzzy...

@bohwaz

This comment has been minimized.

Copy link
Contributor Author

bohwaz commented Feb 15, 2019

  • Move setting the defensive flag to inside the sqlite::open function.

You are not misreading the code, my head was fuzzy when I did this apparently, it's fixed, thank you!

  • Make it be on by default for new connections - probably by adding it to the default flags that are set for that function, so the default flags would be: SQLITE3_OPEN_READWRITE | SQLITE3_OPEN_CREATE | SQLITE3_OPEN_DEFENSIVE.

It is another way of using it, but I guess a hosting provider will want to set that for all the code running on the server, with no way for userland code to disable it, as it might lead to security issues affecting other customers such as this RCE. So a system INI setting seems better suited.

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Feb 15, 2019

Oops, my bad! Thanks for catching this, @Danack, and for quickly fixing, @bohwaz!

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Feb 23, 2019

I tried this PR with SQLite 3.26.0 and 3.27.1, and almost all tests are failing due to segfaults…

@cmb69 cmb69 dismissed their stale review Feb 23, 2019

The segfaults need investigation.

@bohwaz

This comment has been minimized.

Copy link
Contributor Author

bohwaz commented Feb 23, 2019

Thanks, will look into that next week or the one after.

@bohwaz bohwaz force-pushed the bohwaz:patch/sqlite3-defensive branch from c9f44bc to 3eaee5e Mar 7, 2019

@bohwaz

This comment has been minimized.

Copy link
Contributor Author

bohwaz commented Mar 7, 2019

OK @cmb69 this is fixed, I was missing an undocumented parameter to sqlite3_db_config. Tried with SQLite 3.26+

Show resolved Hide resolved ext/sqlite3/sqlite3.c
Show resolved Hide resolved ext/sqlite3/sqlite3.c Outdated
SQLite3: add DEFENSIVE config for SQLite >= 3.26.0 as a mitigation st…
…rategy against potential security flaws

@bohwaz bohwaz force-pushed the bohwaz:patch/sqlite3-defensive branch from 1bf542c to 2bf92ae Mar 7, 2019

@@ -996,8 +996,19 @@ cli_server.color = On
;intl.use_exceptions = 0

[sqlite3]
; Directory pointing to SQLite3 extensions
; http://php.net/manual/en/sqlite3.loadextension.php

This comment has been minimized.

@carusogabriel

carusogabriel Mar 7, 2019

Member
Suggested change
; http://php.net/manual/en/sqlite3.loadextension.php
; https://php.net/manual/en/sqlite3.loadextension.php

This comment has been minimized.

@bohwaz

bohwaz Mar 8, 2019

Author Contributor

I'm not against it but all the other php.net URLs in the php.ini are http only, so I followed, if I proceed, it would be the only one to be in https. Not sure if that's ok?

This comment has been minimized.

@cmb69

cmb69 Mar 8, 2019

Contributor

Makes no sense to switch to https one at a time; instead we should change all the URLs after all of php.net has switched to HTTPS.

@@ -996,8 +996,19 @@ cli_server.color = On
;intl.use_exceptions = 0

[sqlite3]
; Directory pointing to SQLite3 extensions
; http://php.net/manual/en/sqlite3.loadextension.php

This comment has been minimized.

@carusogabriel

carusogabriel Mar 7, 2019

Member
Suggested change
; http://php.net/manual/en/sqlite3.loadextension.php
; https://php.net/manual/en/sqlite3.loadextension.php
@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Mar 11, 2019

Thanks, @bohwaz! Applied as e93259b (PHP-7.2+).

PS: documented with http://svn.php.net/viewvc?view=revision&revision=346996.

@cmb69 cmb69 closed this Mar 11, 2019

; https://www.sqlite.org/c3ref/c_dbconfig_defensive.html
; (for older SQLite versions, this flag has no use)
sqlite3.defensive = 1

This comment has been minimized.

@remicollet

remicollet Mar 19, 2019

Contributor

minor comment, this is the default value, so have to be commented.

Fixed in 78c9a53

This comment has been minimized.

@cmb69

cmb69 Mar 19, 2019

Contributor

Thanks for catching this, @remicollet!

This comment has been minimized.

@bohwaz

bohwaz Mar 19, 2019

Author Contributor

Oh sure, thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.