[wp-hackers] Making WP more secure the evolutionary way

[Florian Thiel]

On Thu, Jan 22, 2009 at 3:28 PM, Jacob Santos <wordpress at santosj.name> wrote:
> The problem is that you are stated XSS attacks, which are unrelated to
> database attacks. What you are trying to prevent, is SQL injection, which is
> what prepare() does. Which isn't a perfect method, however neither is yours.
> You are proposing we abstract and do it for the developer, like they are
> idiots. You are suggesting we do it for WordPress core, which means the core
> developers and contributors are idiots for not protecting against SQL
> injection (which all of the places I've seen already protect against SQL
> injection using prepare).

I'm really sorry I brought up the XSS topic. That only lead to massive
confusion. My current proposal DOES NOT have anything to do with XSS.
It's only about SQL. I wanted to do some research if it would make
sense to do another set of annotations that does for HTML output what
the current one does for SQL. I'm not sure about that yet (if you have
an opinion on that or any hints, I'd be delighted to hear them).

> This ignores true hacking prevention known as sanitization. It is really
> simple to sanitize integers, dates, and what not, because they have a set
> format which can be validated very easily and sanitized. The issue is
> actually strings, such as HTML, which can exist in posts, as well as other
> string formats which need to be accepted, but still protected. What you have
> is something that needs to be sanitized liberally based on the user role and
> capability. WordPress already does this, so I guess my confusion is and what
> I should be question is: Are you proposing to abstract and protect the users
> and developers from themselves with the possibility of entering strings that
> might be venerable to XSS attacks?

No, nothing about XSS yet. You're absolutely right: XSS is much more
complicated and cannot be treated with the same approach as I
suggested for SQL.

> The contention I have is that I come from the understanding that each
> validation and sanitization should be created for each variable. For
> example, if the database field is a date, I will validate and, or sanitize
> for date before placing it in the SQL. If I'm using integers, I'll cast to
> an integer. I come from the group that you shouldn't abstract all data types
> into the same group, when you have multiple groups and multiple validations
> and sanitizations. So, if you protect each string against XSS attacks, you
> are ignoring the date formats. This could be sensed by the abstraction, but
> that will add overhead that would be unacceptable.
>
> So I mean, you are talking about protecting against XSS attacks in the
> database layer, when it belongs to the layer directly above database layer,
> which interacts with the database layer. Adding something that needs to be
> separate from the database layer to the database layer will add protection
> which doesn't exist. If you do it this way, then no one will have freedom in
> adding JavaScript to their posts and you'll need something to override this.
>
> The reason sanitization is added to one area and not others, is because it
> shouldn't be in other areas, because such protections will prevent freedoms
> that would otherwise be abstracted away from the users and developers. I can
> see the bug reports now (as editors and contributors who file bugs that they
> can't enter certain HTML elements) from developers and users saying that
> they can't enter video or [enter some future or current popular site]
> snippet into their post.
>
> The point with security is that there are already functions inside of
> WordPress which exist to protect against certain XSS and also protections in
> the administration to protect against CSRF attacks. So for example, if you
> go to a WordPress blog and try to enter some JavaScript that steals the
> cookies, it won't execute, because the comment functions (and not the
> database methods) protect against XSS attacks. This is the way it should be.
>
> The problem with security is that it isn't cut and dry, unless you don't
> want to accept any user input at all.

Yes, that's the way I see it, too. That's why I said that XSS is a
much more difficult thing than SQL injections.

> I would also advise against using CakePHP, which holds your hand and empties
> your garbage for you, as an example in the future as "best practice" because
> I don't really buy that CakePHP is best practice. It isn't for performance
> reasons, however neither is WordPress and in fact a lot of the developers
> suggest features and implementations which would make WordPress even more
> slow, so take what you will. I'm not the most intelligent developer on this
> list, nor the best, but I do read a lot and I do give advice based on other
> intelligent developers. I'll tell you now, that from what I've read, this
> sort of generalization of security is NOT the way to go, because it offers a
> false sense of security.

There may be another misunderstanding here. I do NOT WANT to use
CakePHP. I just wanted to illustrate what would be possible with the
abstracted functions. For now, all I'm suggesting is to replace the
existing INSERT and UPDATE calls using raw SQL with calls to the
respective functions in wp-db.php (called insert() and update()) and
working from there to gradually replace the other calls after
implementing the respective methods in wp-db.php. Once the annotations
are applied I will gladly send a patch with some of these changes
myself. These annotations are meant as a facilitation to get things
done that are already on the radar (see
http://trac.wordpress.org/ticket/6836). I think the annotations
provide visibility (you see that there's something that needs to be
done in the code. You can even stumble upon an annotation and fix the
respective issue), motivation (you can't let these pesky annotations
lurk around in the code forever) and measurability (you can just count
the remaining annotations to see how far along you are).

Good night,
Florian

> Jacob Sanots
>
>
> Florian Thiel wrote:
>>
>> On Thu, Jan 22, 2009 at 1:44 PM, Jacob Santos <wordpress at santosj.name>
>> wrote:
>>
>>>
>>> I apologize, I recognized some of the functions you were proposing as
>>> those
>>> usually found in database abstraction (which I confused with Active
>>> Record
>>> (!= ADODB) ). When I think data-access abstraction (Documentation might
>>> be
>>> incorrect on php.net, PDO appears to be database access abstraction with
>>> some touch of data access abstraction in that you can use the same
>>> methods
>>> to access queries in any database), I think of PDO and it certainly
>>> doesn't
>>> have an insert, update, where or select method. It has prepare method.
>>> Actually query is actually a prepare that is abstracted from the
>>> developer.
>>>
>>
>> No problem.
>>
>>
>>>
>>> Direct SQL in and of itself is not a bad thing, because if you know what
>>> you
>>> are doing and protect against hacking attempts, then there is no reason
>>> to
>>> further extend it by abstracting the SQL out. You are adding overhead
>>> where
>>> none is really needed. Your requirements have already been met by using
>>> prepare().
>>>
>>> What you seem to be worried about are plugin authors, which don't know
>>> about
>>> or don't use prepare() when using SQL. If they aren't, then they probably
>>> aren't protecting against other attacks.
>>>
>>
>> No, I'm not especially worried about plugin developers. And I'm not
>> especially worried about people who don't know about prepare().
>> Experience shows that these things "just happen" (tm). That's where I
>> was going with the XSS example. WordPress developers seem to know
>> about the problem of XSS in general, but sometimes you forget to
>> sanitize your output (or input in the SQL case) or use the wrong
>> sanitation. If you use abstraction, the right thing has to be done in
>> one place only. If everybody was perfect all the time, your argument
>> would be totally valid. But we're not. That's why reducing redundancy
>> (in the end, that's what these abstractions do) is a good thing. It
>> makes code more readable, more maintainable and more secure.
>>
>> Comet Support has an article about how SQL abstraction is handled in
>> CakePHP (http://cakephp.prometsupport.com/tag/sql-injection/). This is
>> a good example how powerful such an abstraction can be. NOTE: I'm not
>> suggesting WP should absolutely do it that way. I just want to give an
>> example how powerful an abstraction can be (note that you can
>> guarantee correct SQL because you're building the statement in the
>> abstraction method).
>>
>> Still unconvinced?
>> Florian
>>
>
> _______________________________________________
> wp-hackers mailing list
> wp-hackers at lists.automattic.com
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>