PCRE jit security hole WAS PCRE JIT stack size limit

9 years ago by Dan Ackroyd — view source

unread

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.

9 years ago by Christoph Becker — view source

unread

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

9 years ago by Anatol Belski — view source

unread

-----Original Message-----
From: Dan Ackroyd [mailto:danack@basereality.com]
Sent: Thursday, August 13, 2015 5:00 PM
To: Christoph Becker cmbecker69@gmx.de
Cc: internals@lists.php.net
Subject: [PHP-DEV] PCRE jit security hole WAS PCRE JIT stack size limit

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:

Could you please point to a real example where security is hurt, maybe in a real app? A security one were like password match. But even if an attacker sends str_repeat('a', 5431) and application doesn't check input password length - it's a flaw in the app itself with or without jit. If at the same time the stored password were "(a)*" and not escaped prior to matching - same, it's already a security flaw before.

Thanks

Anatol