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

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
WebService
--
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

2.23.3
Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +
approval5.0 +
blocking5.0 +
approval4.4 +
approval4.2 +
approval4.0 +
documentation ?

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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
Summary: WebServices modules should maintain a whitelist of methods that are allowed instead of allowing access to any function imported into it's namespace → WebServices modules should maintain a whitelist of methods that are allowed instead of allowing access to any function imported into its namespace
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.
(Assignee)

Comment 2

3 years ago
(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

3 years ago
(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?
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8522478 [details] [diff] [review]
1090275_1.patch
Assignee: webservice → dkl
Status: NEW → ASSIGNED
Attachment #8522478 - Flags: review?(LpSolit)

Comment 6

3 years ago
Is REST also affected by this problem? If not, why?
(Assignee)

Comment 7

3 years ago
(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

Updated

3 years ago
Severity: normal → major
Flags: blocking5.0?
Target Milestone: --- → Bugzilla 4.0
Flags: blocking5.0? → blocking5.0+
(Assignee)

Updated

3 years ago
Depends on: 1085182
(Assignee)

Updated

3 years ago
Attachment #8522478 - Flags: review?(LpSolit) → review?(dylan)
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 ))
Attachment #8522478 - Flags: review?(dylan) → review-
(Assignee)

Comment 9

3 years ago
Created attachment 8533297 [details] [diff] [review]
1090275_2.patch
Attachment #8522478 - Attachment is obsolete: true
Attachment #8533297 - Flags: review?(dylan)

Comment 10

3 years ago
Wouldn't it be more readable if methods were listed alphabetically?
(Assignee)

Comment 11

3 years ago
Created attachment 8533853 [details] [diff] [review]
1090275_3.patch

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

dkl
Attachment #8533297 - Attachment is obsolete: true
Attachment #8533297 - Flags: review?(dylan)
Attachment #8533853 - Flags: review?(dylan)
Comment on attachment 8533853 [details] [diff] [review]
1090275_3.patch

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

r=dylan
Attachment #8533853 - Flags: review?(dylan) → review+
Flags: approval?
Flags: approval5.0?
the example extension needs to be updated too; please do so on commit.
Flags: documentation?
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval+
Keywords: relnote

Comment 14

3 years ago
(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?
(Assignee)

Comment 15

3 years ago
Created attachment 8537457 [details] [diff] [review]
1090275_4.patch

Added PUBLIC_METHODS to the Example extension. Moving forward r+.
Attachment #8533853 - Attachment is obsolete: true
Attachment #8537457 - Flags: review+
(Assignee)

Comment 16

3 years ago
Created attachment 8538225 [details] [diff] [review]
1090275_44_1.patch

Patch for 4.4
Attachment #8538225 - Flags: review?(dylan)
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
Attachment #8538225 - Flags: review?(dylan) → review+
Flags: approval4.4?
(Assignee)

Comment 18

3 years ago
Created attachment 8538741 [details] [diff] [review]
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.
Attachment #8538225 - Attachment is obsolete: true
Attachment #8538741 - Flags: review+
Flags: needinfo?(dkl)
Flags: approval4.4?
Flags: approval4.4+
Flags: needinfo?(dkl)
(Assignee)

Comment 19

3 years ago
Created attachment 8540333 [details] [diff] [review]
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
Attachment #8540333 - Flags: review?(dylan)
(Assignee)

Comment 20

3 years ago
Created attachment 8540364 [details] [diff] [review]
1090275-40_1.patch
Attachment #8540364 - Flags: review?(dylan)

Updated

3 years ago
No longer depends on: 1085182
Comment on attachment 8540333 [details] [diff] [review]
1090275_42_1.patch

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

r=dylan
Attachment #8540333 - Flags: review?(dylan) → review+
Comment on attachment 8540364 [details] [diff] [review]
1090275-40_1.patch

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

r=dylan
Attachment #8540364 - Flags: review?(dylan) → review+

Updated

3 years ago
Flags: approval4.2?
Flags: approval4.0?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
(Assignee)

Updated

3 years ago
Blocks: 1118994

Updated

3 years ago
Version: unspecified → 2.23.3
(Assignee)

Comment 23

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   4dabf1a..1612292  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   19117cc..211464d  5.0 -> 5.0

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f5b9cba..ce85e36  4.4 -> 4.4

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   7e1b7a5..17e8ba8  4.2 -> 4.2

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   5c7b317..828e407  4.0 -> 4.0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

3 years ago
Released
Group: bugzilla-security
(Assignee)

Updated

3 years ago
Blocks: 1124437
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

3 years ago
Blocks: 1124716
(Assignee)

Comment 26

3 years ago
tracking in new bug 1124716
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Comment 27

3 years ago
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

2 years ago
Added to relnotes for 5.0rc3.
Keywords: relnote

Updated

2 years ago
Blocks: 1154099

Updated

2 years ago
Blocks: 1232186
You need to log in before you can comment on or make changes to this bug.