Closed Bug 1090275 Opened 7 years ago Closed 6 years ago

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

Categories

(Bugzilla :: WebService, defect)

2.23.3
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: dkl, Assigned: dkl)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
(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
(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.
Attached patch 1090275_1.patch (obsolete) — Splinter Review
Assignee: webservice → dkl
Status: NEW → ASSIGNED
Attachment #8522478 - Flags: review?(LpSolit)
Is REST also affected by this problem? If not, why?
(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
Severity: normal → major
Flags: blocking5.0?
Target Milestone: --- → Bugzilla 4.0
Flags: blocking5.0? → blocking5.0+
Depends on: 1085182
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-
Attached patch 1090275_2.patch (obsolete) — Splinter Review
Attachment #8522478 - Attachment is obsolete: true
Attachment #8533297 - Flags: review?(dylan)
Wouldn't it be more readable if methods were listed alphabetically?
Attached patch 1090275_3.patch (obsolete) — Splinter Review
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
(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?
Attached patch 1090275_4.patchSplinter Review
Added PUBLIC_METHODS to the Example extension. Moving forward r+.
Attachment #8533853 - Attachment is obsolete: true
Attachment #8537457 - Flags: review+
Attached patch 1090275_44_1.patch (obsolete) — Splinter Review
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?
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)
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)
Attachment #8540364 - Flags: review?(dylan)
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+
Flags: approval4.2?
Flags: approval4.0?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Blocks: 1118994
Version: unspecified → 2.23.3
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
Closed: 6 years ago
Resolution: --- → FIXED
Released
Group: bugzilla-security
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 → ---
Blocks: 1124716
tracking in new bug 1124716
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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. :(
Added to relnotes for 5.0rc3.
Keywords: relnote
Blocks: 1154099
Blocks: 1232186
Flags: documentation?
You need to log in before you can comment on or make changes to this bug.