Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87743
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain basereality.com from 209.85.160.178 cause and error)
MIME-Version: 1.0
Date: Thu, 13 Aug 2015 15:00:00 +0000
Message-ID: <CA+kxMuSQkk9XUU_fLOmKjH5-a_QHUfzBBiMecZ83U2m8Ommx-w@mail.gmail.com>
To: Christoph Becker <cmbecker69@gmx.de>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: PCRE jit security hole WAS PCRE JIT stack size limit
From: danack@basereality.com (Dan Ackroyd)

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.

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.

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.

I think we need to do either, or both, of the following for PHP 7:

i) Ship PHP with pcre.jit=0 by default, and say that users should
beware enabling it.

ii) Within each preg_* function (or any other function) that calls
PCRE check for PCRE_ERROR_JIT_STACKLIMIT and throw an exception if it
is set.

Other solutions involving switching between jit and non-jit have been
discussed, but it looks like they won't be possible to implement in
time for 7.

Having function calls return bogus values without any warning is just
a very bad idea, and I don't think PHP 7 should ship with that
happening.

cheers
Dan

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.