WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11608 closed defect (bug) (fixed)

wpdb->prepare() is broken

Reported by: hakre Owned by: ryan
Milestone: 3.0 Priority: normal
Severity: critical Version: 2.9
Component: Database Keywords: has-patch tested dev-feedback featured
Focuses: Cc:

Description

the wpdb->prepare() statement plays an utterly important role in database access. This function is not properly implemented. To name it correctly, this function is more or less a wrapper for sprintf / vsprintf which adds some fuzz in the proxy.

Tickets like #11318 pointed to structural problems. Eventhough tricky devs like DD32 can do it working for them it's a plain oversight that data gets manipulated by that function that might render safe queries unsafe and therefore actually opens the gate for sql injections instead of closing them.

Example: CONST = 'percentage stupid or %stupid is the question'

even following the rules to act vsprintf / sprintf (like documented in code) will run you into problems:

Example: CONST = 'percentage stupid or %%stupid is the question'

Please stop this madness and create a ->prepare function that works solidly.

Attachments (10)

11608.diff (665 bytes) - added by dd32 4 years ago.
11608.patch (3.2 KB) - added by hakre 4 years ago.
Unneded code (by definition) moved out, compacted the initial docblock and added a note that this has nothing to do with prepared statements.
11608.2.diff (1.7 KB) - added by miqrogroove 4 years ago.
Fix phpdoc. prepare() can never be called statically because dbh would have no value. Add %-encoding explanation.
11608.3.patch (4.3 KB) - added by hakre 4 years ago.
11608.4.patch (5.4 KB) - added by hakre 4 years ago.
Code is notice-free now, fixes in print_error in case there was no last_query.
11608-tests.php (2.5 KB) - added by hakre 4 years ago.
Testcases
11608-rev2.patch (6.5 KB) - added by hakre 4 years ago.
11608-tests.2.php (5.6 KB) - added by hakre 4 years ago.
Updated Testcases for rev 3
11608-rev3.patch (7.3 KB) - added by hakre 4 years ago.
11608.3.diff (1.0 KB) - added by Denis-de-Bernardy 4 years ago.
double escape first approach (untested)

Download all attachments as: .zip

Change History (86)

comment:1 follow-up: dd324 years ago

  • Component changed from Security to Database
  • Milestone changed from 2.9.1 to Future Release
  • Priority changed from high to normal
  • Severity changed from critical to normal
  • Type changed from defect (bug) to feature request

Settign to feature request and Future release due to nature of the request, In the event that someone takes it on to write a 'better' prepare parser, or a deve feels like it needs changing, it can be brought forward into the current release.

Priority and Severity: It works securely at present if the basic printf rules are followed.

Can you please supply some examples of what doesnt work? What problems are run into when using '%%' for example?

The data being passed into the function may contain whatever it wants, That doesnt affect the parser, The only location where you have to be careful, Is the Query itself, If you require to use '%' in there, then it needs to be escaped properly, That is the only time it should cause an error.

comment:2 miqrogroove4 years ago

$wpdb->prepare("SELECT 1 WHERE table.row LIKE '%stupid' AND othertable.row = %s", $input)

Now you might have a problem.

comment:3 dd324 years ago

Now you might have a problem.

Yes, And escaping it correctly will resolve that. You need to follow the same rules that sprintf() would require, As specified in the docs.

What is your preferred alternative? Any ideas of how it should work underneath? or on top?

comment:4 miqrogroove4 years ago

As specified in the docs.

Woah, in what docs? If I had seen something in wpdb or Codex that said "::prepare is intolerant of LIKE conditions except..." then I would have fixed my code and been on my merry way.

comment:5 miqrogroove4 years ago

Here's where you really get into 1337...

$stringme = array(1,2,3,4,5);
var_dump(vsprintf("SELECT 1 WHERE table.row LIKE '%stupid' AND othertable.row = %s", $stringme));

Make the mistake of passing an array into wpdb::prepare; you are now pwn3d.

comment:6 miqrogroove4 years ago

So the worst case scenario is something like this:

