513593 - The WebService should taint incoming parameters

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect, P1)

Description

[Max Kanat-Alexander]

The WebService currently doesn't taint incoming parameters. This doesn't currently cause any security issues that I'm aware of, but we should still make it taint them. I suspect we'd have to pull in Taint::Util from CPAN or something like that in order to do this.

Comment 1

[Max Kanat-Alexander]

Attachment: Work In Progress

Here's a work in progress. I still need to implement _taint_som.

Comment 2

[Max Kanat-Alexander]

I'm going to mark this as a sec bug until we're certain that this isn't causing any security issues.

Comment 3

[Max Kanat-Alexander]

Attachment: v1

Without any testing of whether or not the actual tainting is causing any trouble, this taints all incoming WebService parameters.

I made Taint::Util required instead of optional because we can replace trick_taint with untaint(), if we have it.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 399361 [details] [diff] [review]
v1

Okay, I've tested this against the current WebService QA tests, and they all still pass, actually. I'm still investigating if there are any unfound security issues, but for now, we might as well review and check in this patch.

I'm asking dkl for review because, as I recall, he was the reviewer for the JSON-RPC stuff.

Comment 5

[Bradley Baetz (:bbaetz)]

+            # We know that all hash keys passed in to the WebService will 
+            # match \w+, so we delete any key that doesn't match that.
+            if ($key !~ /^\w+$/) {
+                delete $container->{$key};

We use - for some params, or are those mapped to something else for the webservices?

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> We use - for some params, or are those mapped to something else for the
> webservices?

  They're mapped, for sure, if we even have any of those remaining in our own code.

Comment 7

[Bradley Baetz (:bbaetz)]

Yeah, doesn't look like we allow the boolean chart stuff (which was what I was thinking of)

Comment 8

[Frédéric Buclin]

Comment on attachment 399361 [details] [diff] [review]
v1

>Index: Bugzilla/Install/Requirements.pm

>+        module  => 'Taint::Util',
>+        module  => 'Data::Walk',

Why don't you use Test::Taint instead of both modules above? taint_deeply() seems to do what you want.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Why don't you use Test::Taint instead of both modules above? taint_deeply()
> seems to do what you want.

  Oh, I didn't know that. I figured it was just for tests. I'd still need a way to validate hash keys on a lower level than the main $params, though, so probably best to just stay with Data::Walk.

Comment 10

[Frédéric Buclin]

But Test::Taint offers test capabilities which may be useful for our own testing, which Taint::Util has not.

Comment 11

[Max Kanat-Alexander]

 There are no unfound security issues by my audit, and the other bugs are now public, so we can unlock this one.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 399361 [details] [diff] [review]
v1

not ok 133 - Bugzilla/WebService/Util.pm --ERROR
#   Failed test 'Bugzilla/WebService/Util.pm --ERROR'
#   at t/001compile.t line 110.
Global symbol "$params" requires explicit package name at Bugzilla/WebService/Util.pm line 57.
Bugzilla/WebService/Util.pm had compilation errors.
not ok 134 - Bugzilla/WebService/Product.pm --ERROR
#   Failed test 'Bugzilla/WebService/Product.pm --ERROR'
#   at t/001compile.t line 110.
Global symbol "$params" requires explicit package name at Bugzilla/WebService/Util.pm line 57.
Compilation failed in require at Bugzilla/WebService/Product.pm line 24.
BEGIN failed--compilation aborted at Bugzilla/WebService/Product.pm line 24.
ok 135 - Bugzilla/WebService/Bugzilla.pm
ok 136 - Bugzilla/WebService/Server.pm
not ok 137 - Bugzilla/WebService/User.pm --ERROR
#   Failed test 'Bugzilla/WebService/User.pm --ERROR'
#   at t/001compile.t line 110.
Global symbol "$params" requires explicit package name at Bugzilla/WebService/Util.pm line 57.
Compilation failed in require at Bugzilla/WebService/User.pm line 31.
BEGIN failed--compilation aborted at Bugzilla/WebService/User.pm line 31.
not ok 138 - Bugzilla/WebService/Bug.pm --ERROR
#   Failed test 'Bugzilla/WebService/Bug.pm --ERROR'
#   at t/001compile.t line 110.
Global symbol "$params" requires explicit package name at Bugzilla/WebService/Util.pm line 57.
Compilation failed in require at Bugzilla/WebService/Bug.pm line 30.
BEGIN failed--compilation aborted at Bugzilla/WebService/Bug.pm line 30.
not ok 139 - Bugzilla/WebService/Server/JSONRPC.pm --ERROR
#   Failed test 'Bugzilla/WebService/Server/JSONRPC.pm --ERROR'
#   at t/001compile.t line 110.
Global symbol "$params" requires explicit package name at Bugzilla/WebService/Util.pm line 57.
Compilation failed in require at Bugzilla/WebService/Server/JSONRPC.pm line 29.
BEGIN failed--compilation aborted at Bugzilla/WebService/Server/JSONRPC.pm line 29.
not ok 140 - Bugzilla/WebService/Server/XMLRPC.pm --ERROR
#   Failed test 'Bugzilla/WebService/Server/XMLRPC.pm --ERROR'
#   at t/001compile.t line 110.
Global symbol "$params" requires explicit package name at Bugzilla/WebService/Util.pm line 57.
Compilation failed in require at Bugzilla/WebService/Server/XMLRPC.pm line 145.
BEGIN failed--compilation aborted at Bugzilla/WebService/Server/XMLRPC.pm line 145.

Comment 13

[Max Kanat-Alexander]

Attachment: v2

Okay, this version compiles, and I switched to using Test::Taint since it's only one module that we have to require, that way.

Comment 14

[David Lawrence [:dkl]]

I have tested this and works for me. r=dkl

Comment 15

[Max Kanat-Alexander]

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

Hey, great. Could you just grant r+? I only need one r+ for this.

Comment 16

[David Lawrence [:dkl]]

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

r=dkl

Comment 17

[Max Kanat-Alexander]

Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.73; previous revision: 1.72
done
Checking in Bugzilla/WebService/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Util.pm,v  <--  Util.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/WebService/Server/JSONRPC.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm,v  <--  JSONRPC.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/WebService/Server/XMLRPC.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/XMLRPC.pm,v  <--  XMLRPC.pm
new revision: 1.7; previous revision: 1.6
done

Comment 18

[Max Kanat-Alexander]

Added to the release notes in bug 547466.