LFI vulnerability in "Create Workbook"

[Anh Nguyen]

Hello,
I've found a Local File Inclusion (LFI) vulnerability in creating a workbook on OpenStack Dashboard.
This vulnerability allows the attacker to read a sensitive file on the server like /etc/password, config file, etc. Tested version: Victoria Horizon 18.6.3
I do not an opportunity to test the other version, but I think those versions also vulnerable.

Steps to reproduce:
1. Create a text file datnt78.txt with content: "/etc/passwd"
2. Select Workflow -> Workbooks -> Create Workbook
3. In "Definition Source" select "File" then browse datnt78.txt file then click Validate and got /etc/passwd content.

This is the request: http://paste.openstack.org/show/806520/
This is the response: http://paste.openstack.org/show/806521/
Please find the sample file and POC image in the attachment.

Thank you,
DatNT78 at FTEL CSOC

[Akihiro Motoki (amotoki)]

Horizon itself does not provide a page for "Workbook". As far as I know, mistral API defines "workbook" resources. Do you use mistral-dashboard? If so, please file this to the mistral launchpad instead of horizon launchpad.

[Akihiro Motoki (amotoki)]

I said "please file this to the mistral launchpad instead of horizon launchpad", but you can add "mistral" project to the affected projects in this bug if my guess is correct.

[Anh Nguyen (nopex)]

Thank you, just added "Mistral" to the affected project.

[Jeremy Stanley (fungi)]

I've added a "won't fix" security advisory task for tracking purposes just now, to indicate that the OpenStack Vulnerability Management Team doesn't expect to distribute a public advisory for this report since it does not oversee reports of suspected vulnerabilities for Mistral. However, we're happy to assist the Mistral team with any logistics and coordination if they wish.

[Akihiro Motoki (amotoki)]

I looked into this as my hat of horizon-core.

A content uploaded as "definition_upload" is accessed at [1]. read() method just retrieves the uploaded content, so the return value would be "/etc/password" in the test upload here. It will be passed to mistraldashboard.api.workbook_validate [2] and api.workbook_validate calls workbooks.validate() from mistralclient [3]. Looking at the response in the bug description, the call [3] fails and an error message was provided to a caller.
This looks like a bug in mistral (or mistralclient) rather than mistral-dashboard.

This is my observation from POV of dashboard implementation. Apparently horizon is not involved in it, so I am marking horizon status as Invalid.

Further action is up to the mistral team.

[1] https://opendev.org/openstack/mistral-dashboard/src/commit/fa785b2bb8314d0981d343bd51fe645017a38cca/mistraldashboard/workbooks/forms.py#L67
[2] https://opendev.org/openstack/mistral-dashboard/src/commit/fa785b2bb8314d0981d343bd51fe645017a38cca/mistraldashboard/workbooks/forms.py#L74-L76
[3] https://opendev.org/openstack/mistral-dashboard/src/commit/fa785b2bb8314d0981d343bd51fe645017a38cca/mistraldashboard/api.py#L262-L268

[Anh Nguyen (nopex)]

Hello, I am not sure 100% but I think a bug occurred in mistralclient [1]
So, I'll change the affected project from 'mistral' to 'mistralclient'

[1] https://github.com/openstack/python-mistralclient/blob/18fff747b500f99a81305b4c73c781f8059b0309/mistralclient/utils.py#L61-L82

[Jeremy Stanley (fungi)]

This bug was publicly disclosed in IRC on Tuesday, so there's no point in continuing to keep it private: https://meetings.opendev.org/irclogs/%23openstack-mistral/%23openstack-mistral.2021-06-29.log.html

[Jeremy Stanley (fungi)]

Following discussion with Adriano, I have switched this report to Public Security now.

[Adriano Petrich (apetrich)]

Added a patch for it https://review.opendev.org/c/openstack/python-mistralclient/+/799835

somehow the automation didn't catch and added a link here, sorry

[Jeremy Stanley (fungi)]

The bug updating automation checks https://opendev.org/openstack/project-config/src/branch/master/gerrit/projects.yaml in order to know where to find bugs for each project, and currently the entry for the openstack/python-mistralclient project says its bugs are grouped with the "mistral" project in Launchpad, so it was expecting bug 1931558 to be for mistral rather than python-mistralclient here. If you push a change to remove the groups entry for it, this would work the way you expect (it would default to looking for python-mistralclient bugs instead of mistral bugs when reporting openstack/python-mistralclient change activity).

[Akihiro Motoki (amotoki)]

I am not a responsibility for this issue as I am not a developer involved in mistral including mistral-dashboard and mistraclient, but I have a concern on the approach of the fix when looking at the proposed review.

The problem is that the code can expose the contents of files on a web server. It is not specific to files under /etc, /proc and so on.

When I commented in #6, I did not know the utility in mistralclient which loads the contents of a local file, but afterr looking at the proposed change in the mistralclient the root cause is that mistral-dashboard allows to use the feature. It looks like the feature was designed to use in a user local env and it is not expected to run on a web server.

[Anh Nguyen (nopex)]

Hi everyone,
Are there any updates about this?
Btw, I agreed with Akihiro's concern about the fix and I think this feature should only use in the local env.

Did we need to allow the user to access a local file on the server via the web app?
For example, we put the path file "/home/user_admin/filename" via Workbook and then the app will access and show it to the user (if the error occurred)?

In my opinion, at least, we need to check the error handling, if the file is an invalid workbook, just show that "Invalid DSL: '/etc/passwd' is not of type 'object'" instead of showing the file's content with the error as we did.

[Adriano Petrich (apetrich)]

