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

Sqlite ATTACH is crashing php when the filename is bind /w open_basedir set #9032

Closed
mvorisek opened this issue Jul 17, 2022 · 10 comments
Closed

Comments

@mvorisek
Copy link
Contributor

Description

The following code:

<?php

$db = new \PDO("sqlite::memory:");
$db->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);

$db->exec('attach database \':memory:\' AS "db1"');
var_dump($db->exec('create table db1.r (id int)'));

$st = $db->prepare('attach database :a AS "db2"');
$st->execute([':a' => ':memory:']);
var_dump($db->exec('create table db2.r (id int)'));

demo: https://3v4l.org/Su8lK

Resulted in this output:

int(0)

Process exited with code 139.

But I expected this output instead:

int(0)
int(0)

PHP Version

any

Operating System

any

@devnexen
Copy link
Member

Process exited with code 139.

Can you reproduce the issue outside of docker ? Tested with proper releases and manually master build and having expected outputs.

@mvorisek
Copy link
Contributor Author

PHP crashed also when reproduced in GH Actions CI.

I tested now also on my local/not virtualized Windows machine - PHP crashed too, only the first int(0) was displayed.

@cmb69
Copy link
Member

cmb69 commented Jul 17, 2022

That might be an issue with SQLite3 itself. Which version do you use, see https://3v4l.org/PrC72.

@mvorisek
Copy link
Contributor Author

On my Windows machine I use 3.31.1. @devnexen when you tested it, did you see 1 or 2 int(0)?

@devnexen
Copy link
Member

2 always, tried with baremetal Linux machines; old CentOs 7 and Alpine virtualized. Also tried at last a manually build with sanitizers in case.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 17, 2022

@devnexen with what Sqlite versions?

https://3v4l.org/NZnPl /w EOL PHP versions shows it is broken since PHP 5.5

interestingly, Sqlite v3.7.7.1 /w PHP 5.5.x is broken, but Sqlite v3.8.10.2 /w PHP 5.4.x is fine. It seems at least something in php-src has impact on this issue.

I also tested PDOStatement::bindParam. Same issue as with PDOStatement::bindValue.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 17, 2022

I looked into the php-src code - https://github.com/search?q=SQLITE_ATTACHrepo%3Aphp%2Fphp-src&type=Code&ref=advsearch

for SQLITE_ATTACH the code does some security checks. And I think that is the issue. I have open_basedir configured for all the tested platforms. Can it be related?

https://3v4l.org/TLlCo - the answer seems yes!

@mvorisek mvorisek changed the title ATTACH :a AS xx is crashing php (Sqlite/PDOStatement::bindValue) Sqlite ATTACH is crashing php when the filename is bind /w open_basedir set Jul 17, 2022
@devnexen
Copy link
Member

Ah was going to ask if you had any particular context, I even ventured into docker official images and still did not get any issue, and locally my sqlite versions ran from 3.7.17 to 3.38.5.

@cmb69
Copy link
Member

cmb69 commented Jul 17, 2022

Yeah, the problem is that NULL is passed to make_filename_safe() for which it is not prepared. Quick-fix:

 ext/pdo_sqlite/sqlite_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c
index bdad23a581..9ce10a431b 100644
--- a/ext/pdo_sqlite/sqlite_driver.c
+++ b/ext/pdo_sqlite/sqlite_driver.c
@@ -775,6 +775,7 @@ static int authorizer(void *autharg, int access_type, const char *arg3, const ch
 		}
 
 		case SQLITE_ATTACH: {
+			if (!arg3) return SQLITE_DENY;
 					filename = make_filename_safe(arg3);
 			if (!filename) {
 				return SQLITE_DENY;

This needs more careful investigation, though. It might be possible to not necessarily deny authorization, and the SQLITE_COPY case should be investigated as well.

@devnexen
Copy link
Member

devnexen commented Jul 17, 2022

If we want to deny in copy too might be simpler to return NULL in this make_filename_safe helper then ? But indeed might need further look.

@cmb69 cmb69 self-assigned this Jul 18, 2022
cmb69 added a commit to cmb69/php-src that referenced this issue Jul 18, 2022
The arguments 3 to 6 of the authorizer callback may be `NULL`[1], and
we have to properly deal with that.  Instead of causing a segfault, we
deny authorization, which is still better than a crash, and apparently,
we cannot do better anyway.

[1] <https://www.sqlite.org/c3ref/set_authorizer.html>
@cmb69 cmb69 linked a pull request Jul 18, 2022 that will close this issue
cmb69 added a commit that referenced this issue Jul 27, 2022
* PHP-8.0:
  Fix GH-9032: SQLite3 authorizer crashes on NULL values
@cmb69 cmb69 closed this as completed in 8ed21a8 Jul 27, 2022
cmb69 added a commit that referenced this issue Jul 27, 2022
* PHP-8.1:
  Fix GH-9032: SQLite3 authorizer crashes on NULL values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@mvorisek @cmb69 @devnexen and others