Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115449
MIME-Version: 1.0
References: <CAFv4g+HRHrvCOTgpbVfCXj06De1kL+1+RwnGgq6tcyLNAtLfQA@mail.gmail.com>
 <CAFv4g+ErRYaGJKJRJm9qYvCMQzUgbJE_hYfhTn6CtSeZ9SKxNw@mail.gmail.com>
 <CAFv4g+FH2ntZz_fSKhoFDMmjaDVhs56oR7x4tE3F250CAZsNtg@mail.gmail.com>
 <CAJmy8YFT9UA8fGmfAh9vcKQA=uM8nuSnEeu4U2sdD1oxQq1YjQ@mail.gmail.com> <CAFv4g+Hzgsjr6W=HWaWi2C1Joxz8Jg3PGwQCL7beVZc2YpLuHA@mail.gmail.com>
In-Reply-To: <CAFv4g+Hzgsjr6W=HWaWi2C1Joxz8Jg3PGwQCL7beVZc2YpLuHA@mail.gmail.com>
Date: Sat, 17 Jul 2021 09:58:42 +0200
Message-ID: <CAJmy8YG5ttP_dDKu8zaN39S0cUM8AZGswvVedsGPcgK_CMvbCA@mail.gmail.com>
To: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000cb857605c74d14dc"
Subject: Re: [PHP-DEV] [RFC] [VOTE] is_literal
From: divinity76@gmail.com (Hans Henrik Bergan)

--000000000000cb857605c74d14dc
Content-Type: text/plain; charset="UTF-8"

oh thanks, now the vulnerability is clear. (i would still complain on that
as a pull request though, using double quotes for strings is just a
horrible idea, it's not compliant with ISO sql, and it depends on the MySQL
server it's running on *not* having @@SQL_MODE=ANSI_QUOTES enabled which
changes double-quotes to be ISO-compliant instead of mysql-borky (double
quotes for strings only work on MySQL because of a MySQL quirk, ISO SQL say
double quotes is meant for identifiers just like ` ), and it's not portable
across different SQL servers, and all of these problems go away if you just
use the sensible alternative: single quotes - seriously someone should
complain to whoever wrote that, so at least he doesn't do the same in the
future
i can tell from only that diff that, at least as of 5.5.1,  woocommerce is
not compatible with @@SQL_MODE=ANSI_QUOTES :p
)


On Sat, 17 Jul 2021 at 03:45, Craig Francis <craig@craigfrancis.co.uk>
wrote:

> On Fri, 16 Jul 2021 at 21:24, Hans Henrik Bergan <divinity76@gmail.com>
> wrote:
>
>> short of a bug in esc_like(), i don't even see the vulnerability issue in
>> that code?
>>
>
>
> Sorry Hans, I copied the wrong diff.
>
> There were only 2 changes from woocommerce 5.5.0 to 5.5.1.
>
> Like you I was wondering what that diff was doing before posting - I'm
> fairly sure it's just to be consistent with the other lines (which all use
> $wpdb->prepare).
>
> The diff I should have copied is:
>
> diff -r
> woocommerce.5.5.0/packages/woocommerce-blocks/src/StoreApi/Utilities/ProductQueryFilters.php
> woocommerce.5.5.1/packages/woocommerce-blocks/src/StoreApi/Utilities/ProductQueryFilters.php
> 86c86,92
> <  $attributes_to_count     = array_map( 'wc_sanitize_taxonomy_name',
> $attributes );
> ---
> >  $attributes_to_count     = array_map(
> >    function( $attribute ) {
> >      $attribute = wc_sanitize_taxonomy_name( $attribute );
> >      return esc_sql( $attribute );
> >    },
> >    $attributes
> >  );
>
> In context `$attributes_to_count` simply goes to:
>
> $attributes_to_count_sql = 'AND term_taxonomy.taxonomy IN ("' . implode(
> '","', $attributes_to_count ) . '")';
>
> Where the the esc_sql() is basically a call
> to mysqli_real_escape_string(), which explains why it needs risky quotes
> in/around implode.
>
> Craig
>
>
>
>

--000000000000cb857605c74d14dc--