Bug #2863

CVE-2013-4182 - Privileges escalation via API

Added by Marek Hulán about 1 year ago. Updated 11 months ago.

Status:ClosedStart date:07/30/2013
Priority:NormalDue date:
Assigned To:Marek Hulán% Done:

100%

Category:Security
Target version:1.2.2
Difficulty: Bugzilla link:
Found in release: Pull request:
Story points-
Velocity based estimate-

Description

Daniel Lobato discovered that /api/hosts/<name> does not check whether a current user has a privileges to display particular host.

This is caused by using generic #find_resource in hosts controller instead of limiting it via .my_hosts scope as it's in non-API controller. Since similar bug could be in any other API controller, I'll go over all API controllers and try to find other possible issues.

EDIT: hosts seems to be only object that has customized privileges system

hosts_escalation.patch Magnifier (538 Bytes) Marek Hulán, 07/30/2013 10:44 AM

hosts_escalation.patch Magnifier - tests not working, just for partial review (2.75 KB) Marek Hulán, 07/31/2013 11:04 AM

hosts_escalation.patch Magnifier (4.59 KB) Marek Hulán, 08/01/2013 03:03 AM

hosts_escalation.patch Magnifier (10 KB) Marek Hulán, 08/02/2013 05:01 AM

hosts_escalation.patch Magnifier - final version with fixed tests that depends on all fixtures (14.6 KB) Marek Hulán, 08/02/2013 09:05 AM

0001-fixes-2863-restrict-APIs-1.2-stable.patch Magnifier - final rebased for 1.2-stable (15.9 KB) Dominic Cleal, 08/19/2013 12:37 PM

0001-fixes-2863-restrict-APIs-1.2.0.patch Magnifier - final rebased for 1.2.0 (15.9 KB) Dominic Cleal, 08/19/2013 12:37 PM

Associated revisions

Revision ce13ab5d
Added by Marek Hulán 11 months ago

fixes #2863 - restrict APIs to resources that a user is permitted to manage (CVE-2013-4182)

Revision 3b0b7cb4
Added by Marek Hulán 11 months ago

fixes #2863 - restrict APIs to resources that a user is permitted to manage (CVE-2013-4182)

History

#1 Updated by Marek Hulán about 1 year ago

  • Description updated (diff)
  • Category changed from API to Security
  • Status changed from Assigned to Ready For Testing

Sent a patch for review.

#3 Updated by Dominic Cleal about 1 year ago

Patch review (hosts_escalation.patch, please upload new revisions of the patch as new attachments):

1. my main concern is that it slightly breaks the implied contract of resource_class by returning an AR::Relation instead of the class. While this works as we can call find_by_* on it, I'm a bit concerned. Maybe we can document the base method to say that either the class or a scope is acceptable, or better, add a second method that explicitly returns a scope and use that instead.

2. tests please :)

#4 Updated by Ohad Levy about 1 year ago

what about compute resources, don't we have the same problem there?

#5 Updated by Marek Hulán about 1 year ago

  • Subject changed from Privileges escalation via API to CVE-2013-4182 - Privileges escalation via API

#6 Updated by Marek Hulán about 1 year ago

Dominic Cleal wrote:

Patch review (hosts_escalation.patch, please upload new revisions of the patch as new attachments):

1. my main concern is that it slightly breaks the implied contract of resource_class by returning an AR::Relation instead of the class. While this works as we can call find_by_* on it, I'm a bit concerned. Maybe we can document the base method to say that either the class or a scope is acceptable, or better, add a second method that explicitly returns a scope and use that instead.

I agree but since AR::Relation is usually used in exactly same way as a model and is some kind of a 'proxy' object I wouldn't see this as a big problem. I like the idea that we could add and use new resource_scope method which would by default run .scoped on resource_class.

2. tests please :)

Correct, will upload with new version.

#7 Updated by Marek Hulán about 1 year ago

Ohad was right. Full patch will include fix for compute resources as well. Attaching current fix with not working test. Also tests will use as_user helper instead...

#8 Updated by Dominic Cleal about 1 year ago

Try this:

  test "should not allow access to a host out of users hosts scope" do
    users(:three).roles = [Role.find_by_name('Anonymous'), Role.find_by_name('Viewer')]
    as_user :three do
      get :show, { :id => hosts(:one).to_param }, set_session_user.merge(:user => users(:three).id)
    end
    assert_response :not_found
  end

#9 Updated by Marek Hulán 12 months ago

Thanks Dominic. I did it via fixtures. I'm still not sure why it didn't work with fixture names (see user_roles.yml), probably because of fixed ids of roles. Rails generated random ids for them. However this patch should solve both hosts and compute resources vulnerabilities with test included.

One thing I'd like to ask. Do you think that checking of show action is sufficient or should I cover all controller actions that were affected? They use the same logic but if someone customize any of them in future (update/delete) he could make the same mistake.

#10 Updated by Dominic Cleal 12 months ago

Marek Hulán wrote:

Thanks Dominic. I did it via fixtures. I'm still not sure why it didn't work with fixture names (see user_roles.yml), probably because of fixed ids of roles. Rails generated random ids for them.

Yeah, agreed. We normally assign roles on the fly in tests, but I think this is OK too.

One thing I'd like to ask. Do you think that checking of show action is sufficient or should I cover all controller actions that were affected? They use the same logic but if someone customize any of them in future (update/delete) he could make the same mistake.

I think just the show action is sufficient as find_resource is used consistently, but other tests would be welcome. I can't do the 1.2.1 release until the start of next week due to lack of time, so you have until then if you want to improve it :)

Review comments:
- resource_scope is public in the base controller (correct IMO), but visibility has been changed to private in hosts/compute_resources controllers, it should still be public
- renaming the user to something more descriptive would be useful ("restricted"/"filtered"?), just so people don't reuse it for other purposes in the future and subtly break the tests

Otherwise good, thanks.

#11 Updated by Marek Hulán 12 months ago

Added more tests for hosts and computed resource. Although I'm not sure about having find methods in public scope I moved it from private to public in both controllers to keep consistency. Also I renamed user to "restricted". Please review again.

EDIT: few other test were affected by modifying fixtures (I wish we migrate to factories), I'll look at that, should be just minor changes

#12 Updated by Marek Hulán 12 months ago

Attaching latest version with fixed tests.

#13 Updated by Dominic Cleal 12 months ago

  • Target version changed from 1.2.1 to 1.2.2

#14 Updated by Dominic Cleal 12 months ago

ACK, thank you Marek.

Attaching rebased versions for 1.2.0 (as used downstream) and 1.2-stable (for 1.2.2).

#15 Updated by Dominic Cleal 11 months ago

  • Private changed from Yes to No

#16 Updated by Marek Hulán 11 months ago

  • Status changed from Pending to Closed
  • % Done changed from 0 to 100

Also available in: Atom PDF