CVE-2016-7047: API leaks any MiqReportResult

[isimluk]

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

Depends on ManageIQ/manageiq#15472

[isimluk]

@miq-bot add_label bug, euwe/backported
@miq-bot assign @martinpovolny

[miq-bot]

Checked commits https://github.com/isimluk/manageiq-ui-classic/compare/35b1c51fb4c6555c81fdb1e3dd9b5ad6bf680641~...461b8e3e0f51649489b4e61d5e3766888d1c8540 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
15 files checked, 10 offenses detected

app/controllers/application_controller.rb

• ❗ - Line 187, Col 13 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.

app/controllers/chargeback_controller.rb

• ❗ - Line 343, Col 10 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 432, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/dashboard_controller.rb

• ❗ - Line 719, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/report_controller.rb

• ❗ - Line 431, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/report_controller/reports.rb

• ❗ - Line 25, Col 22 - Style/SpaceAroundOperators - Operator = should be surrounded by a single space.
• ❗ - Line 25, Col 7 - Style/ExtraSpacing - Unnecessary spacing detected.

app/controllers/report_controller/reports/editor.rb

• ❗ - Line 104, Col 16 - Style/MultilineTernaryOperator - Avoid multi-line ternary operators, use if or unless instead.
• ❗ - Line 109, Col 16 - Style/MultilineTernaryOperator - Avoid multi-line ternary operators, use if or unless instead.

app/controllers/report_controller/saved_reports.rb

• ❗ - Line 18, Col 10 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

[martinpovolny]

@h-kataria, @himdel, @mzazrivec : the travis failures seem to be caused by the missing backend. Can you, please, make sure that once the backend is merged we merge this asap? Thx!

[Fryguy]

Backend merged, rekicking travis.

[simaishi]

Fine backport details:

$ git log -1
commit e81f28720fa2e2d8bdb61c54e12948f87f5bccd7
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Jun 30 12:29:43 2017 -0400

    Merge pull request #1627 from isimluk/CVE-2016-7047-final
    
    CVE-2016-7047: API leaks any MiqReportResult
    (cherry picked from commit 844398f6aebc6128e4c1ca32f427638f338dc929)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1450493

[himdel]

@isimluk Turns out this is wrong :(..

Looks like MiqReportResult.for_user only matches reports where miq_group_id IS NOT NULL.

But turns out some report results have miq_group_id = NULL, but still are being returned by MiqWidget_instance.contents_for_user.

(Looks like
contents = contents_for_owner(user.current_group, user, timezone) returns nil but contents ||= contents_for_owner(user.current_group, nil, timezone) succeeds.)

This causes errors like you can see a dashboard with a chart on the dashboard, but can't fullscreen it or download the PDF.

[isimluk]

I am sorry, I have missed occurrences when miq_group is null.

We need to investigate which code generates content and don't assign proper ownership. (as I suggested in https://bugzilla.redhat.com/show_bug.cgi?id=1471014#c3) We cannot revert or blame security fix, we need to make sure that content is always generated with ownership.

I just found there is CVE regression already in this regard in #1827

[himdel]

OK, so.. I have 120 records with NULL miq_group_id, and 35 records with non-null values.
In code related to reports and report results, I can see very little mention of miq_group_id, in fact, it seems that miq_group_id is assigned only in cases where userid doesn't have a | (in before_save).

In all the other cases, miq_group_id is NULL, and all the relevant info is stored in userid as
widget_id_10000000000088|Cloud-Operators|schedule, and miq_group_id is not generated.

Can you give me some info on how for_user actually works? It appears to be defined by rails, but I can't see any docs and it looks at miq_group_id... I wonder if it checks userid at all, or really always needs miq_group_id (but why, why would a record meant for a specific user need a group id?).

Also it seems it was never used in manageiq before - that leads me to think that this is entirely the wrong security mechanism. Or rather, that you introduced a standard security mechanism... which we don't use and support :).

---

Theoretically we could change that before_save to...

--- a/app/models/miq_report_result.rb
+++ b/app/models/miq_report_result.rb
@@ -32,6 +32,9 @@ class MiqReportResult < ApplicationRecord
     if user_info.length == 1
       user = User.find_by_userid(user_info.first)
       self.miq_group_id ||= user.current_group_id unless user.nil?
+    else
+      group = MiqGroup.find(:description => user_info[1])
+      self.miq_group_id ||= group.id unless group.nil?
     end
   end

.. but I'm still confused how for_user only looks at groups.

[isimluk]

for_user is our own custom scope.

Previously, there was not access model for MiqReports and MiqReportResults. At all, there was no designed access model. The only thing we had was a RerportsTree that used to call something on ApplicationController and create tree for given user. This mechanism used miq_group_id to decide, whether the ReportResult shall be shown, that means ... even though there was no explicit access model, there was a part of UI that depended on miq_group_id being set.

IIRC, I have found some of the reports that had miq_group_id set to null and fixed them, however there are perhaps more and I am sorry.

OK, so.. I have 120 records with NULL miq_group_id, and 35 records with non-null values.

This statistic may not necessarily be useful, as the old code used to generate reports without miq_group_id. Some of them may get miq_group_id set if regenerated with the new code. What would be more useful is to find which exact use-cases get miq_group_id=NULL set today.

[himdel]

for_user is our own custom scope.

Aah, found it, thanks :)

Previously, there was not access model for MiqReports and MiqReportResults....