$wpdb->prepare("SELECT 1 WHERE table.row LIKE '%stupid' AND othertable.row = %s", $_GET['thisissafetodolol'])

comment:7 miqrogroove4 years ago

This one is not a security issue, but definitely fails to parse:

"SELECT 1 WHERE table.row LIKE '%%stupid' AND othertable.row = %s"

comment:8 dd324 years ago

OK, Now i get what your problem is.

Would've been a LOT easier to just point out that %%s.. gets mangled by the parser, and as a result doesnt work as intended.

Woah, in what docs?

The PHPDocs, specifically where it states it supports a subset of the sprintf function and to see its syntax for help.

comment:9 miqrogroove4 years ago

Now it's a security issue:

$wpdb->prepare("SELECT 1 WHERE table.row LIKE '%%stupidisas%%stupiddoes' AND othertable.row = %s", $_GET['thisisalsosafetodo'])

Enjoy

dd324 years ago

comment:10 follow-ups: dd324 years ago

  • Keywords has-patch added; needs-patch removed

attachment 11608.diff added

  • Avoid quoting pre-escaped placement holders

While that is a security risk, Its also pretty hard to exploit due to vsprintf throwing its hands up at the mis-matched arguements, some basic sanitization of your input data would also help prevent it.

comment:11 miqrogroove4 years ago

  • Keywords tested added; dev-feedback removed

some basic sanitization of your input data would also help prevent it

Basic sanitization, such as using prepare()? _

comment:12 in reply to: ↑ 1 hakre4 years ago

Replying to dd32:

Priority and Severity: It works securely at present if the basic printf rules are followed.

False prediction.

Can you please supply some examples of what doesnt work? What problems are run into when using '%%' for example?

Execute and read the code to see for yourself. There is nothing better than the own reception. As you wrote in the other ticket you are not properly getting the whole view, so a little playing around won't be wrong I assume.

The data being passed into the function may contain whatever it wants, That doesnt affect the parser, The only location where you have to be careful, Is the Query itself, If you require to use '%' in there, then it needs to be escaped properly, That is the only time it should cause an error.

Per definition query is data passed into the function.

comment:13 in reply to: ↑ 10 hakre4 years ago

Replying to dd32:

attachment 11608.diff added

  • Avoid quoting pre-escaped placement holders

While that is a security risk, Its also pretty hard to exploit due to vsprintf throwing its hands up at the mis-matched arguements, some basic sanitization of your input data would also help prevent it.

congrats making it even more complicated. you should chill this down instead of thrilling this up. this is a step away from your first reaction to say: this needs to be fixed properly with a bitter feeling.

comment:14 dd324 years ago

  • Milestone changed from Future Release to 3.0
  • Type changed from feature request to defect (bug)

Execute and read the code to see for yourself. There is nothing better than the own reception.

Using %% does work for me. Which was the request for more information, It had not clicked that using %%s specifically caused the error.

Per definition query is data passed into the function.

Yes, By definition. But in this case, The data is the query params, I'm talking on a code level, not a syntax level.

congrats making it even more complicated.

It fixes the current issue with the current parser which will be used in 2.8.x and 2.9.x for awhile yet. Whilst leaving it open for someone to suggest a replacement.

comment:15 miqrogroove4 years ago

It fixes the current issue with the current parser

Does not fix my 2nd and 3rd examples above. They are oddball cases but totally exploitable if they exist.

comment:16 hakre4 years ago

Replying to dd32:

Per definition query is data passed into the function.

Yes, By definition. But in this case, The data is the query params, I'm talking on a code level, not a syntax level.

I'm I/O-talking. function: pass something into it, get something back. If I move all my %s's into the data-parameter-on-your-code-level, then I would not need to actually use prepare. Then on your code level I could use vsprintf directly. Just to give you the idea where such argumentation leads to. But this is getting too far for this ticket.

congrats making it even more complicated.

It fixes the current issue with the current parser which will be used in 2.8.x and 2.9.x for awhile yet. Whilst leaving it open for someone to suggest a replacement.

I have the feeling that the regex does not solve the problem acutally but shift it, but I'll review that.


