Fixes #11579 - CVE-2015-5233 - Reports show/destroy not restricted by host authorization #2644
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
|
The duplication on these API controllers & tests is way too much in my opinion. If you're 🆗 with this patch, we should handle this sooner than later. |
| @@ -11,22 +11,25 @@ def index | |||
| def show | |||
| # are we searching for the last report? | |||
| if params[:id] == "last" | |||
| conditions = { :host_id => Host.find(params[:host_id]).try(:id) } unless params[:host_id].blank? | |||
| conditions = { :host_id => Host.authorized.find(params[:host_id]).try(:id) } unless params[:host_id].blank? | |||
There was a problem hiding this comment.
Use .authorized(:view_hosts) rather than plain .authorized, otherwise I think any host permission would be sufficient.
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
| private | ||
|
|
||
| def check_hosts_permission | ||
| @resource_base = resource_base.my_reports |
There was a problem hiding this comment.
resource_base is a method, shouldn't this override it rather than set an instance variable? (And it should be in all controllers.)
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@domcleal Updated by overriding the resource_scope/resource_base methods instead of chaining a new filter, thanks for the review! |
|
|
||
| test 'cannot view the last report' do | ||
| report = FactoryGirl.create(:report) | ||
| get :show, { :id => 'last', :host_id => report.host.id }, set_session_user.merge(:user => User.current) |
There was a problem hiding this comment.
:id => 'last' is only applicable to the UI controller, the API controller has a last action instead. I'm not sure sharing a test for both types of controllers is going to allow easily for these differences?
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
… host authorization ReportsController 'show' and 'destroy' now perform a check to see if the User is authorized to see the Host associated with the Report. In case it's not, it returns 404, as to not give hints whether a Report ID or Host ID are valid. I followed the same approach on the API controllers. 'last' was not vulnerable due to using my_reports which performs the necessary check on 'view_hosts' permission.
shlomizadok
added a commit
to shlomizadok/foreman
that referenced
this pull request
Sep 2, 2015
…ot restricted by host authorization
shlomizadok
added a commit
to shlomizadok/foreman
that referenced
this pull request
Sep 2, 2015
…ot restricted by host authorization
shlomizadok
added a commit
to shlomizadok/foreman
that referenced
this pull request
Sep 2, 2015
…ot restricted by host authorization
shlomizadok
added a commit
to shlomizadok/foreman
that referenced
this pull request
Sep 3, 2015
…ot restricted by host authorization
shlomizadok
added a commit
to shlomizadok/foreman
that referenced
this pull request
Sep 22, 2015
…ot restricted by host authorization
shlomizadok
added a commit
to shlomizadok/foreman
that referenced
this pull request
Sep 22, 2015
…ot restricted by host authorization
ares
added a commit
to ares/foreman
that referenced
this pull request
Sep 22, 2015
…o review/2750 * 'fix_4151' of https://github.com/shlomizadok/foreman: refs #10782 - Apply global host status changes refs #11579 - Addition of PR theforeman#2644 - Reports show/destroy not restricted by host authorization fixes theforeman#4151 - enable reports STI
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.
ReportsController 'show' and 'destroy' now perform a check to see if
the User is authorized to see the Host associated with the Report. In
case it's not, it returns 404, as to not give hints whether a Report
ID or Host ID are valid.
I followed the same approach on the API controllers. 'last' was not
vulnerable due to using my_reports which performs the necessary
check on 'view_hosts' permission.