Implemented request #64810 (impossible to load extension via pdo_sqlite) #3368

cwt137 · 2018-07-05T01:50:57Z

Hello. This is an attempt to fix https://bugs.php.net/bug.php?id=64810 . This allows PDO to load extensions just like the native Sqlite3 API can do. This means there is more feature parity between the two APIs.

A couple of tests are failing. Could I please have some help resolving it?

cwt137 · 2018-07-06T14:17:54Z

Test and stuff finally pass.

cwt137 · 2018-07-10T14:01:53Z

I changed branch to master so that this can be included in PHP 7.3. I also updated the title so it is hopefully more appropriate. I hear the feature freeze is on the 17^th. What can I do to make sure this makes the deadline?

BenMorel · 2018-07-10T14:07:11Z

Would love to see this in PHP 7.3. Ping @cmb69 and @smalyshev !

cmb69 · 2018-07-10T15:15:27Z

This PR is similar to #2698, and as such should at least be discussed on internals@.

smalyshev · 2018-07-10T18:58:37Z

Loading binaries at runtime creates all kinds of tricky situations IMHO, even in single-threaded servers. Does it really needs to be happen at runtime? Maybe have INI value that pre-loads all needed extensions would be better?

BenMorel · 2018-07-10T21:13:26Z

How is that different from SQLite3::loadExtension()?

smalyshev · 2018-07-10T22:56:04Z

indeed, not very different. So I guess if PDO maintainers are fine with it, then ok, but since it's in PDO probably needs to be discussed.

cwt137 · 2018-07-11T01:05:10Z

@cmb69 This is totally different than that PR. That PR is about getting blob data from a result as a stream? This PR is basically to add functionality to SQLite via its extensions system. The only thing similar is that both PRs add a sqlite instance method onto the PDO class.

@BenMorel This PR implements the same functionality as SQLite3::loadExtension(), just for PDO. You can't use PDO and that method together since you didn't open the database using the SQLite3 API. I am trying to make sure that I can use the same features in PDO as I could with SQLite3, so I coded this PR to have the same behaviour. Its a pain to use Laravel and have all the DB setup and then you have to use the SQLite3 API. Makes no sense

@smalyshev It shouldn't be an ini setting because on every request that uses pdo_sqlite would load the extension. Also, you might want to load a different extension, depending on what the request is. This is why I think the people who created the original method for the SQLite3 API implemented it this way. I am trying to make sure PDO has the same functionality as SQLite3.

BenMorel · 2018-07-11T05:24:19Z

@cwt137 I know, I was reacting to @smalyshev's comment.

However, I just realized that your PR adds a sqliteLoadExtension() method to PDO, which makes it similar in this matter to #2698 indeed (this is what @cmb69 meant), and IMHO is not a good thing to do: PDO should stay as generic as possible.

Is there any reason why you couldn't just add support for SELECT LOAD_EXTENSION(), which is what the original PHP bug is about, and which wouldn't require a new PDO method?

cwt137 · 2018-07-11T14:49:58Z

Nobody knows why the select statement doesn’t work. Is it a problem with PHP or SQLite? Who knows. So, I provided a solution that would make it easy for people to transition to PDO.

…

On Jul 10, 2018, at 10:25 PM, Benjamin Morel ***@***.***> wrote: @cwt137 I know, I was reacting to @smalyshev's comment. However, I just realized that your PR adds a sqliteLoadExtension() method to PDO, which makes it similar in this matter to #2698 indeed (this is what @cmb69 meant), and IMHO is not a good thing to do: PDO should stay as generic as possible. Is there any reason why you couldn't just add support for SELECT LOAD_EXTENSION(), which is what the original PHP bug is about, and which wouldn't require a new PDO method? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cmb69 · 2018-07-11T15:01:33Z

SELECT LOAD_EXTENSION() doesn't work, because extension loading is disabled by default. The PR (as well as the respective code in ext/sqlite3) enables it only temporary. See also https://www.sqlite.org/c3ref/enable_load_extension.html.

BenMorel · 2018-07-12T10:12:09Z

What about creating an ini option to enable SQLite LOAD EXTENSION in PDO then? Something like:

pdo_sqlite.enable_load_extension = 1

This would be off by default to keep the current behaviour.

This would avoid having to add a SQLite-specific method to PDO (which again, IMO is not a good idea), and we could use SELECT LOAD_EXTENSION() in client code instead.

BenMorel · 2018-07-12T10:25:38Z

Just noficed that the pdo_sqlite extension already adds 3 methods to PDO; I didn't know that. These methods are explicitly marked as EXPERIMENTAL though, and can be modified (and removed, perhaps?) at any time, without notice.

So I stand with my suggestion above, ini option pdo_sqlite.enable_load_extension, which I feel is more likely to be accepted by the community, and more likely to stay in future releases.

cwt137 · 2018-07-12T10:47:29Z

I forgot about this function. Like you said, the ability to load a SQLite extention is turned on temporarily. This is for security reasons. Turning on the loading of extensions could be done on initializing pdo_sqlite, but a hacker could load an extension using the select statement if they used SQL injection.