The requested documentation can be found here: Protect Queries Against SQL Injection Attacks.

For those who want to get a broader view on the topic and why more delicate stuff can happen by accident (it's not only LIKE which is using %-tokens), can take a look into this ticket and search for WEEK: #10397. This is by accident only, I assume that if the whole core code is reviewed to replace standard queries with variable substitution into the prepare statement ones, this will get us more findings.

hakre4 years ago

Unneded code (by definition) moved out, compacted the initial docblock and added a note that this has nothing to do with prepared statements.

comment:17 hakre4 years ago

Regarding the look for a "better" implemenation, maybe it should be considere to actually use prepared statements? I heard, those have been already implemented and they are working quite alright. Well this is PHP5 and therefore unfourtionatly not compatible with wordpress but at least I would have made sense to just orient on the principles which would help to implement that lib later on... .

comment:18 follow-up: dd324 years ago

Does not fix my 2nd and 3rd examples above. They are oddball cases but totally exploitable if they exist.

Which cases specifically? Passing an array is not supported by the function, Nor is passing non-sprintf-escaped SQL's. My patch is aimed at fixing the current bug in the current parser, Not to replace it with something that can read your mind as to what placement marker in the string you intended to be replace.

Regarding the look for a "better" implemenation, maybe it should be considere to actually use prepared statements?

When WordPress utilises the MySQLi extension rather than the MySQL extension, then sure.

comment:19 miqrogroove4 years ago

Passing an array is not supported by the function

If by not supported you mean "It's secure except we pay no attention to the existence of arrays" then that sounds about right.

comment:20 dd324 years ago

If by not supported you mean "It's secure except we pay no attention to the existence of arrays" then that sounds about right.

Garbage in, Garbage out.

But i have to eat my words.

You *can* pass an array of data. as in, $wpdb->prepare("sql..", array('replacement1', 2, '3')); - Theres another ticket related to this #11102

comment:21 miqrogroove4 years ago

Unneded code (by definition) moved out

Except that would break any query with quoted variables in it. There could be many plugins relying on that behavior.

comment:22 sirzooro4 years ago

  • Cc sirzooro added

comment:23 Denis-de-Bernardy4 years ago

  • Cc Denis-de-Bernardy added

comment:24 in reply to: ↑ 18 Denis-de-Bernardy4 years ago

Replying to dd32:

Regarding the look for a "better" implemenation, maybe it should be considere to actually use prepared statements?

When WordPress utilises the MySQLi extension rather than the MySQL extension, then sure.

I'd -1 that, personally, if we ever consider it. What's really needed is something equivalent to PDO::ATTR_EMULATE_PREPARES

A true prepared statement would be very bad, because it would do a return trip to the server for every query, and the server would actually prepare the statement (in the SQL sense). That is, it would come up with a query plan that works with whichever unknown variables, rather than one that is perfectly optimized for the constant values we give it.

comment:25 follow-up: miqrogroove4 years ago

I think the bottom line is that the printf syntax was a poor choice for query parsing. '%' is syntactically ambiguous in wpdb::prepare() unless you assume that every query author also knows how to use the '%%' syntax, and every server has applied the dd32 diff file from above. That's a fairly impossible standard to meet.

So I'll say it for a third time, both the wpdb class and the Codex page need to explain how this function was intended to work. Hiding behind the dogma of "it's a prepare function that works like sprintf" is a big mistake. The '%%' syntax is not explicitly defined in the PHP function description. It only shows up 3 pages below the fold in the fifth example where it says, "notice the double %%". Before creating the previous ticket, I personally read the wpdb class, the Codex page, and the PHP function description and parameters list, and I still didn't understand why prepare() was broken until dd32 explained it to me. I've been writing various flavors of SQL for over 10 years, and I've never had to escape an intentional wildcard like this.

comment:26 follow-up: Denis-de-Bernardy4 years ago

In retrospect we should have used the syntax that is accepted by PDO for this stuff, i.e. either of:

"SELECT * FROM foo WHERE bar = ?" # ? gets replaced by first variable
"SELECT * FROM foo WHERE bar = :bar" # :bar gets replaced by variable named bar

comment:27 Denis-de-Bernardy4 years ago

  • Keywords tested removed

also, I believe the patch leads to double quoting of quoted %s. lines 563 and 564 should be left untouched.

$wpdb->prepare("'%s'"); // ''%s''

comment:28 miqrogroove4 years ago

  • Keywords tested added

I think you're reading the wrong file ;)

comment:29 Denis-de-Bernardy4 years ago

yeah. I was reading the one from hakre. :-)

