NYCPHP Meetup

[Anthony Ferrara]

Gary,

On Thu, May 22, 2014 at 10:26 AM, Gary Mort <garyamort at gmail.com> wrote:
> On 05/21/2014 02:32 PM, Anthony Ferrara wrote:
>> First off, I do $name = $_GET['name']. I filter when I know what's
>> going to happen with it (such as when it gets bound against a data
>> model).
> But your not a novice programmer, so this doesn't apply to you. Though
> personally, I wouldn't do $name = $_GET['name'].  I'd use $name =
> filter_input(INPUT_GET, 'name', FILTER_UNSAFE_RAW)

Then you completely missed the point of what my intention was. And why
in the world would you use FILTER_UNSAFE_RAW over $_GET['name']? There
is quite literally no benefit. And you're running it through a method
for literally no reason. Exposing yourself to potential bugs in the
process.

Aim for readability, use input directly.

> $_GET is fine when you are writing code for a specific platform where you
> know in advance what the configuration will be.
>
> However, I'm seeing a lot of linux distro's have started set filter_default
> to FILTER_SANITIZE_STRING in their PHP install packages.

And that's a horrific idea. And one reason you should avoid the
filter_* APIs at all costs.

> I prefer writing code which doesn't depend on specific php.ini directives in
> order to work.

That has nothing to do with what we're talking about. I'm talking
about using request information directly.

If you're using a framework, use its request object. If you're not,
use the superglobals directly.

But please don't use filter_*. You think you're making yourself safer.
It tells you it's safer, but it's not. In many cases, it's worse.

>
>>
>>
>> Example: you're using filter_var with FILTER_SANITIZE_STRING. Do you
>> know what that *really* means under the hood?
>>
>> It's really no better than running strip_tags. And we know that we
>> shouldn't be running strip_tags, right?
>
>
> For this use case, yes we should.

Absolutely not. strip_tags is known to be unsafe. It's not something
you should rely upon for security. And that's what we're talking about
here.

> Consider the secretary updating their company website.  They have been told
> that they need some landing page to say "Welcome <name>" at the top.
>
> The pages are mostly html with a bit of PHP here and there.  So they go to
> an online tutorial, go through steps 1-4 where they learn about "hello
> world" which is a simple little tutorial of
> $name = $_GET['name'];
> echo "Hello $name";

Who said to `echo "Hello $name"`? I sure didn't.

I said you need to escape and encode properly for the output context.
If you're rendering to HTML, as a property or as a text node, you
**must** use htmlspecialchars. And you **must** use it properly:

echo "Hello " . htmlspecialchars($name, ENT_QUOTES | ENT_DISALLOWED |
ENT_HTML5, "utf-8");

Anything else is likely going to have holes in it that would allows
you to possibly exploit certain circumstances (just like strip_tags
does).

Yes, that's a lot to write, but more on that in a minute.

> Well, that's good enough for their needs.  A small tweak and now they have
> the page and the company sends out e-mail to their clients and all they have
> to do is add ?name=XXX on the e-mail.
>
> They ALSO have a horrible cross site scripting issue where someone else can
> craft custom links to their website and insert any html and javascript they
> want to.

Which is why you never output unescaped data. But the point is that
you escape it **on output**. NOT on input.

> Strip tags is the correct solution for 90% of introductory coding. It
> sanitizes the input from a bunch of common exploits.   On the downside, it
> removes all text from within angle brackets - but 95% of all minor uses of
> PHP it means everything just works.

It absolutely is not. It's very well understood that it's not ideal:

Example: http://stackoverflow.com/questions/3605629/php-prevent-xss-with-strip-tags
Example: https://isisblogs.poly.edu/2013/07/02/php-strip_tags-not-a-complete-protection-against-xss-repost-from-archive/
Example: http://security.stackexchange.com/questions/10011/is-strip-tags-horribly-unsafe
Example: http://forums.devshed.com/php-development-5/strip_tags-removes-html-tags-stop-xss-934238.html

Instead, you should follow actual recommendations on the subject:
https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet

>
>
>>
>> And that's why a solution like you're proposing is bad. It conflates
>> filtering with escaping. And that's **always** a bad thing.
>
> Nope, all solutions are based on what is simplest, what performs best, what
> is most comprehensive, and what is most appropriate.

And what you're recommending is neither simplest, best, most
comprehensive or appropriate.

> My solution maximizes simplest - because that is the most important thing
> for beginning programmers. And deals with what is most appropriate for new
> programmers.

There are easier solutions that that. But more on that in a second.

