515328 - Object->create() and Object->match() should validate that their fields are actually valid database columns

Status: RESOLVED DUPLICATE of bug 515191

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

When calling insert_create_data as part of create(), it should validate that all of the fields its about to put into the INSERT are in fact actually valid column names.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here we go. This is pretty simple. :-)

Comment 2

[Max Kanat-Alexander]

Attachment: v2

This one actually, I believe, entirely fixes our current security issue for HEAD, in a much simpler way than we are doing for the branches. (However, it also has implications for customizers who add columns without using the Bugzilla::DB methods, so I don't want to suddenly spring it on people in a security release.)

Comment 3

[Frédéric Buclin]

Comment on attachment 399438 [details] [diff] [review]
v2

>Index: Bugzilla/Object.pm

>+        $class->_check_field($field, 'match');

Instead of passing 'match', why don't you call caller() in _check_field()?


>+sub _check_field {

>+    if (!Bugzilla->dbh->bz_column_info($class->DB_TABLE, $field)) {

Why don't you look at $class->DB_COLUMNS instead? I fear that querying the DB for each column is going to slow down Bugzilla a lot. create() isn't called that much, but match() definitely is.


> sub insert_create_data {

>+        $class->_check_field($field, 'create');

This doesn't look like the right place to call _check_field() IMO. I think this check should be done in run_create_validators() as that's its job to validate the data. As we may pass more fields than what currently exists in the DB or in DB_COLUMNS, we could imagine having a CREATE_COLUMNS_WHITELIST or something like that which explicitly lists which extra items are allowed. I know your solution is easier and requires less code; I'm just wondering if this is the right way architecturally.

Comment 4

[Dave Miller [:justdave]]

despite "feeling" like an enhancement here, this is, in fact, fixing a security bug, and needs to be timed to commit coinciding with the security release, which makes it a blocker.

Comment 5

[Dave Miller [:justdave]]

And since this is blocking all attacks of this nature at the place where the damage can be done, and the security fixes on the other branches are only blocking the inward paths (to avoid breaking things), we need to make damn sure there aren't any other inward paths on the other branches that we haven't blocked yet, because once this fix is made available, people will start looking for other inward paths on the older branches.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #3)
> Instead of passing 'match', why don't you call caller() in _check_field()?

  Because from insert_create_data, we want to say the error is coming from create(), not insert_create_data.

> Why don't you look at $class->DB_COLUMNS instead? I fear that querying the DB
> for each column is going to slow down Bugzilla a lot. create() isn't called
> that much, but match() definitely is.

  It doesn't query the DB--it doesn't even touch the DB, except once for each request. This queries the loaded schema object.

> This doesn't look like the right place to call _check_field() IMO. I think this
> check should be done in run_create_validators() as that's its job to validate
> the data.

  No, run_create_validators gets all sorts of things that are not valid database columns. The purpose of this validator is to check that these are *valid database columns* before putting them into SQL, so this is the right place.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
>   It doesn't query the DB--it doesn't even touch the DB, except once for each
> request. This queries the loaded schema object.

Well, if you touch the DB once for each request, you are still touching it too many times I think. If match() has 20 fields, the DB will be touched 20 times, right? Or is the DB schema cached the very first time?

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> Well, if you touch the DB once for each request, you are still touching it too
> many times I think. If match() has 20 fields, the DB will be touched 20 times,
> right? Or is the DB schema cached the very first time?

  The DB schema is cached the very first time. When I say "request" I mean "CGI request".

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
>   The DB schema is cached the very first time. When I say "request" I mean "CGI
> request".

Ah ok, so that's fine. :)

Comment 10

[Max Kanat-Alexander]

Attachment: v3

Okay, this also fixes the OFFSET and LIMIT problem on HEAD.

Comment 11

[Max Kanat-Alexander]

Okay, we're just going to use this fix in bug 515191. I'd rather have total security on the branches and risk a regression, than risk leaving open some possible security issue.

Comment 12

[Max Kanat-Alexander]

The security advisory has been sent, unlocking this bug.