1090275 - WebServices modules should maintain a whitelist of methods that are allowed instead of allowing access to any function imported into its namespace

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[David Lawrence [:dkl]]

Broken out from bug 1085182 to be tracked a separate issue.

In the XML and JSON APIs the dispatching system uses the module namespace to limit the available API methods. Bugzilla also uses exporter extensively, so this results in namespaces having many inappropriate methods available. Out of the methods available this way in 4.4.6, User.bz_locations and Bug.WS_DISPATCH, stand out as the ones that could be argued to return sensitive internal data. 

Two possible solutions:

1) Using a whitelist of allowed method names for the XML/JSON API namespaces 

We can add a ALLOWED_METHODS constant to each WS related module and only those methods will be allowed. If a method is used not in the list, an error is thrown.

2) Avoid importing any functions into these namespaces by adding the empty list to your "use" statements and qualify all function/method calls that go outside of the module with their full namespace.

Instead of:

use Bugzilla::Constants;
bz_locations();

you write

use Bugzilla::Constants ();
Bugzilla::Constants::bz_locations()

Personally I feel 1) is a better approach as it gives us full explicit control of the methods allowed. If we add another 'use module' in the future to our current WS modules to bring in new functionality, if the module being use'ed has methods that are automatically imported we may not catch it and have the same issue over again.

Thoughts?
dkl

Comment 1

[:glob ✱]

a whitelist seems to be the best approach, especially since adding new methods is a rare occurrence.

note: we need to ensure that extensions which add webservice methods can also add whitelists and are protected by this feature.

Comment 2

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #1)
> a whitelist seems to be the best approach, especially since adding new
> methods is a rare occurrence.
> 
> note: we need to ensure that extensions which add webservice methods can
> also add whitelists and are protected by this feature.

glob mentioned in IRC that prefixing the methods in the WS modules with public_ (or something similar) and then have the backend replace the caller's method with the actual one internally may be better solution. We already disallow any calls to methods that start with underscore '_'.

For example, a caller using Bug.get would get translated to Bug.public_get internally and then throw an error if the method does not exist.

This is also better in that setting of ALLOWED_METHODS constants in each module is not extension friendly if an extension is trying to extend a current namespace such as adding Bug.some_ext_method.
But just looking for a prefix is not as difficult.

Whatever we go with will break existing extensions and will need to be updated to work with the new functionality.

dkl

Comment 3

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #2)
> For example, a caller using Bug.get would get translated to Bug.public_get
> internally and then throw an error if the method does not exist.

If WS module A uses another WS module B, and both modules have public_* methods, how do you prevent A.foo from calling B.public_foo despite A.public_foo doesn't exist?

Comment 4

[Dylan Hardison [:dylan] (he/him)]

For imported functions, we could clear them with namespace::clean.
But that is only part of the problem. A whitelist is clearly a better choice.

I see three possible options, each with its own drawback:


1) Whitelist method registry, ideally as a class method on the relevant class.
Class->ws_allowed_methods() returns a hashref of (MethodName => 1). Extensions that inject methods into other classes can either wrap the method or modify its return value (it should be in the contract of the method that the return value is a modifiable reference, e.g. not cloned).

2) Prefix method names, as mentioned in comment #2. The drawback of this is having to rename all the methods we currently provided. I'm not sure how this will effect our test that checks for documented methods, as well. But it is one of the easiest approaches.

3) Use subroutine attributes. This doesn't require any additional dependencies.
So, to use Bug.get as an example:

package Bug;
...

sub get :Public {


}

Tagging methods with information is exactly what subroutine attributes are for. The downside of this is similar to renaming all the methods, but we don't have to worry about *accidental* public methods (if we don't use namespace::clean, and our chosen prefix (public_?) is imported into the package, it will be public.)

The likelihood of an imported subroutine having an attribute is very, very low.

Comment 5

[David Lawrence [:dkl]]

Attachment: 1090275_1.patch

Comment 6

[Frédéric Buclin]

Is REST also affected by this problem? If not, why?

Comment 7

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #6)
> Is REST also affected by this problem? If not, why?

No as the REST resources are basically very specific regular expressions that match to specific methods in the WebService modules. It should not be possible for someone to use a REST path to access any other subroutine that we have not specified. Otherwise they get an error message each time.

dkl

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8522478 [details] [diff] [review]
1090275_1.patch

Review of attachment 8522478 [details] [diff] [review]:
-----------------------------------------------------------------

A few suggested changes.

r-

