Project

General

Profile

Actions

Bug #2863

closed

CVE-2013-4182 - Privileges escalation via API

Added by Marek Hulán over 10 years ago. Updated over 10 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Security
Target version:
Difficulty:
Triaged:
Fixed in Releases:
Found in Releases:

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


Files

hosts_escalation.patch hosts_escalation.patch 538 Bytes Marek Hulán, 07/30/2013 10:44 AM
hosts_escalation.patch hosts_escalation.patch 2.75 KB tests not working, just for partial review Marek Hulán, 07/31/2013 11:04 AM
hosts_escalation.patch hosts_escalation.patch 4.59 KB Marek Hulán, 08/01/2013 03:03 AM
hosts_escalation.patch hosts_escalation.patch 10 KB Marek Hulán, 08/02/2013 05:01 AM
hosts_escalation.patch hosts_escalation.patch 14.6 KB final version with fixed tests that depends on all fixtures Marek Hulán, 08/02/2013 09:05 AM
0001-fixes-2863-restrict-APIs-1.2-stable.patch 0001-fixes-2863-restrict-APIs-1.2-stable.patch 15.9 KB final rebased for 1.2-stable Dominic Cleal, 08/19/2013 12:37 PM
0001-fixes-2863-restrict-APIs-1.2.0.patch 0001-fixes-2863-restrict-APIs-1.2.0.patch 15.9 KB final rebased for 1.2.0 Dominic Cleal, 08/19/2013 12:37 PM
Actions #1

Updated by Marek Hulán over 10 years ago

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

Sent a patch for review.

Actions #3

Updated by Dominic Cleal over 10 years 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 :)

Actions #4

Updated by Ohad Levy over 10 years ago

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

Actions #5

Updated by Marek Hulán over 10 years ago

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

Updated by Marek Hulán over 10 years 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.

Actions #7

Updated by Marek Hulán over 10 years 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...

Actions #8

Updated by Dominic Cleal over 10 years 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
Actions #9

Updated by Marek Hulán over 10 years 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.

Actions #10

Updated by Dominic Cleal over 10 years 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.

Actions #11

Updated by Marek Hulán over 10 years 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

Actions #12

Updated by Marek Hulán over 10 years ago

Attaching latest version with fixed tests.

Actions #13

Updated by Dominic Cleal over 10 years ago

  • Target version changed from 1.2.1 to 1.2.2

Updated by Dominic Cleal over 10 years ago

ACK, thank you Marek.

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

Actions #15

Updated by Dominic Cleal over 10 years ago

  • Private changed from Yes to No
Actions #16

Updated by Marek Hulán over 10 years ago

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

Also available in: Atom PDF