WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

Last modified 3 weeks ago

#41925 closed enhancement (wontfix)

Bring back, support and document %1$s support in wpdb->prepare

Reported by: soulseekah Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Database Keywords:
Focuses: Cc:

Description

4.8.2 specifically restricts the very convenient usage of %1$s, %1$d, %1%f placeholders in WPDB::prepare. For yet undisclosed security purposes.

Since this has been very widely used in code as an undocumented feature, I propose to bring it back, provide official support and documentation for it.

https://github.com/search?q=wpdb-%3Eprepare+%251%24s&type=Code&utf8=%E2%9C%93 over 1.2 million search results using the no longer supported syntax. Including the very popular Yoast SEO plugin which broke unexpectedly with the recent security update.

Attachments (1)

41925.patch (5.7 KB) - added by soulseekah 4 weeks ago.
First attempt at making wpdb::prepare great again

Download all attachments as: .zip

Change History (22)

#1 @ocean90
4 weeks ago

  • Type changed from feature request to enhancement
  • Version trunk deleted

Noting that this format was never really supported as stated in the DocBlock:

The following directives can be used in the query format string:

%d (integer)
%f (float)
%s (string)
%% (literal percentage sign - no argument needed)

All of %d, %f, and %s are to be left unquoted in the query string and they need an argument passed for them.
Literals (%) as parts of the query must be properly written as %%.

This function only supports a small subset of the sprintf syntax; it only supports %d (integer), %f (float), and %s (string).
Does not support sign, padding, alignment, width or precision specifiers.
Does not support argument numbering/swapping.

#2 @ocean90
4 weeks ago

#41926 was marked as a duplicate.

#3 @soulseekah
4 weeks ago

It wasn't, but it worked even when the docs said it wasn't supported. And seeing that 1.2 million lines of code made use of this unofficial feature I think it's a good idea to bring it back. Right now WordPress is generating invalid SQL code for 1.2 million usages. Fatal errors. I don't think this is good back-compatibility policy. Yes, all of us have failed to stick to the official documentation, but the unofficial way worked. So this is just a feature request to start supporting it seeing it's vast usage in the wild.

This ticket is a feature request for 1.2 million lines of code out there. They just don't know they need it yet, but by the end of the month you'll be seeing all hell break loose in support departments and forums and oh the poor developers! Who are they going to blame? wpdb->prepare and WordPress.

Let's bring this back and support it officially and safely, shall we? Let's be back compatible, even if 1.2 million "bad" decisions were made, let's make it right as soon as possible. Again, this is a matter of patching 1 line in WordPress vs. patching 1.2 million lines in the wild.

However, to bring it back (or reimplement it) in a safe way we have to first understand what the security issue with it was. Until that's disclosed a patch cannot be worked on.

Last edited 4 weeks ago by soulseekah (previous) (diff)

This ticket was mentioned in Slack in #forums by netweb. View the logs.


4 weeks ago

#6 @soulseekah
4 weeks ago

The related vulnerability that was based on the usage is disclosed here https://medium.com/websec/wordpress-sqli-bbb2afcc8e94

It seems to be based on "%1$%s" being used, which isn't even a valid sprintf placeholder. "%1$s" is. But the issue is that numbered placeholders are not quoted. So indeed, the usage of unquote numbered parameters is unsafe in core.

How do we go from this to safe patch? That satisfies both current usage in the wild and security? Well, we need to quote them and write dozens of unit tests for the prepare method to make sure it works.