comment:30 in reply to: ↑ 26 sirzooro4 years ago

Replying to Denis-de-Bernardy:

In retrospect we should have used the syntax that is accepted by PDO for this stuff, i.e. either of:

"SELECT * FROM foo WHERE bar = ?" # ? gets replaced by first variable
"SELECT * FROM foo WHERE bar = :bar" # :bar gets replaced by variable named bar

We can consider adding new function prepare2() which will use PDO-like syntax, and mark prepare() as deprecated. This will require a lot of work (quick search showed 43 files), but we will avoid such problems in the future. This can be done under another ticket.

comment:31 nacin4 years ago

  • Keywords dev-feedback added; tested removed

comment:32 miqrogroove4 years ago

new function prepare2() which will use PDO-like syntax

If you're going to re-invent the wheel, query parsing seems like a bad approach in general, and that particular syntax improvement offers very marginal benefit above the needed documentation.

comment:33 in reply to: ↑ 25 hakre4 years ago

Replying to miqrogroove:

The '%%' syntax is not explicitly defined in the PHP function description. It only shows up 3 pages below the fold in the fifth example where it says ...

You're wrong, it's documented clearly upfront in the description of the format parameter (as the first possible type even):

  1. A type specifier that says ...
  • % - a literal percent character. No argument is required.

Replying to Denis-de-Bernardy:

also, I believe the patch leads to double quoting of quoted %s. lines 563 and 564 should be left untouched.

$wpdb->prepare("'%s'"); // ''%s''

%s will get quoted by design (reference to vsprintf), quoted %s will get double quoted. so quoted %s is a misconcept. by definition %s are getting quoted and must not be quoted by the user. I removed the fuzz out of the prepare function. That was by intention to have that out. Do not train users writing bad queries for that function.


Replying to sirzooro:

We can consider adding new function prepare2() which will use PDO-like syntax, and mark prepare() as deprecated. [...]

I think it's a good approach to shift away from the broken design.


Regex suggestion by DD32 fails on strings like:

%%%s

which should expand to

%%'string value'

but which aren't any longer. as I assumed, the regex does not help here. For those arguing that is not a valid SQL clause or not or valid sprtinf format string: This is about USER-INPUT we handle here and this must be handeled strict and safe. We should not be talking about workarounds (only) when it comes to safety and security but how to solve things from the ground up.

miqrogroove4 years ago

Fix phpdoc. prepare() can never be called statically because dbh would have no value. Add %-encoding explanation.

comment:34 hakre4 years ago

Just a short note:
This function only supports a small subset of the sprintf syntax; it only supports %d (decimal number), %s (string).
can be extended to:
This function only supports a small subset of the sprintf syntax; it only supports %d (decimal number), %s (string) and %% (a literal percent character).

As the sprintf PHP documentation does. Thanks for taking care everybody. It's pretty much summing up here.

comment:35 follow-up: miqrogroove4 years ago

Replying to hakre:

You're wrong,

  1. A type specifier that says ...
  • % - a literal percent character. No argument is required.

No, the documentation is wrong. As illustrated in the previous ticket, bare % characters cause vsprintf to return bool(false).

comment:36 follow-up: miqrogroove4 years ago

Why on Earth would anyone want "%%'string value'" in a query?

comment:37 in reply to: ↑ 35 Denis-de-Bernardy4 years ago

Replying to miqrogroove:

Replying to hakre:

You're wrong,

  1. A type specifier that says ...
  • % - a literal percent character. No argument is required.

No, the documentation is wrong.