BenMorel · 2018-07-12T17:46:03Z

To me, SQL injection is a thing of the past. If you're using prepared statements, you should never ever have to build a query string from a user-originated variable, so this is a non-issue.

This is just my opinion, of course.

# Conflicts: # ext/pdo_sqlite/config.m4 # ext/pdo_sqlite/config.w32

cwt137 · 2018-10-29T01:11:28Z

@BenNorel I agree with you 100%! Everybody should be parameterizing their SQL. But unfortunately it is not the case and it is the #1 security hole/threat/whatever according to OWASP. Also, I do not think it is a good idea removing the current 3 PDO methods for SQLite. They provide a valuable purpose and because of the unique nature of SQLite, there is no equivalent in other SQL databases. For example, in SQLite, you create your own user defined function (UDF) in PHP and have the database use it. But, the UDF does not run on the database, but runs in the programming language that created the UDF. Here is one big usecase; because the default build of SQLite does not have any trigonometric functions, you must use PHP to create a UDF to use inside of PHP. This is accomplished by using one of the 3 extra PDO methods. Here is an example.

BenMorel · 2018-10-29T14:32:57Z

You have a point regarding the necessity of keeping the current PDO methods for SQLite!

Now, I honestly don't mind which solution is retained, as long as LOAD_EXTENSION() is possible in some way.

I'm just standing up by my comment above, that SQL injection should not be a reason not to add useful functionality in a library. Anyone not using prepared statements and blindly injecting user input into their SQL queries is asking for trouble anyway. When you can already fiddle with, or drop their database, I don't care if you can, also, execute abitrary extensions on their server.

teresko · 2019-04-08T09:44:08Z

So, has there been any progress regarding this feature?
Or is everyone waiting for branch to get stale enough :)

beberlei · 2019-04-08T09:47:17Z

@BenMorel i strongly disagree on a global INI option to enable loading extensions, potentially everywhere in every PDO::query call. Stating "sql injection is a thing of the past" is also quite dangerous, given that it is still in the top 10 of OWASP list, it is very much a thing of the present.

PDOs API is weird and different drivers add different things to the class. This is not restricted to sqlite driver and I don't think taking a stand now on preventing further changes to this weird API makes sense.

beberlei · 2019-04-08T10:08:49Z

The PR needs a rebase onto 7.4 or master, since the ZTS threadsafe handling in sqlite extensions is not present anymore.

nikic

I think this should be okay in this form for 7.4. As has been pointed out, there are already multiple similar methods provided by PDO SQLite and the API closely matches what the sqlite3 extension does. I agree with @beberlei that having this contained API is better than a global ini setting, especially in light of the recent introduction of the defensive setting for sqlite3.

nikic · 2019-04-08T10:04:22Z

ext/pdo_sqlite/config.m4

@@ -66,6 +66,10 @@ if test "$PHP_PDO_SQLITE" != "no"; then
  PHP_CHECK_LIBRARY(sqlite3,sqlite3_key,[
    AC_DEFINE(HAVE_SQLITE3_KEY,1, [have commercial sqlite3 with crypto support])
  ])
