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

[Florian Thiel]

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
>