Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115157
MIME-Version: 1.0
References: <CAFv4g+Gv1qQ2Gtx2Qwo0Dx2Cb_gOXJ=rm7rVy+Jo89PmD-3jnA@mail.gmail.com>
 <CA+hqhpfUiOMLDZyXTqP+mm9jjG6v+K94Ug7K_zvuuZus_XhOaQ@mail.gmail.com>
 <CAKws9z34nTT0_CXoo4Yq7zO2CZX622has23xmfHbWjfaDrgLqA@mail.gmail.com>
 <CA+hqhpdR=W8nbWy8UBgLtKdvb_KCss5mpzturS67HhqRj4q6BQ@mail.gmail.com>
 <CAKws9z1=6fmCxJWYVfN+xKT+u=_ZTq0m7C0nZPT6LB5scbfpXQ@mail.gmail.com>
 <95D16F2E-E9DD-4964-A0E2-62E1FB0D976B@koalephant.com> <CAKws9z1M6aLMJ+T7KEAYbEu7N3ufFi8Lsqpr4GP6G06+8aG3SQ@mail.gmail.com>
 <4DE5E2EC-26D6-4D2C-95A9-B843B440EE87@koalephant.com> <CAGBsUrfar+bY+4R9Wp=WfTr5r7-Ur0pVDmNYGJTt1nh9AQ3-Zw@mail.gmail.com>
 <26037CB4-4723-4DC5-BD92-BBDC4F548E17@koalephant.com> <24E12B58-7613-4E67-852C-3312F4AE769C@newclarity.net>
 <CAOwTYAtnib3uWToDxHD5xwHPDq755_xYV4Xte+4UBZQcXA+kow@mail.gmail.com>
 <CA+kxMuSdY6w4pUGsZgSe8RntjsXEFoa=kBHqFYg9vwmD4zEJ9w@mail.gmail.com>
 <CAFv4g+GimufiLQ97fuRu=yg6uhwuf2cY1mh_uEveKWcQ8wddeg@mail.gmail.com>
 <CA+kxMuRD3zZfuydusXaH7z0WQDq0-uUHD2TV8kvkaNmxXUAixQ@mail.gmail.com> <64D2FC1D-E4BA-46E7-A19E-3A7DE35B2096@gmail.com>
In-Reply-To: <64D2FC1D-E4BA-46E7-A19E-3A7DE35B2096@gmail.com>
Date: Sat, 26 Jun 2021 14:47:49 +0100
Message-ID: <CA+kxMuT3SbRqgiAnxCqno7e_OhT7Tsapo4Ua1tJGAAHJ-QdGsQ@mail.gmail.com>
To: Rowan Tommins <rowan.collins@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] [RFC] Name issue - is_literal/is_trusted
From: Danack@basereality.com (Dan Ackroyd)

On Sat, 26 Jun 2021 at 13:47, Rowan Tommins <rowan.collins@gmail.com> wrote:
>  the actual bug will almost certainly have happened somewhere else in the
> program, and you'll need to trace the data flow of $foo and $bar to find out where.
>

That depends on what you mean by bug. In particular I don't agree that
"The actual root cause is in getColor(),". Whether user-controlled
data is dangerous or not depends on where it's used, not where it's
coming from.

For me, the bug when $up->getColor() changes from returning a literal
string to a user supplied value is in this line of code:

"<div style='color: " . $up->getColor() . "'>";

Where a programmer has assumed that the value returned by
$up->getColor() is a literal string. It's not a bug for that function
to return a user controlled value.

> so tracking the root cause could be just as difficult.

Not as difficult. It would give the exact line of code where the bad
assumption was made.

literal_concat("<div style='color: ", $up->getColor(), "'>");

When $up->getColor() is changed from returning a literal string to a
user controlled one, it would throw an exception. You could look at
the exception, see that the 2nd parameter isn't literal and is coming
from the getColor method, and know this code isn't safe and needs to
be changed to something that can do the appropriate escaping.

Even if the origin of the value was more obscure as you suggest, so
maybe something like:

function getDiv($color) {
    $div = literal_concat("<div style='color: ", $color, "'>");
    // ...
    return $div;
}

$color = $up->getColor();
$html .= getDiv($color);

Although the origin of where the variable comes from wouldn't be in
the stacktrace, the error still would be.

There is a line of code that has a bad assumption of whether $color is
a literal string or not, and it's that line of code that needs to be
changed, to use something that understands HTML escaping, in
particular how to escape user input for html attribute context.

So even when the origin of a value might be hard to understand, the
precise line of code that has the bad assumption would be part of the
stacktrace, which makes figuring out the problem be a lot easier.

That's why it worked for Google. By not having to understand where
variables come from, but only having the error be where there are used
inappropriately, you completely get rid of the "having to understand a
large chunk of code" problem, and instead only have to consider where
the variables are being used, and in what context they are being used.

cheers
Dan
Ack