Re: Validation of class names in the autoload process

On Thu, Oct 17, 2013 at 2:31 PM, Johannes Schlüter <johannes@schlueters.de> wrote: > Are there codepaths where this can be injected from the outside? In the > unserializer this was prevented in > > https://github.com/php/php-src/commit/ff8055fc5c9750482aac7a25a074aae0b1e64706 > and some further commits, i.e. > > https://github.com/php/php-src/commit/7126de4912d9d4c7499deb1f9239980400aa7ec7#diff-d697fc054b607bb0ffd7493daeb6a1afR616 Sorry when I said deserialize I meant this as a generic term not php's unserialize(). I mean say you have two types of something, and the API works taking a type param + some data for it, and you create it with something along those lines: $class = 'Foo\Bar\Type'.$userGivenType; $obj = new $class($userData); In this case, the autoloader would be called with whatever the user passed in unless you validate it, but since autoloader injection isn't a very common vector many devs wouldn't validate this and assume a happy path I guess. > Reasons against moving those in a more central place were > > - Performance. checking always costs time (any autoloader is > way slower, shouldn't matter too much) > - There are more esoteric ways (mostly for extensions) to create > classes with "illegal" names (only illegal class names I know are > OCI-Lob and OCI-Collection from oci8 extension) an autoloader > (inside an extension) might produce them (I think that is unlikely > to exist in reality) > - Autoloader magic (developers sometimes do things I can't imagine) > - Autoloaders still have to verify (i.e. maximum length for the backend > used, some backends might require additional protection (i.e. \ needs > to be escaped)) IMO performance is the only valid concern, but it'll be faster done once in the proper way in C than done in various crappy ways in every userland autoloader, and it will also protect everyone at once. There might be edge cases not covered by invalidating "." and "NUL", but to keep it fast it may be best to only check for known bad chars instead of the full regex? Cheers