Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115386
MIME-Version: 1.0
References: <CAFv4g+HRHrvCOTgpbVfCXj06De1kL+1+RwnGgq6tcyLNAtLfQA@mail.gmail.com>
 <CAFPFaMJs7qvko=tfBsXuwCqMV3VqXL4TjO2aKWWPjrAicZzM9w@mail.gmail.com> <782e08f6-d6a1-e1e7-b2d6-4706277a4d86@gmail.com>
In-Reply-To: <782e08f6-d6a1-e1e7-b2d6-4706277a4d86@gmail.com>
Date: Sun, 11 Jul 2021 18:00:10 +0100
Message-ID: <CA+kxMuSuqaPq_q8eZy2_pHQNr1zMNjout2f2bZc=ZUZiBx3mPQ@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] [VOTE] is_literal
From: Danack@basereality.com (Dan Ackroyd)

Rowan Tommins <rowan.collins@gmail.com> wrote:

> I still don't follow this reasoning, for the reasons I outlined here:
> ...
> Whether "$foo . $bar" is always non-literal, or non-literal only if one
> of its operands is, you're going to get an error about a non-literal
> string somewhere else in the program, and have to trace back to find
> where the "bad" concatenation happened.


There are two differences. The first is "even if there is a problem,
you at least can find the relevant code".

With string concatenation preserving literal flags, when a problem is
detected, the situation is: "There is a non-literal somewhere in this
chunk of HTML. Good luck finding it in the string, and then figuring
out where it comes from in the code."

With having to jump through the hoop of using a specific function to
preserve the literal flag, the situation is: "This line of code is
wrong. It needs to be changed to use escaping."

To be precise, you wouldn't _have_ to trace back to find where the
non-literal value comes from. At the point where the error is you
would probably just change it to use the appropriate escaping. e.g. if
this line of fails, because the color suddenly becomes a non-literal:

literal_concat("<div style='color: ", $up->getColor(), "'>");

you don't need to figure out where the color is coming from, you can
just change it to use a function that does the appropriate escaping:

html("<div style='color: %s'>", $up->getColor());

In general, any use of literal_concat() where the correctness of it
isn't obvious from reading the code, should probably start a
conversation of "have you considered defaulting to using
escaping/prepared query here"?

# It prevents the problem reaching an end-user.

The solution at Google prevents any security problem from reaching the
end-user by refusing to compile code.

One of the problems with PHP is that it is 'normal' for logs to be
filled with warnings...which means that most people just ignore them.

By allowing security problems to get through, rather than defaulting
to being an error means that they will reach end-users, and the
warning messages will end up in log files, and the process for finding
and fixing them will take more time than people will like.

So instead of defaulting to safe, it will default to being ignorable.
And the whole point (in my understanding) of Chris Kern's talk, is
that you need to make software safe by default.


> What it won't do, is tell you when you've forgotten to use it,

Which is a huge difference.

If you're aware that you're touching a security sensitive bit of code,
you're likely to do the right thing anyway, and so won't need the
crutch of is_literal.

If you're unaware that you're touching a security sensitive bit of
code, you're more likely to accidentally include user input where it
isn't meant to be used.

That means the feature would be annoying for the people who need it most.

Allowing errors to get through to users, for them to be allegedly
fixed at a later date is referred to as "Normalization of deviance"
and isn't a good choice for this RFC, imo.

cheers
Dan
Ack


* problems with static analysis as a solution

** Not everyone runs it, and it would be nicer to allow wordpress et
al to provide this security threat detection rather than hoping users
decided to use static analysis.

** It might require libraries also using the same static analysis annotations.

** It might get quite tricky to implement correctly, in particular for
functions where the literal-ness of the output depends on the
literal-ness of the input.