::: Bugzilla/WebService/Server/JSONRPC.pm
@@ +404,5 @@
>          }
>      }
>  
> +    # Only allowed methods to be used from our whitelist
> +    if (!grep($_ eq $method, $pkg->PUBLIC_METHODS)) {

any { } (from List::Util) would be better here, as it will stop (short-circuit) after the first match.

::: Bugzilla/WebService/Server/XMLRPC.pm
@@ +97,5 @@
>      my ($self, $classes, $action, $uri, $method) = @_;
>      my $class = $classes->{$uri};
>      my $full_method = $uri . "." . $method;
> +    # Only allowed methods to be used from the module's whitelist
> +    eval("require $class") || die $@;

I would recommend using this for a variety of reasons (speed, safety, etc)

my $file = $class;
$file =~ s{::}{/}g; $file .= ".pm";
require $file;

(a better solution would Class::Load->load_class(), but I won't suggest that dependency right now.)

@@ +98,5 @@
>      my $class = $classes->{$uri};
>      my $full_method = $uri . "." . $method;
> +    # Only allowed methods to be used from the module's whitelist
> +    eval("require $class") || die $@;
> +    if (!grep($_ eq $method, $class->PUBLIC_METHODS)) {

s/grep/any/;
(use List::Util qw( any ))

Comment 9

[David Lawrence [:dkl]]

Attachment: 1090275_2.patch

Comment 10

[Frédéric Buclin]

Wouldn't it be more readable if methods were listed alphabetically?

Comment 11

[David Lawrence [:dkl]]

Attachment: 1090275_3.patch

New patch with public methods alphabetically sorted. (yay vim :sort).

dkl

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8533853 [details] [diff] [review]
1090275_3.patch

Review of attachment 8533853 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 13

[:glob ✱]

the example extension needs to be updated too; please do so on commit.

Comment 14

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #13)
> the example extension needs to be updated too; please do so on commit.

What needs to be updated there? Also, it's not ok to do fixes on checkin for a security bug. Linux distros usually pick patches directly from here and do not look at what has been committed. And shouldn't we wait for 4.4 backports before approving this bug?

Comment 15

[David Lawrence [:dkl]]

Attachment: 1090275_4.patch

Added PUBLIC_METHODS to the Example extension. Moving forward r+.

Comment 16

[David Lawrence [:dkl]]

Attachment: 1090275_44_1.patch

Patch for 4.4

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8538225 [details] [diff] [review]
1090275_44_1.patch

Review of attachment 8538225 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan, please fix the indentation on Bugzilla::WebService::Server::XMLRPC::new() before commit.

::: Bugzilla/WebService/Server/XMLRPC.pm
@@ +38,5 @@
> +sub new {
> +my $class = shift;
> +my $self = $class->SUPER::new(@_);
> +$self->{debug_logger} = sub {};
> +return $self;

nit: this isn't indented

Comment 18

[David Lawrence [:dkl]]

Attachment: 1090275_44_2.patch

That new() was actually not supposed to be there and was added simply to allow the webservice tests to work. Removed and moving r+ forward.

Comment 19

[David Lawrence [:dkl]]

Attachment: 1090275_42_1.patch

Realized we also need sep patches for 4.2 and 4.0 as the 4.4 will not apply. Here is 4.2. 4.0 coming right up.

dkl

Comment 20

[David Lawrence [:dkl]]

Attachment: 1090275-40_1.patch

Comment 21

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8540333 [details] [diff] [review]
1090275_42_1.patch

Review of attachment 8540333 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 22

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8540364 [details] [diff] [review]
1090275-40_1.patch

Review of attachment 8540364 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 23

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   4dabf1a..1612292  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   19117cc..211464d  5.0 -> 5.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   f5b9cba..ce85e36  4.4 -> 4.4

To ssh://[e-mail]/bugzilla/bugzilla.git
   7e1b7a5..17e8ba8  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   5c7b317..828e407  4.0 -> 4.0

Comment 24

[David Lawrence [:dkl]]

Released

Comment 25

[:glob ✱]

this doesn't pass tests, and burned all the trees:

#   Failed test 'Bugzilla/WebService/Server/JSONRPC.pm has 1 error(s):
# user error tag 'unknown_method' is used at line(s) (410) but not defined for language(s): any'
#   at t/012throwables.t line 202.
# Looks like you failed 1 test of 217.

Comment 26

[David Lawrence [:dkl]]

tracking in new bug 1124716

Comment 27

[Frédéric Buclin]

Comment on attachment 8540364 [details] [diff] [review]
1090275-40_1.patch

>+++ b/Bugzilla/WebService/Product.pm

>++use constant PUBLIC_METHODS => qw(
>++    get
>++    get_accessible_products
>++    get_enterable_products
>++    get_selectable_products
>++);

This code is wrong! ++ means that all these lines now start with a "+". runtests.pl caught it, but nobody paid attention to it. :(

Comment 28

[Frédéric Buclin]

Added to relnotes for 5.0rc3.