Fixed issue with downloading report

[yrudman]

Issue: Connecting as admin user and executing download will set userid column in miq_report_result table to 'admin|2d7db0c5addb0eb91de3541c811ea02e|download'. As result record is not found for current_user.

https://bugzilla.redhat.com/show_bug.cgi?id=1471014

Solution: Do not check if curent_user can access data for downloading. We can remove this check since download button available only when specific report selected and visibility restriction was already applied to list of reports.

BEFORE:

AFTER:

@miq-bot add-label bug, fine/yes, euwe/yes

\cc @gtanzillo

[miq-bot]

Checked commit yrudman@9e3e8bc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[yrudman]

\cc @h-kataria @dclarizio

[h-kataria]

looks good.

[simaishi]

Fine backport details:

$ git log -1
commit cd6dce3934f327a305f8313280f1e6ff9c3e7afe
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Aug 4 09:51:45 2017 -0400

    Merge pull request #1827 from yrudman/fix-downloading-reports
    
    Fixed issue with downloading report
    (cherry picked from commit 282755ef8662022271b5ca55446a81ca4220a4d5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1479994

[simaishi]

Euwe backport (to manageiq repo) details:

$ git log -1
commit 024c1b49e851ac947cc1bf6c864a952c74504d6c
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Aug 4 09:51:45 2017 -0400

    Merge pull request #1827 from yrudman/fix-downloading-reports
    
    Fixed issue with downloading report
    (cherry picked from commit 282755ef8662022271b5ca55446a81ca4220a4d5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1481743

[ZitaNemeckova]

@yrudman @h-kataria Same problem in Cloud Intel -> Dashboard -> FullScreen/Download PDF.

On lines https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/application_controller.rb#L574 and https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/application_controller/report_downloads.rb#L54 .

Not sure if it should be solved in the same way... because it was introduced by #1627 .

[martinpovolny]

Solution: Do not check if curent_user can access data for downloading. We can remove this check since download button available only when specific report selected and visibility restriction was already applied to list of reports.

Sorry, but this is wrong. It's very easy to get around such check. You cannot trust the browser/user/the Internet that an action will not be executed because the button is not there. This creates a security issue.

This issue needs to be fixed differently.

[martinpovolny]

@simaishi : this should not get released

[himdel]

See discussion from #1627 (comment) on.

TLDR: we can probably find all the places which don't set miq_group_id, but I have no idea what value of miq_group_id would actually make any sense.

[yrudman]

@simaishi
Downloading report done in 3 phases:

1. Request for download - it could be initiated ONLY by user who is in compliance with CVE-2016-7047 patch here:

   manageiq-ui-classic/app/controllers/application_controller/report_downloads.rb

   Line 54 in 56483b5

   rr = MiqReportResult.for_user(current_user).find(session[:report_result_id]) # Get report task id from the session

   . Result of request for download is temporary record in MiqReportTabe
2. Download actual report
3. Delete temporary record

Security check removed from temporary record.

I agree, solution is not perfect (since something could go wrong in step 2 and temp record could become permanent) but not as bad as looks like, or I am missing something ?

[martinpovolny]

Ok, cool @yrudman. I have read the explanation in the PR description and that sounded much worse than it probably is.

I'll leave this to you guys @yrudman @himdel @isimluk

[himdel]

I think this particular fix is really not a security regression per se.

The only problem with this fix is that it doesn't fix all the places, and fixing them the same way would be a security regression. So.. this is OK, but dangerous if anybody copies it.

So... maybe let's focus on fixing it properly instead, so we can keep all the fixes the same?

@yrudman What do you know about what report results should be actually visible to which user?

Do you have any answer to my question here? - Should the report results really be visible to any member of the current group of the user who generated it?

[yrudman]

right, this fix can not be blindly copied.
Common stuf, i think, we can remove security check where report id taken from session.

It looks like that different workflows related to reports should be treated differently.
For example issue with widgets: Widgets can be available by role and group, MiqReportResult.userid will have pipe separated list of groups which have access to this record and having single MiqReportResult.miq_group_id will not work;

EDIT: it looks like different record created for different group, look at record starting with widget_id_6

[himdel]

Ah, there can be multiple groups in there? Any idea where that would come from?

All I can see is that userid = "#{options[:userid]}|#{options[:session_id]}|#{options[:mode]}" which ...would fit the format, but not sure if this always comes from there, especially since those groups in the middle don't look like session_id to me :).

But .. in the screenshot, only the middle field is a group, so so far I'm not seeing any evidence of multiple groups being supported.

[himdel]

Widgets can be available by role and group

Ah, so this mechanism is definitely insuficcient and will need at least a role support added, good to know :).

I wonder if it makes more sense to fix that for_user scope to check for these (but have to parse strings) or to add new db fields, WDYT?

EDIT: actually, silly question, definitely need to add role id field if that is something we need to support.

[yrudman]

Any idea where that would come from?

[himdel]

Yet I can see only one of those groups in that field..

So we're probably losing that data right now, right? (Or, keeping it in widget, but not in the report result.)

[yrudman]

we are not loosing it,
there is one record per group in miq_report_results: (I was wrong assuming multiple groups pipe separated), it used to create custom dashboards