No no, it's not. A % followed by a % returns a % sign. It reads quite clear to me, but that's not the point of the ticket. The point here is to fix wpdb->prepare().

comment:38 in reply to: ↑ 36 hakre4 years ago

Just tested 11608.2.diff, it still fails on it's own defintion:

%%%s

which should expand to

%%'string value'

does not work. you can not use regex to parse for the format tokens, looks broken to me.

comment:39 hakre4 years ago

corrected :

Just tested 11608.2.diff, it still fails on it's own defintion:

%%%s

which should expand to

%'string value'

does not work. you can not use regex to parse for the format tokens, looks broken to me.

comment:40 follow-up: miqrogroove4 years ago

@Denis It's a semantic nuance. It literally says %% is treated as a % literal. It doesn't say that a bare % is not treated as a % literal, or that %' is not treated as %', or that all % literals must be converted to %%. Among those phrases, they chose the most ambiguous one ;)

@hakre Please explain what you're getting at? Why in the world would %%'string value' ever appear in a query?

comment:41 in reply to: ↑ 40 sirzooro4 years ago

I have just created ticked #11622 in order to discuss new prepare2() function with PDO-like syntax.

comment:42 dd324 years ago

does not work. you can not use regex to parse for the format tokens, looks broken to me.

it is NEVER going to support every single oddball case you want to throw at it. You cannot use regex to create an efficient parser to combat every single item. I'm certainly not the first to admit that, And whilst its used elsewhere in WordPress, Its not needed here to parse a database query..

That does however fix a bug, whilst not introducing more, It mearly misses other cases which are pretty hard to pick up, and incredibly rare.

hakre4 years ago

comment:43 hakre4 years ago

New patch with a function that does what is announced: To simply replace %s with %s while the three tokens %, d and s are allowed. Works syntactically correct.

Naturally this does NOT work for LIKE queries and other functions unless you use the full parameter because it will substitue %s with a single quoted value (so for this tickets scope not all of [otherwise valid] hints by miqrogroove do correctly apply here). The user still needs to take care to build valid SQL on it's own. But this time the function does what it said it will do, enabling proper use for various MYSQL functions that need to have parameters containing %'s.

The new function provides a strict mode when configured which means that it would be even possible to snytactically validate the $query throwed into the public function wpdb->prepare(). In case you like it hot, you should not use that feature for shure :). (WPDEBUG maybe to signal stuff here?).

Please keep in mind that this ticket is about the actual data passed by the user as query, not the parameters (so not an escaping issue here for the acutal data in parameters, see comment in last paragraph as well).

Since it was criticised that I moved out the child-safety fuzz-logic to handle "double quotings by accident (???)", I moved them after the single-quote insertion which makes that more stable as well.

Tests (from the attached testcase, the new function only not the prepare function):

   %         -> %          (Syntax: Error)
   %%        -> %%         (Syntax: Ok)
   %s        -> '%s'       (Syntax: Ok)
   %%s       -> %%s        (Syntax: Ok)
   %%%s      -> %%'%s'     (Syntax: Ok)
   %d        -> %d         (Syntax: Ok)
   %-        -> %-         (Syntax: Error)
   %d%s%%    -> %d'%s'%%   (Syntax: Ok)
   %%d%s%%   -> %%d'%s'%%  (Syntax: Ok)
   %%%d%s%%  -> %%%d'%s'%% (Syntax: Ok)
   %%%d%%s%% -> %%%d%%s%%  (Syntax: Ok)
   %%d%%s%   -> %%d%%s%    (Syntax: Error)
   SELECT FROM t1 WHERE a LIKE (%s) -> SELECT FROM t1 WHERE a LIKE ('%s') (Syntax: Ok)
   SELECT FROM t1 WHERE a = %s -> SELECT FROM t1 WHERE a = '%s' (Syntax: Ok)
   SELECT 1 WHERE table.row LIKE '%stupid' AND othertable.row = %s -> SELECT 1 WHERE table.row LIKE ''%s'tupid' AND othertable.row = %s (Syntax: Ok)
   SELECT 1 WHERE table.row LIKE '%stupid' AND othertable.row = %s -> SELECT 1 WHERE table.row LIKE ''%s'tupid' AND othertable.row = %s (Syntax: Ok)
   SELECT 1 WHERE table.row LIKE '%%stupid' AND othertable.row = %s -> SELECT 1 WHERE table.row LIKE '%%stupid' AND othertable.row = '%s' (Syntax: Ok)
   SELECT 1 WHERE table.row LIKE '%%stupidisas%%stupiddoes' AND othertable.row = %s -> SELECT 1 WHERE table.row LIKE '%%stupidisas%%stupiddoes' AND othertable.row = '%s' (Syntax: Ok)

