[wp-trac] [WordPress Trac] #41925: Bring back, support and document %1$s support in wpdb->prepare

[WordPress Trac]

#41925: Bring back, support and document %1$s support in wpdb->prepare
-------------------------+----------------------
 Reporter:  soulseekah   |       Owner:
     Type:  enhancement  |      Status:  closed
 Priority:  normal       |   Milestone:
Component:  Database     |     Version:
 Severity:  normal       |  Resolution:  wontfix
 Keywords:               |     Focuses:
-------------------------+----------------------

Comment (by soulseekah):

 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:

 {{{#!php
 "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?

 {{{#!php
 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?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/41925#comment:16>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform