Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122681
Precedence: bulk
MIME-Version: 1.0
References: <e9e82b80-da0a-40a0-aa4d-00237208ec61@gmail.com>
In-Reply-To: <e9e82b80-da0a-40a0-aa4d-00237208ec61@gmail.com>
Date: Mon, 18 Mar 2024 13:25:05 +0100
Message-ID: <CAP1Jc108pdjzqkCiaswOrhrp2cyo7_fP2Luf5YMKUJTX1Ld=CA@mail.gmail.com>
Subject: Re: [PHP-DEV] XSLTProcessor recursion limit
To: Niels Dossche <dossche.niels@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000003c77940613ee73ee"
From: arnaud.lb@gmail.com (Arnaud Le Blanc)

--0000000000003c77940613ee73ee
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Niels,

On Sat, Mar 16, 2024 at 5:49=E2=80=AFPM Niels Dossche <dossche.niels@gmail.=
com>
wrote:

> Hi internals
>
> Based on https://bugs.php.net/bug.php?id=3D71571 I've opened a PR to
> implement two new properties for XSLTProcessor: maxTemplateDepth and
> maxTemplateVars. The reasoning behind this is that large templates with
> lots of recursion and/or variables can bump against the default recursion
> limit set by the libxslt library.
>
> PR link: https://github.com/php/php-src/pull/13731
>
> I used an adapted version of
> https://github.com/nikic/popular-package-analysis to download every
> package that satisfies the search term "xsl". Then I checked if there are
> any classes that extend XSLTProcessor and have the maxTemplateDepth or
> maxTemplateVars field.
> None do, and as such no package in packagist will observe a BC break
> because of this PR.
>
> One sad detail however, is that you can set the recursion limit so high
> that you can exhaust the stack space, which will crash the process with a
> segfault. In fact, you can hit this already right now without the PR, usi=
ng
> XSL. I.e. you can create a very deep VM re-entry that won't cause the sta=
ck
> limit protection to kick in, and then start a recursive XSL template
> processing that does not hit XSL's limit, but exhausts the remaining stac=
k
> space. Note that as soon as you do callbacks however, the stack limit
> protection _will_ kick in.
> I tried to check if it's possible to prevent this, but the stack limit
> check would need to happen inside the libxslt library, so I don't think
> it's possible.
>
> Let me know if there are any complaints about this. If no complaints, I'l=
l
> merge this in 2 weeks or so.
>
> Kind regards
> Niels
>

For PCRE there is a pcre.recursion_limit setting [1] with the same issue.
This is fine because that's a convenience feature (not a security one) and
it is useful by catching most cases of uncontrolled recursion in practice.

The default value is not changed by this PR (so there is no risk of BC
break by lowering the limit), and appears to be low enough to prevent stack
exhaustions on a default 2MiB stack on Linux.

+1 for this

[1]
https://www.php.net/manual/en/pcre.configuration.php#ini.pcre.recursion-lim=
it

Kind regards,
Arnaud

--0000000000003c77940613ee73ee
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Niels,<br></div><br><div class=3D"gmai=
l_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Sat, Mar 16, 2024 at 5:49=
=E2=80=AFPM Niels Dossche &lt;<a href=3D"mailto:dossche.niels@gmail.com">do=
ssche.niels@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_qu=
ote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,20=
4);padding-left:1ex">Hi internals<br>
<br>
Based on <a href=3D"https://bugs.php.net/bug.php?id=3D71571" rel=3D"norefer=
rer" target=3D"_blank">https://bugs.php.net/bug.php?id=3D71571</a> I&#39;ve=
 opened a PR to implement two new properties for XSLTProcessor: maxTemplate=
Depth and maxTemplateVars. The reasoning behind this is that large template=
s with lots of recursion and/or variables can bump against the default recu=
rsion limit set by the libxslt library.<br>
<br>
PR link: <a href=3D"https://github.com/php/php-src/pull/13731" rel=3D"noref=
errer" target=3D"_blank">https://github.com/php/php-src/pull/13731</a><br>
<br>
I used an adapted version of <a href=3D"https://github.com/nikic/popular-pa=
ckage-analysis" rel=3D"noreferrer" target=3D"_blank">https://github.com/nik=
ic/popular-package-analysis</a> to download every package that satisfies th=
e search term &quot;xsl&quot;. Then I checked if there are any classes that=
 extend XSLTProcessor and have the maxTemplateDepth or maxTemplateVars fiel=
d.<br>
None do, and as such no package in packagist will observe a BC break becaus=
e of this PR.<br>
<br>
One sad detail however, is that you can set the recursion limit so high tha=
t you can exhaust the stack space, which will crash the process with a segf=
ault. In fact, you can hit this already right now without the PR, using XSL=
. I.e. you can create a very deep VM re-entry that won&#39;t cause the stac=
k limit protection to kick in, and then start a recursive XSL template proc=
essing that does not hit XSL&#39;s limit, but exhausts the remaining stack =
space. Note that as soon as you do callbacks however, the stack limit prote=
ction _will_ kick in.<br>
I tried to check if it&#39;s possible to prevent this, but the stack limit =
check would need to happen inside the libxslt library, so I don&#39;t think=
 it&#39;s possible.<br>
<br>
Let me know if there are any complaints about this. If no complaints, I&#39=
;ll merge this in 2 weeks or so.<br>
<br>
Kind regards<br>
Niels<br></blockquote><div><br></div><div><div><div>For PCRE there is a pcr=
e.recursion_limit=20
setting [1]
 with the same=20
issue. This is fine because that&#39;s a convenience feature (not a=20
security one) and it is useful by catching most cases of uncontrolled recur=
sion in=20
practice.<br></div><div><br></div><div>The default value is not changed by =
this PR (so there is no risk of BC break by lowering the limit), and appear=
s to be low enough to prevent stack exhaustions on a default 2MiB stack on =
Linux.<br></div><div><br></div><div>+1 for this</div><br><div>[1] <a href=
=3D"https://www.php.net/manual/en/pcre.configuration.php#ini.pcre.recursion=
-limit">https://www.php.net/manual/en/pcre.configuration.php#ini.pcre.recur=
sion-limit</a></div>=C2=A0</div><div>Kind regards,</div><div>Arnaud<br></di=
v>=C2=A0</div></div></div>

--0000000000003c77940613ee73ee--