Have fun.

comment:44 hakre4 years ago

I liked the idea to have it logged in case wrong data is provided. Feedback welcome, I have no idea if that's the right way to do logging in WPDB.

comment:45 follow-up: miqrogroove4 years ago

String overflow: $query[++$i]

comment:46 in reply to: ↑ 45 hakre4 years ago

Replying to miqrogroove:

String overflow: $query[++$i]

What exactly do you mean by String overflow? I think you are more referring to C than to PHP.

comment:47 follow-up: westi4 years ago

  • Cc westi added
  • Keywords reporter-feedback added; has-patch dev-feedback removed

After reading through all the comments above I can not see a clear definition of the bug here that exists in $wpdb->prepare.

Most of what I see is incorrect usage of prepare itself instead.

You shouldn't be writing:

$wpdb->prepare("SELECT 1 WHERE table.row LIKE '%stupid' AND othertable.row = %s", $input)

but:

$wpdb->prepare("SELECT 1 WHERE table.row LIKE %s AND othertable.row = %s", $like, $other)

Can you summarise the exact bug with example queries where $wpdb->prepare used correctly has a bug /security issue.

comment:48 follow-up: miqrogroove4 years ago

What exactly do you mean by String overflow?

Well, "overrun" might be a more correct term. The array subscripts are exceeding the end of the string.

comment:49 miqrogroove4 years ago

Most of what I see is incorrect usage of prepare

Dear hackers, please pay no attention to incorrect usage. kthx.

comment:50 in reply to: ↑ 48 hakre4 years ago

Replying to miqrogroove:

What exactly do you mean by String overflow?

Well, "overrun" might be a more correct term. The array subscripts are exceeding the end of the string.

Regarding String overflow: $query[++$i] you were wrong but pointing in the right direction. Wrong, because there is no thing such string overflow or overrun in PHP, nor are that array subscripts. It's standard PHP string access, and even invalid seeming offsets (do not do that in C!) are actually possible (if you do not care for notices). Know the details.

But you were right because I did not tested boundary conditions well and your feedback made me aware of that. For example, the function with an empty string would do an iteration in the for loop which is not necessary. You're really making up your mind, thanks for the find and thanks for taking care! :)

hakre4 years ago

Code is notice-free now, fixes in print_error in case there was no last_query.

comment:51 hakre4 years ago

I've updated the patch (and testfile). Now properly boundary checked and Notice free.

A minor change has been made to WPDB->print_error() to reflect cases where there is no last_query parameter to prevent misleading output.

comment:52 in reply to: ↑ 47 hakre4 years ago

  • Keywords has-patch added; reporter-feedback removed

Replying to westi:

After reading through all the comments above I can not see a clear definition of the bug here that exists in $wpdb->prepare.

You're right, the clear definition of the bug is missing:

  1. Improper handling of input data in wpdb->prepare()
  2. Invalid docblock in wpdb->prepare()

The function just does not behave properly on input data. Often it's argumented that problems will be catched by vsprintf() in it, but that's just not the case (just one example: $wpdb->prepare( $query = '% wrong query', '' )).

I hope you share the point that wpdb->prepare() is an important function being interface to core coders and many plugin authors. Next to encouraging them to actually use that function, it should at least provide a certain level of stability and enough defensive potential that it can take up with the input by us and our beloved users.

It was not far ago I criticised code in the WPDB class (again) that it does not provide a secure level of data escaping. In that discussion Sivel pointed to wpdb->prepare() as the only function that provides a proper mysql escape via the classes public interface. I would like to see that one at least properly implemented.