I think we are all in agreement that the patch is not the best solution, but I don't see how to block this feature only for mistral-dashboard. mistralclient doesn't have a config file for it so disabling this feature based on configuration it not an option that can be created without a lot of coding. I don't see how to disable that only for mistral-dashboard but I'm unfamiliar with the codebase as I've never touched it. Maybe a mistral-dashboard developer might know.

[Akihiro Motoki (amotoki)]

Hi,

Another fix has been proposed from Takashi. It seems that you are not aware of it.
https://review.opendev.org/c/openstack/python-mistralclient/+/800950 (python-mistralclient) and https://review.opendev.org/c/openstack/mistral-dashboard/+/800952 (mistral-dashboard).

I believe this is a more complete fix. The root cause is a double loading of file contents and the mistralclient python binding does not have a way to distinguish if a filename is a content already loaded or a file to be loaded. The proposed pair of patches allows to prevent the double loading of file contents explicitly, so I think this is the right fix.

[Akihiro Motoki (amotoki)]

Some more detail.

The ideal solution is to break the "definition" argument of validate() [1] into two arguments "definition_contents" and "definition_filename". By doing so, we can avoid double loading of file contents.

On the other hand, this solution is not backward-compatible, so https://review.opendev.org/c/openstack/python-mistralclient/+/800950 proposes a workaround to allow us to pass "definition_contents" explicitly.
This workaround works at least from the prespective of the usage of mistral-dashboard.

[1] https://opendev.org/openstack/python-mistralclient/src/commit/609f4f48a9a740c56aeae1ce1ba1581a7f07838d/mistralclient/api/v2/workbooks.py#L98

[Adriano Petrich (apetrich)]

That seems like a better solution overall. I'm abandoning mine and +2ing it. Great job Takashi and Akihiro.

[Anh Nguyen (nopex)]

Thank you all for the information and the fix.
And could we have a CVE id for this issue?

[Jeremy Stanley (fungi)]

Anyone can obtain a CVE assignment directly from MITRE using their request form. The process recommended by the VMT is outlined here: https://security.openstack.org/vmt-process.html#send-cve-request

Since this report is not under any embargo, I wouldn't bother supplying an encryption key for their response.

[Akihiro Motoki (amotoki)]

The fix is still partial.

Mistral team, can you take care of the progress of the fix?
I am a horizon-coresec and horizon-stable-maint but have no power to move this forward.

The following is my understanding. Hope it helps you track the progress.

- master branch
-- A preparation added in python-mistralclient has landed.
-- A new release of python-mistralclient is required so that mistral-dashboard can consume it.
-- A fix in mistral-dashboard should land.
- stable branches
-- The change in both mistral-dashboard and python-mistralclient should be backported in all maintained branches
-- I don't know which branches are affected.

[Akihiro Motoki (amotoki)]

I added "mistral" to the affected project because mistral-dashboard issue is tracked by "mistral" launchpad and the fix is required in mistral-dashboard.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/python-mistralclient/+/803688

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/python-mistralclient/+/803814

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/mistral-dashboard/+/800952
Committed: https://opendev.org/openstack/mistral-dashboard/commit/8b876b0b22b365f24af1eb9eae01ad3d22cc1533
Submitter: "Zuul (22348)"
Branch: master

commit 8b876b0b22b365f24af1eb9eae01ad3d22cc1533
Author: Takashi Kajinami <email address hidden>
Date: Thu Jul 15 23:13:21 2021 +0900

    Enforce usage of raw definitions

    This change ensures that any definitions passed is treated as raw
    contents. With this change mistral-dashboard no longer tries to load
    contents based on file path or uri passed in by users, and this
    prohibits access to any local files or any internal contents accessible
    without authentication.

    Depends-on: https://review.opendev.org/800950
    Closes-Bug: #1931558
    Change-Id: I4de45cadc4e174794d0c2ef82223a9da5cbdcabc

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/yoga
Review: https://review.opendev.org/c/openstack/mistral-dashboard/+/852955

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/mistral-dashboard/+/852955
Committed: https://opendev.org/openstack/mistral-dashboard/commit/c077728bfa6001f0cb1ac22b0bacd74eb1967b04
Submitter: "Zuul (22348)"
Branch: stable/yoga

commit c077728bfa6001f0cb1ac22b0bacd74eb1967b04
Author: Takashi Kajinami <email address hidden>
Date: Thu Jul 15 23:13:21 2021 +0900

    Enforce usage of raw definitions

    This change ensures that any definitions passed is treated as raw
    contents. With this change mistral-dashboard no longer tries to load
    contents based on file path or uri passed in by users, and this
    prohibits access to any local files or any internal contents accessible
    without authentication.

    Depends-on: https://review.opendev.org/800950
    Closes-Bug: #1931558
    Change-Id: I4de45cadc4e174794d0c2ef82223a9da5cbdcabc
    (cherry picked from commit 8b876b0b22b365f24af1eb9eae01ad3d22cc1533)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/mistral-dashboard 15.0.0.0rc1 release candidate.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/mistral-dashboard 14.0.1 release.

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Elod Illes <email address hidden>" on branch: stable/victoria
Review: https://review.opendev.org/c/openstack/python-mistralclient/+/803814
Reason: stable/victoria branch of openstack/python-mistralclient is about to be deleted. To be able to do that, all open patches need to be abandoned. Please cherry pick the patch to unmaintained/victoria if you want to further work on this patch.

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Elod Illes <email address hidden>" on branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/python-mistralclient/+/803688
Reason: stable/wallaby branch of openstack/python-mistralclient is about to be deleted. To be able to do that, all open patches need to be abandoned. Please cherry pick the patch to unmaintained/wallaby if you want to further work on this patch.