Right now wpdb quotes %s parameters before escaping them. I propose that we quote them after escaping them, unless the result is numeric, although there may be issues with locale-aware floats and is_numeric (http://php.net/manual/en/function.is-numeric.php) but even if they're quoted the SQL will still be valid.

https://github.com/WordPress/WordPress/blob/master/wp-includes/wp-db.php#L1228

Quoting the result of escape_by_ref if not is_numeric should work. Patch and tests in progress.

@soulseekah
4 weeks ago

First attempt at making wpdb::prepare great again

#7 @soulseekah
4 weeks ago

Added a patch which addresses 2 critical issues:

  1. Make sure %s placeholders are quoted correctly (odd number of leading %)
  2. Make sure %f placeholders are converted correctly (odd number of leading %)

Adds 1 feature:

  1. Freaking numbered placeholders.

Includes over 20 test cases. Let's write some more.

From the time spent writing the patch I understood that the original vulnerability with %1$s was that it was not quoted and would result in a SQL injection, nothing more, nothing else.

So $wpdb->prepare( 'SELECT * FROM wp_posts WHERE post_ID = %1$s', '1 OR 1 = 1' ); would yield

SELECT * FROM wp_posts WHERE post_ID = 1 OR 1 = 1; a classic injection.

Yes, it's bad. Anyone who used it without quoting it like '%1%s' is vulnerable. A false sense of security was attained by those who did. Let's make things right again. For everyone. For those who wanted numbered placeholders but read that WordPress doesn't support it, for those who found out that it worked and used it in a vulnerable way, and for those who used it in a safe way but can now no longer use it.

Let's make wpdb::prepare great again.

Last edited 4 weeks ago by soulseekah (previous) (diff)

This ticket was mentioned in Slack in #core by soulseekah. View the logs.


4 weeks ago

#9 @katzwebdesign
4 weeks ago

I always assumed placeholders were safe to use, and I expect them to continue working.

I support merging.

#10 @aaroncampbell
4 weeks ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Hey @soulseekah,

I can appreciate the desire to have numbered placeholders in wpdb::prepare(). They're definitely my preference for sprintf() et al. Unfortunately, even after spending a lot of time on this, the security team wasn't able to find a way to keep them around and also fix the security issue, without basically writing an SQL parser. The more complex the parser got, the more likely there were additional edge cases that would cause problems.

I ran your patch against some test cases I have from when we were working on this issue, and unfortunately it re-introduces the vulnerability that was closed with this change. Enforcing the simple subset of %s, %F, and %d allows us to make wpdb::prepare() safe and effective, which is it's main purpose.

Thanks,
Aaron
WordPress Security Team Lead

#11 @soulseekah
4 weeks ago

@aaroncampbell Thanks for your response. Would you be able to send me the test cases that are failing? Maybe we can make it work with a fresh set of eyes? I find it difficult to believe that it's such a huge issue. I'll be the first one to admit it is, if it really is.

And why is this feature request closed? Maybe we can write a full parser within this context. Why are you giving up on it like this, killing the dreams of many? Please allow me to reopen the ticket. It's a valid request after all. We have tickets that have been open for tens of years with seemingly impossible tasks yet you close this one in less than 12 hours?

Is this the reality of one of the most prominent open-source projects out there? Just like that?

Last edited 4 weeks ago by soulseekah (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 weeks ago

#13 @pbiron
4 weeks ago

As someone who manages several sites that are under constant injection attacks, I fully support the security team's efforts to close the vulnerability.

However, I also support the request from @soulseekah to leave this ticket open in case someone can find a way that closes the vulnerability while still allowing more expressive sprintf() args.

#14 @pbiron
4 weeks ago

I'd note that the way the vulnerability was closed also rules out args like %.6f in addition to %1$s, which broke WordFence (see the change log entry for 6.3.19).

#15 @soulseekah
4 weeks ago

I deduce that the secret test case that is mentioned is actually:

$wpdb->prepare( 'SELECT * FROM wp_posts WHERE ID = %1$%s', '1 OR 1=1' );

Which does in fact bring out an injection that my patch does not address. It is a typo (syntax error) but the expected replacement after sprintf() would need to be "$'1 OR 1=1'" in that part, I believe. Continuing to work on the patch. Let's see if we can make it work.

#16 @soulseekah
4 weeks ago

Okay, so, correct me if I'm wrong, but the vulnerability that is seeping through is actually based on a typo of a numbered placeholder?

%1$s vs. %1$%s? Whoever mistypes that would get a SQL error immediately because of the enclosed quote. It's like typing:

"SELECT * FROM wp_posts WHERE ID = '%s''".

The extra quote will throw errors until the developer sees that he has the wrong numbered placeholder syntax and fixes it to the correct one. That alone is not enough to get SQL injection either way. A malformed SQL query - yes. Chaining several of these might perhaps lead to an injection, where you have several placeholders and you inject "1 +" in one and "0" in the next one to attain WHERE ID = 1 + '0' and get everything back. So yes, I agree, as tiny as the probability of getting to a real injection as it is, it's still a probability.

Do you know what the C compiler has to say about the incorrect syntax usage?

prepare.c: In function ‘main’:
prepare.c:4:10: warning: conversion lacks type at end of format [-Wformat=]
  printf( "SELECT * FROM wp_posts WHERE ID = %1$'%s'", "1 OR 1=1" );
          ^
prepare.c:4:10: warning: missing $ operand number in format [-Wformat=]

And you know what happens if I run it? And yes, when I run that we get "SELECT * FROM wp_posts WHERE ID = %s'" from C as well. Not surprised, since PHP's printf family is usually redirected to the C counterparts without many changes.

But even my editor's (vim) syntax highlighting sees the issue and colors it differently. So a typo, heh? Anyone actually seen it used like that in the wild? Why is PHP not protecting us from this by also throwing a warning?

Should we do PHP's job and throw a warning here? Perhaps. Why don't we throw a warning when $wpdb->query is not preceded by a $wpdb->prepare call in the chain? The probability and reality of that happening is much much higher than the probability of mistyping the %1$s placeholder.

But okay, let us be entertained. Let's throw a warning. Let's see how GCC does it.

https://github.com/gcc-mirror/gcc/blob/06340e70bab4698299f5bab138abf152a33d7cdc/gcc/c-family/c-format.c#L2794

Yeh, we won't be doing all that... better solutions?

Back to the drawing board. Let's see, how does this even happen?

%1$'%s' this part is gobbled up leaving %s' to fend off for itself. How do we detect this?

A protective regular expression? That looks for variations of %1$[^dfFs] and throws a warning at the developer? Variations that account for leading %% escapes?

Ideas anyone? How do we build a safety net for this typo?

And once we do, how do we build a safety net for the myriad of other typos (including stray quotes) in prepare that would also yield the same injection opportunities? Shall we build a parser for a parser? I'm really stumped here.

I asked for clarification from the vulnerability author (https://medium.com/@soulseekah/hold-on-hold-on-hold-on-45e549d7baf1) and some sort of response might be due. Keeping this shut and hush-hush and secret test cases, seeing how it was disclosed almost 4 weeks ago is really preventing others from exploring the problem space and coming up with a better solution that improves wpdb not makes it worse. The community is ready to help, embrace it. It's what open source is about.

But again, this is just a feature request. I would like to see support for numbered placeholders in the prepare statements. I'd probably open the same ticket even if they didn't work at all. So let's figure out a way to do it without breaking the documented compatibility (plain s, f, d). Nothing more, nothing less.

Thank you.

Last edited 4 weeks ago by soulseekah (previous) (diff)

This ticket was mentioned in Slack in #core by soulseekah. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by joostdevalk. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 weeks ago

#20 @SergeyBiryukov
4 weeks ago

#41942 was marked as a duplicate.

#21 @Clorith
3 weeks ago

#41997 was marked as a duplicate.

Note: See TracTickets for help on using tickets.