The last paragraph is for background information regarding this ticket's title:

wpdb->prepare() is broken.

Flaws I saw so far:

  1. wpdb->prepare()'s docblock says it return NULL on error, actually that function does return FALSE and STRINGs on error as well. By returning STRINGs the user is not able to determine wether or not prepare failed on the query-pattern provided.
  2. wpdb->prepare()'s docblock does not properly document the substitution pattern, especially the usage of %%.
  3. wpdb->prepare() does not validate the $query parameter
  4. wpdb->prepare() passes unvalidated data to vsprintf().
  5. wpdb->prepare() ocasionally modifies the user's input on best guess (w/o being documented properly), by definition there is no need to do so.

Regarding 5.: I think this is a bug, to retain backwards compability it might be too late to take this out (ref. to DDB). I have at least improved that in my patch, but it does not solve that issue, so it is only more stable which is relative to say at least. Let's attribute this as hardening.

hakre4 years ago

Testcases

comment:53 in reply to: ↑ 10 hakre4 years ago

Oh, I forgot one (and it's an important one):

  1. wpdb->prepare() does not properly substititue '%%s'.

Regarding 6.: the fix-suggesting regex flavored alternative (ref. to dd32) doesn't properly substitute '%%%s'. Naturally such cases are properly handeled in my patch.

hakre4 years ago

comment:54 follow-up: westi4 years ago

I think we need to be pragmatic here.

If we need a better doc block, and I think we do, lets have one.

I am not convinced that we need to make the function try and resolve every incorrect usage and try and correct every possible programmer mistake.

The whole point of the code is that it should be simple, functional and fast.

The more complexity you add the slower it is going to be.

comment:55 in reply to: ↑ 54 hakre4 years ago

Just reviewed, my code has a bug and there are improvements for speed in prepare(), Westi gave me feedback via IRC that he likes to have that speed improved. What's often forgotten is that the @-error-surpress operator costs us a lot of speed. I will update to a new revision later.

comment:56 hakre4 years ago

Related: #11605

hakre4 years ago

Updated Testcases for rev 3

hakre4 years ago

comment:57 hakre4 years ago

Revision 3 out now. Fixed a bug in the quote function, added more testcases and we're now able to remove the @-operator for performance reasons. That will give us more speed, that @ operator is a real blocker.

Additionally I merged the improvements over from #11605 into this for even mroe speed in case a user wants to use escape.

comment:58 follow-up: Denis-de-Bernardy4 years ago

@hakre: I've been chewing on this since we discussed, and I'm really wondering if the approach is correct...

The issue with suppressing the @ operator is that it silences warning when more than the needed number of arguments are passed. That's probably a good thing,

Re the ticket in its entirely, I think that another, simpler possibility would be to initially double all % signs in the query, i.e.:

$query = str_replace('%', '%%', $query);
$query = str_replace(array('"%%s"', "'%%s'", '%%s'), '%s', $query);
$query = str_replace(array('"%%d"', "'%%d'", '%%d'), '%d', $query);

A few bugs would remain in odd edge cases (i.e. WHERE col LIKE '%something'); to cover those too, we actually need a regexp. Probably something like:

$query = str_replace('%', '%%', $query);
$query = preg_replace("/(\s)(['\"])?%%([sd])\\2(\s|$)/", "$1%$3$4", $query);

comment:59 miqrogroove4 years ago

initially double all % signs in the query

My brain just esploded.

comment:60 in reply to: ↑ 58 hakre4 years ago

Replying to Denis-de-Bernardy:

The issue with suppressing the @ operator is that it silences warning when more than the needed number of arguments are passed.

My patch already checks for the correct number of args in rev 3. That's why it was possible to remove that operator.

Re the ticket in its entirely, I think that another, simpler possibility would be to initially double all % signs in the query

Which is just bad practise. But what I can do is to auto-duplicate all %-signs which aren't valid format specifiers. But I think it's better to not filter the users input but to flag an error instead. Because the query-pattern then just is wrong. We can not try to find out what the users wants. I would really like to see the {%s} and {"'%s'"} cases ignored either. Thats a similar topic.

I know that if you read too much of the WP source code such ideas can come into mind because those approaches seem common but they are actually bad practice.

comment:61 hakre4 years ago

{%s} and {"'%s'"} in the last comment was destroyed because of bad formattings and stands for the double quoted cases: ''%s'' "'%s'"

Denis-de-Bernardy4 years ago

double escape first approach (untested)

comment:62 Denis-de-Bernardy4 years ago

I've uploaded a patch that uses the double approach. that being said, I think that no matter what we do, there will still be problematic strings with literal % characters. Example:

WHERE foo = 'a %s b'

comment:63 Denis-de-Bernardy4 years ago

  • Keywords dev-feedback added

Core dev feedback would be welcome on this ticket, btw. It's like, do we even want to try to fix part or all of the problem, and if we do, what is the favored approach?

comment:64 hakre4 years ago

Bah :) I suggest to keep it simple, keep the regexes out. No need to double escape the hell out of it, just exactly do what is announced is the save route to go. for example writing queries like:

