Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:69619
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain seld.be designates 209.85.223.170 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1382016068.3980.264.camel@guybrush>
References: <CA+GmM=yBFMwDS10HEeCEgDQJ7mZvDdiLXq7j3ABNC_641Lb94A@mail.gmail.com>
	<1382013115.3980.236.camel@guybrush>
	<CA+GmM=zqdbKzZNeLt-wTzd1jfdYSSZxw1Q4cqrN2Xok3BeAbpg@mail.gmail.com>
	<1382016068.3980.264.camel@guybrush>
Date: Thu, 17 Oct 2013 15:49:28 +0200
Message-ID: <CA+GmM=xnWsXJ7RWcekfcbrYtjYiKpdgE3x=7afQRGsofKvWNnQ@mail.gmail.com>
To: =?ISO-8859-1?Q?Johannes_Schl=FCter?= <johannes@schlueters.de>
Cc: PHP Developers Mailing List <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Validation of class names in the autoload process
From: j.boggiano@seld.be (Jordi Boggiano)

On Thu, Oct 17, 2013 at 3:21 PM, Johannes Schl=FCter
<johannes@schlueters.de> wrote:
> I know new $class() and other ways, I was more curious about the real
> security impact -- usage of unchecked class names has quite a few
> security implications even without autoloader. Are there "common" ways
> about how unsafe class names might be injected?
>
> An attacker could use that to trigger "bad" constructor behavior or
> such.

Certainly this is possible, but it's a much slimmer risk IMO since it
would need bad code to already be loaded vs allowing the attacker to
trigger a require of an arbitrary file.

In any case the fact that this would not address every security issue
doesn't make it a worthless change I think?

> If PHP does a "minimal" check only there's nothing won. Autoloaders
> still have to check according to their needs. For example on Windows
> checking : is important, else I inject $classname =3D "c:\\foobar" as
> filename and bypass one or the other autoloader. And who tells that
> classes are loaded from filesystems, not database+eval, suddenly ' is
> dangerous. There are tons of other examples easy to construct and we
> should not pretend to be safe there.

OK so maybe we need to add : to the list to protect windows. I don't
think the database+eval case is realistic, I'm sure someone out there
does that but what I am trying to achieve is protecting *most* people
doing sane things. If you load classes from the DB then escaping the
class name is a no-brainer, and it's not a performance hit compared to
the DB IO anyway. A require() is expected to be safe, and people may
not expect php to let through such messed up "Foo\..\Bar" class names
since they are pretty much never seen out in the wild. Please try to
see this in a pragmatic way. And in the worst case if select chars
isn't feasible, as I said I'm fine with filtering the whole class
identifier regex if that's the safest way. I just would like this to
be addressed in some way.

> If one takes class names from untrusted sources one has to be careful.

True of course, but I don't think it's good to deflect that
responsibility to php users while there is something that can be done
at fairly low cost in the language to help them.

Cheers