If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

hash keys containing non-word characters such as dashes are dropped from web service params

RESOLVED WONTFIX

Status

()

Bugzilla
WebService
RESOLVED WONTFIX
7 years ago
7 years ago

People

(Reporter: dkl, Unassigned)

Tracking

3.6.1
x86
Mac OS X

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Created attachment 459155 [details] [diff] [review]
Patch to allow hash keys in web services that contain dashes (v1)

I noticed that params containing dashes were being dropped from out web service params such as field0-0-0, etc. and therefore breaking out bug search methods in 3.6. In Bugzilla::WebService::Util::_delete_bad_keys (called by taint_data()) is
performing a \w+ match on the hash key and dropping those that do not match.

            if ($key !~ /^\w+$/) {
                delete $item->{$key};
            }

If I change that to $key !~ /^[\w-]+$/ it will no longer drop keys containing dashes.

Attaching a patch for 3.6.

Dave
Attachment #459155 - Flags: review?(mkanat)

Comment 1

7 years ago
Comment on attachment 459155 [details] [diff] [review]
Patch to allow hash keys in web services that contain dashes (v1)

Unfortunately, "-" is not legal in a SQL identifier, and params are, in some cases, inserted directly into SQL. There are probably situations in which "-" could be used to inject SQL or modify the behavior of Bugzilla, since it means "minus" in SQL.

For your WebServices, you may have to put on a little frontend bit that converts the "field-" to "field_".
Attachment #459155 - Flags: review?(mkanat) → review-

Comment 2

7 years ago
Because this is a security issue, for the reasons above, I'm unfortunately going to have to mark this WONTFIX.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 3

7 years ago
(In reply to comment #1)
> Comment on attachment 459155 [details] [diff] [review]
> Patch to allow hash keys in web services that contain dashes (v1)
> 
> Unfortunately, "-" is not legal in a SQL identifier, and params are, in some
> cases, inserted directly into SQL. There are probably situations in which "-"
> could be used to inject SQL or modify the behavior of Bugzilla, since it means
> "minus" in SQL.
> 
> For your WebServices, you may have to put on a little frontend bit that
> converts the "field-" to "field_".

This will need more thought as I would assume that the webservices API will eventually expand to allow similar search capability that web UI provides. Maybe converting the web UI to use "field0_0_0" instead of "field0-0-0"
for example?

When you say add a frontend bit for conversion, do you mean customize _delete_bad_keys to do the following?:


sub _delete_bad_keys {
    foreach my $item (@_) {
        next if ref $item ne 'HASH';
        foreach my $key (keys %$item) {
            $key =~ s/-/_/g;
            if ($key !~ /^[\w-]+$/) {
                delete $item->{$key};
            }
        }
    }
    return @_;
}
(Reporter)

Comment 4

7 years ago
(In reply to comment #3) 
> sub _delete_bad_keys {
>     foreach my $item (@_) {
>         next if ref $item ne 'HASH';
>         foreach my $key (keys %$item) {
>             $key =~ s/-/_/g;
>             if ($key !~ /^[\w-]+$/) {
>                 delete $item->{$key};
>             }

Acutally it would be:

sub _delete_bad_keys {
    foreach my $item (@_) {
        next if ref $item ne 'HASH';
        foreach my $key (keys %$item) {
            my $old_key = $key;
            $key =~ s/-/_/g;
            if ($key !~ /^[\w-]+$/) {
                delete $item->{$old_key};
            }
        }
    }
    return @_;
}

Comment 5

7 years ago
(In reply to comment #3)
> This will need more thought as I would assume that the webservices API will
> eventually expand to allow similar search capability that web UI provides.

  Yeah. But it most likely won't do that until after the boolean chart redesign, and will have some data structure as input.

> When you say add a frontend bit for conversion, do you mean customize
> _delete_bad_keys to do the following?:

  Yeah, that would work. (With of course the fix you mentioned in your next comment.)
You need to log in before you can comment on or make changes to this bug.