WHERE foo = 'a %s b'

just shows that you do not have understood how to use prepare and that's it. Just a wrong input, it will create a syntactically wrong formatted query and that's it. %s should be consideres unquoted according the prepare documentation.

comment:65 hakre4 years ago

Related: #11102

comment:66 Denis-de-Bernardy4 years ago

Well, no, it's definitely not wrong. I think you merely misunderstood the example: if you're looking for "a %s b", as in *literally*, and you're expecting that it won't get sprintf'ed.

comment:67 hakre4 years ago

I found why that child-security is in there, it's for the class itself. The deeper you dig, the more you find:

$sql = "INSERT INTO `$table` (`" . implode( '`,`', $fields ) . "`) VALUES ('" . implode( "','", $formatted_fields ) . "')";

somewhere in function insert($table, $data, $format = null).

Now funny to read in (untouched) prepare this comment reagarding doubled quotes:

in case someone mistakenly already singlequoted it doublequote unquoting

wpdb::insert() teaches that this is not by mistake but by design. Just for the log.

comment:68 hakre4 years ago

Related: #6836

comment:69 hakre4 years ago

The birth of prepare() [5778]

comment:70 hakre4 years ago

based on revision 12601, changes as referenced / related in #11644 made bigger changes to the class because it's currently replaced with the hyper-db thingy from WPMU (which contains a lot of stuff not needed for standard WP installs).

comment:71 Denis-de-Bernardy4 years ago

  • Keywords bug-hunt added

comment:72 Denis-de-Bernardy4 years ago

  • Keywords featured added; bug-hunt removed

comment:73 miqrogroove4 years ago

  • Keywords tested added
  • Severity changed from normal to critical

+1 to 11608.diff

This patch has been in production testing for almost two months, and working perfectly.

What it does: Corrects a very serious flaw in the logic for adding quotes around string literals.

What it does not: Does not change the syntax expected by prepare(), which is incompatible with with MySQL data manipulation syntax. This has been knocked down to a "documentation issue" in #11318.

Regarding the other patches: None of them really made sense to me. The more string parsing that ends up in the hands of WordPress, the more convoluted and the more vuln-prone the system will be.

Also restoring the correct Severity value, based on the exhaustive hole-poking provided above.

comment:74 ryan4 years ago

(In [13357]) Don't quote escaped strings. Props dd32. see #11608

comment:75 dd324 years ago

  • Resolution set to fixed
  • Status changed from new to closed

I'm of the opinion that here we've gone as far as we can.

We can reinvent the wheel and make yet another prepare() method, or we can simply accept that with the PHP versions we support, what we have works acceptably as long as people understand how to use it.

In the future when we support MySQLi or PDO or some other form of technology, then it will make sense to create a new ticket for that then. In the meantime, if someone wants to re-invent a wheel, they may do so by overriding the wpdb class through WP_CONTENT_DIR/db.php

comment:76 hakre4 years ago

Related: #12942

Note: See TracTickets for help on using tickets.