Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115075
MIME-Version: 1.0
References: <CAFv4g+Gv1qQ2Gtx2Qwo0Dx2Cb_gOXJ=rm7rVy+Jo89PmD-3jnA@mail.gmail.com>
 <CAEMvS4GRaqMBPn7ocBX-XU3jzrtpe+9uGfYpG10qc_iKZY80UA@mail.gmail.com>
 <03f7955c-69a8-4841-9245-449d7851e207@www.fastmail.com> <CAFv4g+EaT309KDWz63j3S7n8E58czEWTKUEw9hAvgt7s6R9eKQ@mail.gmail.com>
In-Reply-To: <CAFv4g+EaT309KDWz63j3S7n8E58czEWTKUEw9hAvgt7s6R9eKQ@mail.gmail.com>
Date: Wed, 23 Jun 2021 14:21:39 -0400
Message-ID: <CAKws9z02yCZTb2_mw-5xUO5-QLc4jbOUcBRfJ8nFSNPP1rgO3g@mail.gmail.com>
To: Craig Francis <craig@craigfrancis.co.uk>
Cc: Larry Garfield <larry@garfieldtech.com>, php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [RFC] Name issue - is_literal/is_trusted
From: scott@paragonie.com (Scott Arciszewski)

On Wed, Jun 23, 2021 at 10:54 AM Craig Francis <craig@craigfrancis.co.uk> w=
rote:
>
> On Wed, 23 Jun 2021 at 14:37, Larry Garfield <larry@garfieldtech.com> wro=
te:
>
> > I'm still very torn on is_literal; I fear that the people who would
> > benefit from it are the very people that don't use the tools that would
> > leverage it (DBALs et al), and so the net benefit will be small.
> >
>
>
> This RFC will not help those who aren=E2=80=99t using libraries (DBALs), =
that=E2=80=99s
> something we could look at in the future (I have a suggestion in the Futu=
re
> Scope section, but whatever that involves, it will need to use this flag,
> so it would need to be in place first).
>
> But - and why I=E2=80=99m here - my experience is that it=E2=80=99s still=
 a big issue for
> those who *do* use libraries. It frequently comes up at software
> agencies/companies that maintain their code (built with free
> libraries/frameworks), and employ junior developers (i.e. the cheapest),
> who make many "quick edits" (time is money), and in doing so introduce th=
e
> issues the RFC covers (and not to say we more experienced coders don=E2=
=80=99t
> occasionally make mistakes too!). While non-library users are the main
> cause, the library users are still a big part of why Injection
> Vulnerabilities remain at the top of the OWASP Top 10.
>
> Craig

Hi,

I was asked for my thoughts on this RFC (and its naming) from a
security engineering perspective.

The old is_literal() isn't correct if it includes non-string values
(i.e. integers).
The new is_trusted() is potentially misleading, especially to people
who don't read the docs.

My knee-jerk reaction was simply, "Why not is_untainted()?" but that
invokes the imagery of taint-checking, which the RFC explicitly
doesn't implement. A better name might be is_noble(), where we get to
define the concept of noble inputs (name inspired by the Noble gases
from Chemistry).

The main reason I don't like is_trusted() is that everyone's threat
model and risk tolerance is different, and trust is too nebulous a
concept for a built-in function. But also, it's really easy to jump
the guard-rail: https://3v4l.org/4GM8Q#focus=3Drfc.literals

One concern that Joe Watkins asked about is: Is it reasonable to cover
both integers and strings that are not influenceable by potential
external attackers (n.b. ones that can't already overwrite your source
code)? Outside the chr()/pack()/sprintf()/etc. technique demonstrated
above, there's no risk of injection inherent to concatenating a
trusted string with an untrusted integer.

Injection attacks (SQL injection, LDAP injection, XSS, etc.) are, at
their core, an instance of type confusion between data and code. In
order for the injection to *do* anything, it needs to be in the same
input domain as the language the code is written in. Try as you might,
there is no integer that will, upon concatenation with a string,
produce a control character for HTML (i.e. `>`) or SQL (i.e. `'`).

Therefore, if the concern is Injection attacks, integer inputs do not
need to be tracked to provide a security gain.

This is only true for integers, not all numeric types. I haven't
investigated the safety of floats in every possible context, and the
`e`, `+`, and `.` characters could be problematic in corner cases.

TL;DR
- Why not is_noble()?
- String + int concatenation isn't an injection risk.

Cheers,
Scott