PDO Subclasses coming to vote soon.

10 months ago by Dan Ackroyd — view source

unread

Hi everyone,

Just giving an update on the
https://wiki.php.net/rfc/pdo_driver_specific_subclasses RFC as time is
running out. The RFC text has been updated with the implemented
subclasses stubs.

There are a few small things to note, and one larger thing:

Marc Bennewitz wrote:

It would be great if driver specific constants would be added to the
driver specific sub-classes without the driver name repeated in the
const name.

Okay, that will be done before it goes to vote. It probably has a
downside of making there need to be a large duplication of tests,
rather than being able to re-use the existing tests, but I guess it's
probably worth doing.

Create all DB sub-classes?

yes please

k. That has been done.

Rowan Tommins wrote:

but as a minimum there should be an internal API for
registering a sub-class, without any modification to ext/pdo.

There is now.

The larger issue is whether to add an ini setting for SQLite extension
loading or not.

The sqlite3 PHP extension has an ini setting which limits extensions
to be loaded from a particular directory, presumably as a safety
precaution.

The Sqlite3 extension uses the code:

sqlite3_enable_load_extension(sqlite_handle, 1);

which affects both the C api and loading extensions through SQL code.

However, in the proposed PdoSqlite class this code:

sqlite3_db_config(db, SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION, 1);

is used to temporarily enable extension loading, which only enables it
through the C api.

As that means that SQLite extensions can only be loaded through C code
(not through SQL), and if someone can upload and execute code to your
server, your server is compromised anyway, having to edit ini files to
enable extension loading, seems like a bad tradeoff.

Thoughts?

cheers
Dan
Ack

10 months ago by Larry Garfield — view source

unread

Hi everyone,

Just giving an update on the
https://wiki.php.net/rfc/pdo_driver_specific_subclasses RFC as time is
running out. The RFC text has been updated with the implemented
subclasses stubs.

The RFC doesn't specify if new PDO(...) changes behavior at all. Currently, as I understand it, $db = new PDO($postgresConnectString) returns a PDO instance that has the Postgres-specific methods enabled, which they can use. With this RFC, if someone does the same, will they now get back PdoPgsql, or will PDO still have the postgres methods, or will their code break?

--Larry Garfield

10 months ago by Dan Ackroyd — view source

unread

The RFC doesn't specify if new PDO(...) changes behavior at all.

That behaviour is not changed at all.

will PDO still have the postgres methods

Yes, until someone does another RFC to deprecate and remove them.

if someone does [new PDO(...)], will they now get back PdoPgsql,

No.

New`ing one object, and getting a different object back would be far
too surprising to even be contemplated.

cheers
Dan
Ack

10 months ago by Larry Garfield — view source

unread

The RFC doesn't specify if new PDO(...) changes behavior at all.

That behaviour is not changed at all.

will PDO still have the postgres methods

Yes, until someone does another RFC to deprecate and remove them.

if someone does [new PDO(...)], will they now get back PdoPgsql,

No.

New`ing one object, and getting a different object back would be far
too surprising to even be contemplated.

cheers
Dan
Ack

OK, thanks. I'm fine with that approach, but it would be good to make it explicit, and probably add a Future Scope for "we should probably remove the base versions eventually in a few versions" or something.

--Larry Garfield

9 months ago by BohwaZ — view source

unread

As that means that SQLite extensions can only be loaded through C code
(not through SQL), and if someone can upload and execute code to your
server, your server is compromised anyway, having to edit ini files to
enable extension loading, seems like a bad tradeoff.

I'm sorry to disagree, but changing this would be a bad idea for
security.

Imagine this scenario:

user can execute SQL statements (that's the case in my app: users
can execute read-only statements, this is by design)
user can upload files

I'm quite sure that I never want users to be able to load any
extension through SQL, or it would mean trouble :(

Also, we have recently see malicious extensions able to escape SQLite3
and gain shell access... Sounds bad.

So just like in the SQLite3 extension, extension loading should be
limited to using the PHP method, not SQL. And we should be able to
restrict which extensions can be loaded, at the server level
(PHP_INI_SYSTEM), as in mass hosting we don't want hosted sites to be
able to load any extension.

For other points of the RFC:

forcing to UTF8 is OK for me, I don't think anyone ever requested
being able to do something different over the years for the SQLite3
extension.
it would be great to also have ::setAuthorizer in PDOSQLite :)
same for ::backup :)
not sure what would be the best for asserting if a statement is
read-only, to get the same as SQLite3Stmt::readOnly(), maybe
something like:
PDOStatement::getAttribute(PDOSQLite::READ_ONLY_STATEMENT) ?

Thanks for your work and time! It's great to finally see some progress
on PDO and custom driver features :)

9 months ago by Dan Ackroyd — view source

unread

I'm sorry to disagree, but changing this would be a bad idea for
security.

I'm quite sure that I never want users to be able to load any
extension through SQL, or it would mean trouble :(

So just like in the SQLite3 extension, extension loading should be
limited to using the PHP method, not SQL.

Yes?

I think you possibly misread my email.

Danack wrote:

As that means that SQLite extensions can only be loaded through C code
(not through SQL),

aka the Sqlite people improved the api around extension loading since
it was implemented in the Sqlite3 extension
https://www.sqlite.org/c3ref/load_extension.html

So the only way to load an extension would be through the PHP
loadExtension method, it wouldn't be possible to load one through SQL.

it would be great to also have

Sorry, I am in too much pain to look at those before the deadline.
There is always 8.4 https://www.youtube.com/watch?v=wccRif2DaGs .

cheers
Dan
Ack

9 months ago by BohwaZ — view source

unread

Yes?

I think you possibly misread my email.

Ah! sorry I understood the opposite, that you were wondering about
letting people load extensions from SQL. Great that we are on the same
note :)

Sorry, I am in too much pain to look at those before the deadline.
There is always 8.4 https://www.youtube.com/watch?v=wccRif2DaGs .

No worries, what you did is already awesome :) If I myself get any free
time I'll look at porting my code from the sqlite3 extension to PDO.