+  PHP_CHECK_LIBRARY(sqlite3,sqlite3_load_extension,
+    [],
+    [AC_DEFINE(SQLITE_OMIT_LOAD_EXTENSION, 1, [have sqlite3 with extension support])


This is a bit odd, we'd usually go for a positive define instead. HAVE_SQLITE3_LOAD_EXTENSION would be typical.

nikic · 2019-04-08T10:06:33Z

ext/pdo_sqlite/config.w32

@@ -4,6 +4,7 @@ ARG_WITH("pdo-sqlite", "for pdo_sqlite support", "no");

 if (PHP_PDO_SQLITE != "no") {
 	if (SETUP_SQLITE3("pdo_sqlite", PHP_PDO_SQLITE, PHP_PDO_SQLITE_SHARED)) {
+		ADD_FLAG('CFLAGS_PDO_SQLITE', "/D SQLITE_THREADSAFE=" + (PHP_ZTS == "yes" ? "1" : "0") + " ");


Not sure I understand the purpose of this change. As we no longer bundle libsqlite3 nowadays, does this even have an effect?

BenMorel · 2019-04-08T11:45:40Z

@BenMorel i strongly disagree on a global INI option to enable loading extensions, potentially everywhere in every PDO::query call. Stating "sql injection is a thing of the past" is also quite dangerous, given that it is still in the top 10 of OWASP list, it is very much a thing of the present.

That's a crazy state of things in 2019, probably due to a lot of legacy code, and poor developer training.

PDOs API is weird and different drivers add different things to the class. This is not restricted to sqlite driver and I don't think taking a stand now on preventing further changes to this weird API makes sense.

I guess you're right. Either way to implement it will be good for me anyway! 👍

RollingHog · 2020-05-07T19:59:59Z

Once again, what was the solution? Feature still seems to be non-existent, at least in PHP documentation.

cmb69 · 2020-05-08T09:34:50Z

This PR is still unresolved. The problem is that adding driver specific methods to PDO via the currently existing mechanism (basicall __call like), has faced some strong oposition, so this should be discussed on internals. It probably would make sense to start some general discussion about this issue, and to hopefully come to a solution, so driver specific functionality could be added to PDO in the future.

RollingHog · 2020-05-12T07:45:23Z

Uh-oh. From one point, I do understand concern about "polluting the function space". From another, I kinda really need it (and some other people from SO too, I guess). Maybe they would agree on something like callDriverSpecific('functionName', arguments...)? It looks awful, but at least it can work.

aboks · 2020-05-28T05:49:53Z

I found a workaround for this functionality not being present yet, but it involves a fair amount of spooky black magic and I definitely wouldn't recommend it for production.

krakjoe · 2021-05-10T11:34:14Z

This appears to have gone stale, and there appears to be more recent work going on in this area, so closing now.

BenMorel · 2021-05-10T13:48:30Z

@krakjoe This is still not solved as far as I can tell, though. Could you please point me to another issue / PR that would deal with this?

cmb69 · 2021-05-10T14:08:51Z

I'm afraid there is no recent work going on in this area. This needs a general solution regarding driver specific functionality; otherwise such feature additions are blocked indefinitely. :(

cmb69 · 2021-05-10T14:26:30Z

@BenMorel, please raise this as general issue on the internals ML – we really need a general solution (do it like it has done before; driver specific subclasses; helper classes; whatever).

krakjoe · 2021-05-10T14:44:35Z

My apologies, at a glance, it looks like 5765 is related and more recent, but I see it's not really going anywhere either ...

cmb69 · 2021-05-11T09:56:58Z

Oh, indeed, PR #5765 tries to adress this isseu in a general way. Driver objects might be the way to go to reduce BC issues.

pbowyer · 2021-07-07T14:34:20Z

@krakjoe I'd like #5765 to address this issue but concluded my C/PHP internals skills are not up to the task.

cwt137 added 3 commits July 4, 2018 18:43

Initial code to implement PDO::sqliteLoadExtension

f4295ab

Fixed unit tests and compilation error

c0a294f

More compilation fixes

4cdba49

nikic added the Feature label Jul 7, 2018

cwt137 changed the base branch from PHP-7.2 to master July 9, 2018 20:23

cwt137 changed the title ~~Implement PDO::sqliteLoadExtension()~~ Implemented request #64810 (impossible to load extension via pdo_sqlite) Jul 9, 2018

Merge branch 'master' into pdo_sqlite-loadExtension

fd97d00

tenken mentioned this pull request Aug 31, 2018

simple way to get full sqlite extension support? lando/lando#1161

Closed

tylergets mentioned this pull request Oct 24, 2018

Sqlite is missing json1 support docker-library/php#731

Closed

Merge branch 'master' into pdo_sqlite-loadExtension

cfc704e

# Conflicts: # ext/pdo_sqlite/config.m4 # ext/pdo_sqlite/config.w32

nikic reviewed Apr 8, 2019

View reviewed changes

pbowyer mentioned this pull request Jun 24, 2020

PDO and driver specific methods #5765

Draft

10 tasks

krakjoe closed this May 10, 2021

This was referenced Jun 3, 2022

Check details of a bug and PR. Imagick/php-src#2

Closed

DB specific functions to add Imagick/php-src#9

Closed

Implemented request #64810 (impossible to load extension via pdo_sqlite) #3368

Implemented request #64810 (impossible to load extension via pdo_sqlite) #3368

Conversation

cwt137 commented Jul 5, 2018

cwt137 commented Jul 6, 2018

cwt137 commented Jul 10, 2018

BenMorel commented Jul 10, 2018

cmb69 commented Jul 10, 2018

smalyshev commented Jul 10, 2018

BenMorel commented Jul 10, 2018

smalyshev commented Jul 10, 2018

cwt137 commented Jul 11, 2018

BenMorel commented Jul 11, 2018

cwt137 commented Jul 11, 2018 via email

cmb69 commented Jul 11, 2018

BenMorel commented Jul 12, 2018

BenMorel commented Jul 12, 2018

cwt137 commented Jul 12, 2018

BenMorel commented Jul 12, 2018

cwt137 commented Oct 29, 2018

BenMorel commented Oct 29, 2018

teresko commented Apr 8, 2019 • edited Loading

beberlei commented Apr 8, 2019

beberlei commented Apr 8, 2019

nikic left a comment

Choose a reason for hiding this comment

nikic Apr 8, 2019

Choose a reason for hiding this comment

nikic Apr 8, 2019

Choose a reason for hiding this comment

BenMorel commented Apr 8, 2019

RollingHog commented May 7, 2020

cmb69 commented May 8, 2020

RollingHog commented May 12, 2020 • edited Loading

aboks commented May 28, 2020

krakjoe commented May 10, 2021

BenMorel commented May 10, 2021

cmb69 commented May 10, 2021

cmb69 commented May 10, 2021

krakjoe commented May 10, 2021

cmb69 commented May 11, 2021

pbowyer commented Jul 7, 2021

teresko commented Apr 8, 2019 •

edited

Loading

RollingHog commented May 12, 2020 •

edited

Loading