Bug #2863
closedCVE-2013-4182 - Privileges escalation via API
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
Updated by Marek Hulán over 11 years ago
- Description updated (diff)
- Category changed from API to Security
- Status changed from Assigned to Ready For Testing
Sent a patch for review.
Updated by Marek Hulán over 11 years ago
- File hosts_escalation.patch hosts_escalation.patch added
Updated by Dominic Cleal over 11 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 :)
Updated by Ohad Levy over 11 years ago
what about compute resources, don't we have the same problem there?
Updated by Marek Hulán over 11 years ago
- Subject changed from Privileges escalation via API to CVE-2013-4182 - Privileges escalation via API
Updated by Marek Hulán over 11 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.
Updated by Marek Hulán over 11 years ago
- File hosts_escalation.patch hosts_escalation.patch added
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...
Updated by Dominic Cleal over 11 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
Updated by Marek Hulán over 11 years ago
- File hosts_escalation.patch hosts_escalation.patch added
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.
Updated by Dominic Cleal over 11 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.
Updated by Marek Hulán over 11 years ago
- File hosts_escalation.patch hosts_escalation.patch added
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
Updated by Marek Hulán over 11 years ago
- File hosts_escalation.patch hosts_escalation.patch added
Attaching latest version with fixed tests.
Updated by Dominic Cleal over 11 years ago
- Target version changed from 1.2.1 to 1.2.2
Updated by Dominic Cleal about 11 years ago
- File 0001-fixes-2863-restrict-APIs-1.2.0.patch 0001-fixes-2863-restrict-APIs-1.2.0.patch added
- File 0001-fixes-2863-restrict-APIs-1.2-stable.patch 0001-fixes-2863-restrict-APIs-1.2-stable.patch added
- Status changed from Ready For Testing to Pending
ACK, thank you Marek.
Attaching rebased versions for 1.2.0 (as used downstream) and 1.2-stable (for 1.2.2).
Updated by Marek Hulán about 11 years ago
- Status changed from Pending to Closed
- % Done changed from 0 to 100
Applied in changeset ce13ab5d1197c128acccd0725c06a2526e19b4ac.