Ok, so.. what should be the right behaviour? When I generate a report, should it really be generated as visible to my current group?

It seems to me that if we can have different users in the same group see different VMs, we probably don't want them to be able to see these reports either - so a group-based filtering may be insufficient. (Especially if the group is EvmGroup-user :).)

What would be more useful is to find which exact use-cases get miq_group_id=NULL set today.

Uh, I'm having trouble finding use-cases where it is set to non-NULL :).

It seems like there's only 2 ways of creating a MiqReportResult with miq_group_id:

• rely on that before_save and provide a valid username in userid
• call MiqReport#build_create_results with an explicit miq_group_id

manageiq-ui-classic/app/controllers/application_controller/report_downloads.rb
# render_pdf
27:      rr = report.build_create_results(:userid => userid)

manageiq/app/models/miq_report/generator/async.rb
# async_generate_table
103:      task.miq_report_result = build_create_results(options.merge(:userid => userid), taskid)

manageiq/app/models/miq_widget.rb
# generate_report_result 
317:    rpt.build_create_results(:userid => userid_for_result, :report_source => "Generated for widget", :timezone => timezone, :miq_group_id => group.id)

It seems render_pdf never sets it, generate_report_result always does, and as for async_generate_table (only used from ui-classic):

app/controllers/application_controller/timelines.rb
# tl_gen_timeline_data - no miq_group_id
155:        initiate_wait_for_task(:task_id => @report.async_generate_table(:userid => session[:userid]))

app/controllers/application_controller/performance.rb
# perf_gen_data_before_wait - no miq_group_id
694:    initiate_wait_for_task(:task_id => MiqReport.async_generate_tables(:reports => rpts, :userid => session[:userid]))
# perf_gen_tag_data_before_wait - no miq_group_id
778:    initiate_wait_for_task(:task_id => rpt.async_generate_table(
# perf_gen_top_data_before_wait - no miq_group_id
923:      initiate_wait_for_task(:task_id => rpts.first.async_generate_table(
928:      initiate_wait_for_task(:task_id => MiqReport.async_generate_tables(:reports => rpts, :userid => session[:userid]))
# perf_util_daily_gen_data - no miq_group_id
1017:        initiate_wait_for_task(:task_id => rpt.async_generate_table(
# perf_planning_gen_data - no miq_group_id
1176:      initiate_wait_for_task(:task_id => rpt.async_generate_table(

app/controllers/mixins/chargeback_preview.rb
# vm_chargeback - no miq_group_id
28:      initiate_wait_for_task(:task_id => rpt.async_generate_table(:userid => session[:userid]))

app/controllers/dashboard_controller.rb
# tl_gen_timeline_data - no miq_group_id
791:      initiate_wait_for_task(:task_id => @report.async_generate_table(:userid => session[:userid]))

app/controllers/report_controller/reports.rb
# show_preview - no miq_group_id
40:      initiate_wait_for_task(:task_id => @rpt.async_generate_table(

So.. never :).

EDIT: ah, but some of those provide the single-value userid .. doing a table ..
.. but async_generate_table will always overwrite that userid unless the mode is set to something other than adhoc.

[himdel]

• render_pdf - we can probably rely on session[:userid] being a simple userid - should be OK
• generate_report_result - should be OK
• tl_gen_timeline_data - no mode, no miq_group_id - definitely broken
• perf_gen_data_before_wait - no mode, no miq_group_id - definitely broken (*)
• perf_gen_tag_data_before_wait - mode "charts", session[:userid] - should be OK
• perf_gen_top_data_before_wait - mode "charts", session[:userid] only when length == 1 - definitely broken (when dealing with multiple reports) (*)
• perf_util_daily_gen_data - mode "charts", session[:userid] - should be OK
• perf_planning_gen_data - mode "charts", session[:userid] - should be OK
• vm_chargeback - no mode, no miq_group_id - definitely broken
• tl_gen_timeline_data - no mode, no miq_group_id - definitely broken
• show_preview - mode "adhoc", no miq_group_id - definitely broken

(*) with perf_gen_data_before_wait and the plural mode of perf_gen_top_data_before_wait, async_generate_tables is used instead of async_generate_table .. and that one looks like it doesn't actually create a MiqReportResult, so maybe those two are fine.

[himdel]

Also a few places which create a MiqReportResult but don't go via any of the above..

• MiqReport#async_generate_result - called by render_report_data:

manageiq-ui-classic/app/controllers/application_controller/report_downloads.rb
55:        task_id = rr.async_generate_result(@sb[:render_type], :userid     => session[:userid],

(=> probably OK because of that session[:userid])

• MiqReport#queue_generate_table - called from many places:

manageiq/tools/generate_report.rb
28:report.queue_generate_table(:userid => USER_ID)
# hardcoded to "admin" - OK

manageiq/app/models/service.rb
# generate_chargeback_report
379:    report.queue_generate_table(options)
# ends up getting called from schedule_chargeback_report_for_service_daily, without args - broken

manageiq-api/app/controllers/api/reports_controller.rb
# run_resource
20:      report_result = MiqReportResult.find(report.queue_generate_table(:userid => User.current_user.userid))
# uses a sane userid - OK

manageiq-ui-classic/app/controllers/report_controller/reports.rb
# render_pdf
11:    rep.queue_generate_table(:userid => session[:userid])
# session[:userid] - probably OK

... so, probably only generate_chargeback_report is broken out of these.