CVE-2016-7047: API leaks any MiqReportResult #15472
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This query was missing as a scope, however we have rules to compose RerportsTree on the ui, that uses this exact same set of rules. Relates to cve-2016-7047.
Show list only of those reports he or she is authorized to. Relates to CVE-2016-7047
And fix tests. Relates to CVE-2016-7047
And fix tests. Relates to CVE-2016-7047
And fix tests. Relates to CVE-2016-7047
And fix tests. Relates to CVE-2016-7047
|
Some comments on commits isimluk/manageiq@80eff5a~...36e042b spec/requests/api/reports_spec.rb
|
|
Checked commits isimluk/manageiq@80eff5a~...36e042b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@abellotti: this looks good to me, but there are also API changes. Please, merge if it looks ok to you. |
|
It's better merge as it is. We have this backported to 5.7.z already. If we are going to make changes to this, they should go to separate pr, so they don't get lost. |
isimluk
changed the title
|
Merging as this has already been pushed to euwe. |
|
euwe commit is 6c833cc cc @imtayadeway |
simaishi
pushed a commit
that referenced
this pull request
Jun 30, 2017
CVE-2016-7047: API leaks any MiqReportResult (cherry picked from commit a13b67b) https://bugzilla.redhat.com/show_bug.cgi?id=1450493
|
Fine backport details: |
gtanzillo
added a commit
to gtanzillo/manageiq
that referenced
this pull request
Dec 21, 2017
This change expands the existing scope of the current user's group to the current user's tenant allowing visibility to all reports created by users within the tenant. The scope was changed in ManageIQ#15472 but was too restrictive Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526058
gtanzillo
added a commit
to gtanzillo/manageiq
that referenced
this pull request
Jan 2, 2018
This change expands the existing scope of the current user's group to the current user's tenant allowing visibility to all reports created by users within the tenant. The scope was changed in ManageIQ#15472 but was too restrictive Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526058
gtanzillo
added a commit
to gtanzillo/manageiq
that referenced
this pull request
Jan 4, 2018
This change expands the existing scope of the current user's group to the current user's tenant allowing visibility to all reports created by users within the tenant. The scope was changed in ManageIQ#15472 but was too restrictive Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526058
gtanzillo
added a commit
to gtanzillo/manageiq
that referenced
this pull request
Jan 4, 2018
This change expands the existing scope of the current user's group to the current user's tenant allowing visibility to all reports created by users within the tenant. The scope was changed in ManageIQ#15472 but was too restrictive Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526058
gtanzillo
added a commit
to gtanzillo/manageiq
that referenced
this pull request
Jan 4, 2018
This change expands the existing scope of the current user's group to the current user's tenant allowing visibility to all reports created by users within the tenant. The scope was changed in ManageIQ#15472 but was too restrictive Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526058
gtanzillo
added a commit
to gtanzillo/manageiq
that referenced
this pull request
Jan 4, 2018
This change expands the existing scope of the current user's group to the current user's tenant allowing visibility to all reports created by users within the tenant. The scope was changed in ManageIQ#15472 but was too restrictive Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526058
NickLaMuro
added a commit
to NickLaMuro/manageiq-api
that referenced
this pull request
Dec 5, 2018
The fix here simply matches up the `if` checks used in: - `Api::ReportsController#reports_search_conditions` - `MiqReport.for_user` to both use `User#report_admin_user?` * * * Further background info: This bug is the result of a collision between the following two PRs: - ManageIQ/manageiq#15472 - ManageIQ/manageiq#17444 The first PR, ManageIQ/manageiq#15472, introduces: `Api::ReportsController#reports_search_conditions` Which uses `unless User.current_user.admin?` to check if it should use the `where_clause` from the `MiqReport.for_user` for the search conditions (passed into a `ActiveRecord` `.where` farther up the stack). The idea for this initial check is building this where clause only makes sense if it isn't an `all`, otherwise where it is used will cause an error in Postgres syntax because of `... WHERE ()` being passed This, however, doesn't match the `user.admin_user?`(at the time of ManageIQ/manageiq#15472) check that is done in `.for_user` method, which means it would be called for users that don't match the `admin` username, but still are part of either of the following roles - `"EvmRole-super_administrator"` - `"EvmRole-administrator"` The bug in question: https://bugzilla.redhat.com/show_bug.cgi?id=1650531 tested this with a custom role, which doesn't match either of those two roles, so the `User#admin?` check and `User#admin_user?` check happened to be equivalent in `5.9` (see comment ManageIQ#4 in BZ link), but not `5.10`. But, if the replication steps had instead said to: - Create a user, "User1", that is part of the "EvmGroup-administrator" group This would have also failed in the same way on `5.9` as it does on `5.10` currently. This fails on 5.10, though, because in ManageIQ/manageiq#17444, the `admin_user?` method changes to `report_admin_user?`, and it's functionality also now checks if the role is valid for querying the group role's product features the user has access to, and it only has an eject for the role that happens to be `admin?`. Meaning that the `admin` "super user" still has no problem with this query, but any other user that could still count as a `report_admin_user?` would (both users in the `EvmGroup-administrator` role and just having the proper features enabled)
NickLaMuro
added a commit
to NickLaMuro/manageiq-api
that referenced
this pull request
Dec 5, 2018
The fix here simply matches up the `if` checks used in: - `Api::ReportsController#reports_search_conditions` - `MiqReport.for_user` to both use `User#report_admin_user?` * * * Further background info: This bug is the result of a collision between the following two PRs: - ManageIQ/manageiq#15472 - ManageIQ/manageiq#17444 The first PR, ManageIQ/manageiq#15472, introduces: `Api::ReportsController#reports_search_conditions` Which uses `unless User.current_user.admin?` to check if it should use the `where_clause` from the `MiqReport.for_user` for the search conditions (passed into a `ActiveRecord` `.where` farther up the stack). The idea for this initial check is building this where clause only makes sense if it isn't an `all`, otherwise where it is used will cause an error in Postgres syntax because of `... WHERE ()` being passed This, however, doesn't match the `user.admin_user?`(at the time of ManageIQ/manageiq#15472) check that is done in `.for_user` method, which means it would be called for users that don't match the `admin` username, but still are part of either of the following roles - `"EvmRole-super_administrator"` - `"EvmRole-administrator"` The bug in question: https://bugzilla.redhat.com/show_bug.cgi?id=1650531 tested this with a custom role, which doesn't match either of those two roles, so the `User#admin?` check and `User#admin_user?` check happened to be equivalent in `5.9` (see comment ManageIQ#4 in BZ link), but not `5.10`. But, if the replication steps had instead said to: - Create a user, "User1", that is part of the "EvmGroup-administrator" group This would have also failed in the same way on `5.9` as it does on `5.10` currently. This fails on 5.10, though, because in ManageIQ/manageiq#17444, the `admin_user?` method changes to `report_admin_user?`, and it's functionality also now checks if the role is valid for querying the group role's product features the user has access to, and it only has an eject for the role that happens to be `admin?`. Meaning that the `admin` "super user" still has no problem with this query, but any other user that could still count as a `report_admin_user?` would (both users in the `EvmGroup-administrator` role and just having the proper features enabled) Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1650531
This was referenced Dec 5, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A flaw was found in the CloudForms API. A user with permissions to use the MiqReportResults capability within the API could potentially view data from other tenants or groups to which they should not have access.
Further, the attacker was able to schedule MiqReport run, thus they are in control (to extent) of what leaks.
https://access.redhat.com/security/cve/CVE-2016-7047