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

[Florian Thiel]

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

> Florian Thiel wrote:
>>
>> On Thu, Jan 22, 2009 at 12:53 PM, Jacob Santos <wordpress at santosj.name>
>> wrote:
>>
>>>
>>> The Active Record Pattern already has many (3: Zend Framework, ADODB, and
>>> ADODB Lite, also part of other frameworks (Yii, Code Igniter, etc) )
>>> libraries devoted to it. Basically, the idea of using it for security is
>>> pointless based on the idea of abstracting everything. Yeah sure, you can
>>> use something like mysql_real_escape_string() and protect the database
>>> from
>>> SQL injection attacks.
>>>
>>
>> I'm NOT suggesting the Active Record pattern. Active Record would
>> require a rework of the WordPress data model. All I suggest is doing
>> for the rest of the queries what has already been done for INSERT and
>> UPDATE. And I wanted to provide you with a smooth path to get there.
>>
>>
>>>
>>> This will not protect against XSS attacks or CSRF attacks. Those are
>>> really
>>> independent of the database abstraction, you'll really just want to focus
>>> on
>>> protecting the database, which prepare does a lot to do in the first
>>> place.
>>> I suppose the reason the XSS attack was fixed in one place was that was
>>> the
>>> one known place where it was affected. If you protected every place from
>>> XSS
>>> attacks, then it would really screw with plugins and posts that want to
>>> post
>>> YouTube or something like Google Analytics.
>>>
>>
>> My remark on XSS was about the point of doing checks, filtering, etc.
>> in a central place because it's much easier to maintain. My current
>> proposal DOES NOT fix XSS or CSRF. But would argue that another set of
>> annotations could do (at least part of) what the current annotation
>> does for SQL.
>>
>>
>>>
>>> The magic escaping method was probably KSES or another of the entities
>>> encoding functions in WordPress. This is how it should be.
>>>
>>> I'd be against Active Record Pattern, because of the additional overhead
>>> and
>>> no real benefits from it. It isn't like WordPress is actually ever going
>>> to
>>> add support for other RDBMS, so it isn't like it will have full support
>>> for
>>> other databases. Of course, WordPress could use ADODB Lite, it is GPL,
>>> but
>>> you know it would even more solidify that every plugin and some themes
>>> would
>>> have to be GPL.
>>>
>>
>> Yes, you're absolutely right. Moving to Active Record or using ADODB
>> is quite a disruptive change and really hard to bear for the project.
>> That's why I wanted to provide a way for gradual improvement. You can
>> change the SQL calls one at a time without breaking anything. And
>> after that is done, we could move on to another area (XSS comes to
>> mind, but that needs a little more research). I think lightweight,
>> evolutionary approaches are called for.
>>
>> Florian
>>
>>
>>>
>>> Florian Thiel wrote:
>>>
>>>>
>>>> On Thu, Jan 22, 2009 at 11:32 AM, DD32 <wordpress at dd32.id.au> wrote:
>>>>
>>>>
>>>>>
>>>>> 2009/1/22 Florian Thiel <flo.thiel+wphackers at googlemail.com>:
>>>>>
>>>>>
>>>>>>>
>>>>>>> Also, It seems to be that you're suggesting in your patch that using
>>>>>>> raw SQL (even though its prepared) is a bad idea?  Or am i reading it
>>>>>>> wrong? :)
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Yes, that's my point, I'm probably not making that clear enough. The
>>>>>> whole idea is centralising access to the database. When you have 5
>>>>>> places in the system where the db is accessed, it's really easy to fix
>>>>>> things once you notice there is a security vulnerability. You're
>>>>>> fixing it in one place and all callers profit from that. Experience
>>>>>> shows that if you have critical things like db access all over the
>>>>>> place you will forget updating one or two places when applying a
>>>>>> security fix. When you look at the last XSS incidents with WP, there's
>>>>>> the exact pattern. The problem was fixed by applying another magic
>>>>>> escaping method to the vulnerable spot. This probably leaves other
>>>>>> places vulnerable because escaping is not done in a consistent way
>>>>>> everywhere it's used. Obviously, you still need to find all the places
>>>>>> where db access (or client output) happens. But you only have to do
>>>>>> that one time.
>>>>>>
>>>>>>
>>>>>
>>>>> Thats the thing, I dont see how this is different from current. All
>>>>> queries which use user-data go through the wpdb::prepare() function,
>>>>> which sanitizes the data for the query.. wpdb->prepare("SELECT a, b, c
>>>>> FROM table WHERE a = %s", ' '\ UNION...'); would result in the -exact-
>>>>> variable passed in being quoted -correctly-, Of course, since
>>>>> wpdb::query() uses addslashes() instead of mysql_real_escape_string()
>>>>> There may be chars which make it through that.. But thats beside the
>>>>> point, Do you see where my confusion lies?
>>>>>
>>>>>
>>>>
>>>> Using prepare everywhere is definitely a step in the right direction.
>>>> The problem is that it does not help against flawed SQL passed from
>>>> the caller. If you do it like it's done for INSERT and UPDATE now, you
>>>> control the construction of SQL statements in one place and can avoid
>>>> constructing evil SQL. Since you have an abstract representation of
>>>> the statement (semantically richer) you have much more control over
>>>> the SQL generated. You could even do fancy things like making sure
>>>> that all columns mentioned in SELECT or WHERE really exist. We don't
>>>> have to go that far, but once it's centralized, improving these
>>>> central methods has a much larger payoff, since you only have to do it
>>>> once. That justifies spending more time on this, creating a more
>>>> secure WordPress in the end.
>>>>
>>>> Florian
>>>> _______________________________________________
>>>> wp-hackers mailing list
>>>> wp-hackers at lists.automattic.com
>>>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>>>
>>>>
>>>
>>> _______________________________________________
>>> wp-hackers mailing list
>>> wp-hackers at lists.automattic.com
>>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>>
>>>
>>
>> _______________________________________________
>> wp-hackers mailing list
>> wp-hackers at lists.automattic.com
>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>
>
> _______________________________________________
> wp-hackers mailing list
> wp-hackers at lists.automattic.com
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>