Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87746
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.17.21 as permitted sender)
Message-ID: <55CCCDF7.9000408@gmx.de>
Date: Thu, 13 Aug 2015 19:03:51 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: Dan Ackroyd <danack@basereality.com>
CC: "internals@lists.php.net" <internals@lists.php.net>
References: <CA+kxMuSQkk9XUU_fLOmKjH5-a_QHUfzBBiMecZ83U2m8Ommx-w@mail.gmail.com>
In-Reply-To: <CA+kxMuSQkk9XUU_fLOmKjH5-a_QHUfzBBiMecZ83U2m8Ommx-w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: PCRE jit security hole WAS PCRE JIT stack size limit
From: cmbecker69@gmx.de (Christoph Becker)

On 13.08.2015 at 17:00, Dan Ackroyd wrote:

> On 23 July 2015 at 11:07, Christoph Becker <cmbecker69@gmx.de> wrote:
>> PHP7 supports PCRE's JIT compilation of patterns by default, which
>> mostly works fine.  However, there are issues when the matching exceeds
>> the JIT stack limit, see bug #70110[1].
> 
> So to summarise and bring more people's attention to this
> conversation; the change to using pcre.jit does not seem to be a
> fantastic one. It allows preg_match to fail silently e.g. this code:
> 
> preg_match('~(a)*~', str_repeat('a', 5431), $match);
> var_dump($match);
> 
> gives the output:
> 
> array(0) {
> }
> 
> i.e. there is no notification that the preg_match failed to match.

That is "normal" behavior for preg_match(); whenever matching fails (for
instance, because of exceeding the recursion limit, or invalid UTF-8
encoded subjects) the function is supposed to return FALSE and to set
PCRE_G(error_code), which can be queried via preg_last_error().

> People use preg_match for black/white listing input for security
> purposes. As the input is under attacker control this seems like a
> very bad situation, as attackers will be able to figure out what input
> can be used to make those security checks give incorrect results
> silently.

Isn't it much more common to have something like

  if (preg_match($pattern, $subject)) {...}

for input white-list validation?  In this case there is no particular
security problem, if the preg_match() fails.

> I don't think we should ship PHP 7 with this issue unresolved. A
> suggested fix for PHP 7.1 has been mentioned:
> 
>> Instead I suggest to add
>> a new error constant which would be returned from preg_last_error().
> 
> I think this is a complete non-starter. It would be horrible to check
> after every call to preg_* whether or not the call succeeded. The
> regex engine failing is defintely way into the domain of exceptional
> circumstances, for which having an exception be thrown is the correct
> choice.

Well, I'm not happy with the current behavior either.  There has been
quite some discussion regarding random_bytes|int() and there have been
arguments against introducing exceptions triggered by functions for PHP
7 (because we're already in feature freeze, and because there is no
general concept regarding the details).  I'm not sure what has been the
outcome of this discussion.

Anyway, if we consider the current behavior (i.e. "silently" failing on
"internal" errors) of preg_match() and friends a security issue, we
would have to fix this for PHP 5.5 and 5.6 as well (and maybe for 5.4,
too), and raising an exception appears to be definitely inacceptable for
PHP 5.

> p.s. with the ini setting of pcre.jit=0 the code does appear to fail-safe:
> 
>> Now please consider the following simple expression
>>
>>  preg_match('/^(foo)+$/', str_repeat('foo', $n))
>>
>> This will fail (i.e. yield FALSE) independently of pcre.jit for large
>> enough $n.
> 
> It doesn't actually return false, instead it segfaults for me. This is
> obviously not ideal, but at least the function doesn't silently return
> a bogus value.

Um, for very large $n (e.g. 100,000) I can reproduce the segfault with
PHP 5.6.11, but not with current master (neither pcre.jit=1 nor
pcre.jit=0).  It seems to be appropriate to open a bug report.

-- 
Christoph M. Becker