> When people are learning, they put the code up on whatever 'temporary' web
> space they have at hand.  Whether that is a godaddy site they just setup, a
> subdirectory on an active website someone has given them, on virtual systems
> running as subdomains for a university vps123.sandbox.marist.edu [which
> means that they can set cookie information for the entire marist.edu domain]
>
> And when their done, they forget about it and leave it running - publicly
> accessible and exploitable.
>
> That harms everyone on the internet - because those exploitable bits of code
> are used as launching points for denial of service attacks, controlling
> e-mail spam botnets, and many more.
>
> Furthermore, unlike most solutions out there, using an anonmous function
> tied to a variable means that as programmer learns, they can grow and make
> it better.
>
> Eventually, they will get to the point where they learn about including
> files and maintaining common code.  So then, instead of sticking 4 lines of
> code at the top of every php file, they can move it all to a file of common
> declarations.
>
> Then they will learn about better ways of filtering data, so instead of
> going through every piece of legacy code that they have, they can instead
> change it in one place.
>
> $get = function($varName, $filter = 'string') {
>
> $var = filter_input(INPUT_GET, $varName, FILTER_UNSAFE_RAW);
> return \Filterus\filter($var, $filter);
> }

PLEASE NO. This kind of thinking that security is "generic" and
something you can just sprinkle on after the fact and tune is part of
the reason XSS is so prevalant. Because you have people writing
functions like

make_safe($name) {
    return mysql_real_escape_string(addslashes(htmlspecialchars($_REQUEST[$name])));
}

And I strongly hope I don't need to elaborate on why that's a horrific idea.

> If instead they started with using a specific filter library and then
> discover that the library is no longer supported, then they have to go
> throughout ALL their code where they called
> $var = \Filterus\filter($var, $filter);

Which if you used abstraction, isn't that many places. But more on
that in a minute.

> And change every single instance.... of course, this assumes that they had
> the patience to wait until they reached the course section on filtering
> data.
>
> And for the professional PHP developers, promoting these simple 4 lines has
> an added benefit.  When the small business grows and reaches the point of
> needing to hire a professional - when the professional opens that code up in
> their IDE they can easily find every instance of problem code - since it is
> all flagged with the \\FIXME:  tag which every IDE I know of automatically
> will parse and display a list of ever single instance of those tags[in
> PHPStorm it's View-->Tools-->ToDo]
>
> I'm certainly open to alternatives that actually address the problem: novice
> programmers writing insecure code. https://github.com/ircmaxell/filterus as
> it is written right now is not it.  Looking at that page, you have to go all
> the way to the bottom to get something a novice programmer might be able to
> use - and even there what is written is not of any real use.  But then, I
> assume that you didn't write it for the novice programmer, so I don't see a
> problem with it.  Just re-iterating that it doesn't address the same issues
> my solution does.

The point is that there's a difference between input and output. You
**never** escape input. That's the wrong place to do it. It leads to
second-order injections and XSS. Where you thought the data in the
database was safe, because you escaped it when it came in the first
time. But that doesn't mean it will be safe forever.

Example: what if your escaping routine isn't good enough, and there's
a bug. Now you're screwed. Because you need to change it, but all of
your data is already escaped. So instead, you're stuck having to
either double-encode everything (yuck), or somehow go back and
re-parse everything in the DB to fix it. Both solution SUCK. And I've
done both of them in the past. And it sucked. And you should be doing
that right now, since you're not safe either (as I pointed out,
filter_var doesn't actually protect you from XSS).



The correct way of handling these issues is splitting up filtering
(ensuring input is in a valid data domain) and escaping (ensuring
output is encoded properly for the output context).

For input, Filterus, or even is_string, etc can help a lot. The point
though is that you want to verify that the data is valid to you. Not
that it's safe from XSS. That it's valid for your business purpose.
That's all you should **ever** worry about from input.

For output, the easiest, simplest and most proper way to handle this
sort of thing is by not handling it at all. Use a templating engine
like Twig or Mustache, which will auto-escape for you by default. This
isn't a magic bullet, as output into JS or CSS contexts can still
cause you a lot of pain, but it handles the 98% case quite well. If
you're outputting directly from PHP, either define an escape function
which wraps htmlspecialchars, or don't output directly from PHP.



As far as "catering to the novices", that's a completely bullshit
phrasing. Novices don't need to be catered to. They don't need things
to be dummbed down. They don't need to be spoon fed. They need to be
educated. And telling them that "just use this and you're fine" is
dangerous. And it's doubly dangerous when what you're suggesting is
not actually fine (as I've pointed out numerous times in this thread.

Never mediate, always educate.

If you think filterus is too hard for a novice to navigate, then
improve the documentation. Make a pull request to make it simpler to
understand. Write a blog post explaining how to properly work these
systems. Don't just assume that just because they are a beginner that
they are too dumb to understand. Most of the time it's simply that
they don't understand the terminology. But if you explain it to them
properly, no problem.

But saying that they should do the wrong thing, which is known to be
insecure, because they won't be able to understand the wright thing,
is horrible.

Anthony