Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125090
Feedback-ID: ifab94697:Fastmail
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 21 Aug 2024 14:03:57 +0200
To: "John Coggeshall" <john@coggeshall.org>, Bilge <bilge@scriptfusion.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Message-ID: <c0058f34-063c-4610-a3aa-6bc3f328e229@app.fastmail.com>
In-Reply-To: <7E80614A-63CF-4515-9DF1-C6869218F864@getmailspring.com>
References: <48c22449-6cdf-449f-917f-365506e3f2d0@scriptfusion.com>
 <7E80614A-63CF-4515-9DF1-C6869218F864@getmailspring.com>
Subject: Re: [PHP-DEV] [Concept] Flip relative function lookup order (global, then
 local)
Content-Type: multipart/alternative;
 boundary=4246d22494f84058b5554b35cca9a4ee
From: rob@bottled.codes ("Rob Landers")

--4246d22494f84058b5554b35cca9a4ee
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable



On Wed, Aug 21, 2024, at 10:23, John Coggeshall wrote:
>=20
>=20
> On Aug 2 2024, at 4:37 pm, Bilge <bilge@scriptfusion.com> wrote:
>> My only concern is there needs to be an alternative way to do this: i=
ntercepting internal calls. Sometimes, whether due to poor architecture =
or otherwise, we just need to be able to replace an internal function ca=
ll. One example I can think of recently is where I had to replace `heade=
r()` with a void function in tests, just to stop some legacy code emitti=
ng headers before the main framework kicked in, then unable to emit its =
own response because HTTP headers had already been sent. In a perfect wo=
rld it shouldn't be necessary, but sometimes it is, so I think for this =
proposal to be palpable there must still be a way to achieve this.
>=20
> Just a tangent thought to the above, but I've always been a little con=
cerned with the idea that a malicious composer package could potentially=
 do nasty things because PHP looks at the local namespace first for func=
tions. For example, if a composer package focused on Laravel that define=
s malicious versions of internal functions for common namespaces like `A=
pp\Models` , `App\Http\Controllers` , etc. it could do some nasty stuff =
-- and supply-chain attacks aren't exactly uncommon.  Even worse is Word=
press or any other PHP-based software package that allows arbitrary plug=
ins to be installed by non-technical users who really would have no idea=
 if the package was safe even if they were looking at the code.
>=20
> <?php
> // something.php
> namespace App\Models;
>=20
> function password_hash(string $password, string|int|null $algo, array =
$options =3D []): string
> {
>    print("Hello");
>    return $password;
> }
>=20
> <?php
> // my code
> namespace App\Models;
>=20
> include "something.php";
>=20
> password_hash('foobar', PASSWORD_DEFAULT);

If this is an attack vector for your application, then fully qualified n=
ames is the way to go (WordPress does this nearly everywhere, for exampl=
e).

>=20
> I don't recall why local namespace first won, but IMO it wasn't a grea=
t call out the gate for that reason alone.  Yes, you can always use `\pa=
ssword_hash`  instead of `password_hash` , but making the default insecu=
re and slower is silly IMO -- and not fixing it because of BC seems like=
 the weaker argument here.
>=20
> John

It's not (at least for me) the BC break. It's being able to override glo=
bal functions. There are legitimate use-cases outside of testing. For ex=
ample, consider when a global function signature changes. In your librar=
y, you have to check the php version. You can change this 100 times for =
every single call, or you can just wrap it in a function that supports t=
he old signature and proxies it to the new signature. In other words, it=
 provides options that may be better than the alternative.

=E2=80=94 Rob
--4246d22494f84058b5554b35cca9a4ee
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div><br></div><div>=
<br></div><div>On Wed, Aug 21, 2024, at 10:23, John Coggeshall wrote:<br=
></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div><br></div><di=
v><br></div><div class=3D"qt-gmail_quote_attribution">On Aug 2 2024, at =
4:37 pm, Bilge &lt;bilge@scriptfusion.com&gt; wrote:<br></div><blockquot=
e><div class=3D"qt-moz-cite-prefix"><div>My only concern is there needs =
to be an alternative way to do this: intercepting internal calls. Someti=
mes, whether due to poor architecture or otherwise, we just need to be a=
ble to replace an internal function call. One example I can think of rec=
ently is where I had to replace `header()` with a void function in tests=
, just to stop some legacy code emitting headers before the main framewo=
rk kicked in, then unable to emit its own response because HTTP headers =
had already been sent. In a perfect world it shouldn't be necessary, but=
 sometimes it is, so I think for this proposal to be palpable there must=
 still be a way to achieve this.<br></div></div></blockquote><div><br></=
div><div>Just a tangent thought to the above, but I've always been a lit=
tle concerned with the idea that a malicious composer package could pote=
ntially do nasty things because PHP looks at the local namespace first f=
or functions. For example, if a composer package focused on Laravel that=
 defines malicious versions of internal functions for common namespaces =
like <code>App\Models</code>&nbsp;, <code>App\Http\Controllers</code>&nb=
sp;, etc. it could do some nasty stuff -- and supply-chain attacks aren'=
t exactly uncommon.&nbsp; Even worse is Wordpress or any other PHP-based=
 software package that allows arbitrary plugins to be installed by non-t=
echnical users who really would have no idea if the package was safe eve=
n if they were looking at the code.<br></div><div><br></div><div>&lt;?ph=
p<br></div><div>// something.php<br></div><div>namespace App\Models;<br>=
</div><div><br></div><div>function password_hash(string $password, strin=
g|int|null $algo, array $options =3D []): string<br></div><div>{<br></di=
v><div>&nbsp;&nbsp; print("Hello");<br></div><div>&nbsp;&nbsp; return $p=
assword;<br></div><div>}<br></div><div><br></div><div>&lt;?php<br></div>=
<div>// my code<br></div><div>namespace App\Models;<br></div><div><br></=
div><div>include "something.php";<br></div><div><br></div><div>password_=
hash('foobar', PASSWORD_DEFAULT);<br></div></blockquote><div><br></div><=
div>If this is an attack vector for your application, then fully qualifi=
ed names is the way to go (WordPress does this nearly everywhere, for ex=
ample).</div><div><br></div><blockquote type=3D"cite" id=3D"qt" style=3D=
""><div><br></div><div>I don't recall why local namespace first won, but=
 IMO it wasn't a great call out the gate for that reason alone.&nbsp; Ye=
s, you can always use <code>\password_hash</code>&nbsp; instead of <code=
>password_hash</code>&nbsp;, but making the default insecure and slower =
is silly IMO -- and not fixing it because of BC seems like the weaker ar=
gument here.<br></div><div><br></div><div>John<br></div></blockquote><di=
v><br></div><div>It's not (at least for me) the BC break. It's being abl=
e to override global functions. There are legitimate use-cases outside o=
f testing. For example, consider when a global function signature change=
s. In your library, you have to check the php version. You can change th=
is 100 times for every single call, or you can just wrap it in a functio=
n that supports the old signature and proxies it to the new signature. I=
n other words, it provides options that may be better than the alternati=
ve.<br></div><div><br></div><div id=3D"sig121229152">=E2=80=94 Rob<br></=
div></body></html>
--4246d22494f84058b5554b35cca9a4ee--