User can list other tenant's and admin's export locations

Recommended workaround for this bug: don't share the UUIDs of shares with other tenants.

Risk for this bug is fairly low because UUIDs are impossible to guess and a successful security breach would require obtaining the UUIDs using another security exploit. Also, this bug leaks sensitive information but doesn't allow actual access to the data unless a separate exploit is used to bypass share access control.

This issue is not triggered by the python-manilaclient since it does an explict get on the relevant share before using the API to list its export locations. The get on the share itself fails a policy check if the share belongs to another tenant and is not public.

I have confirmed however that if one uses the UUID of a non-public share belonging to another tenant and invokes the API directlyn (e.g. via curl command) to list the export locations for the share or to show the contents of a particular export for the share this succeeds.

We'll need to adjust corresponding tempest tests as well given Rodrigo's observations above, but first I want to get consensus on the appropriate fix for the functional code.

Candidate fix is attached.

I'm attaching tempest log of a run of the existing admin export locations test suite with this functional change (tempest.log1), where the two tests that Rodrigo cited now fail, as they should. Also a log (tempest.log2), where these two tests have been moved to negative tests and modified to assert a policy exception because a tenant is trying to get export location info for a non-public share that it doesn't own.

?field.comment=I'm attaching tempest log of a run of the existing admin export locations test suite with this functional change (tempest.log1), where the two tests that Rodrigo cited now fail, as they should. Also a log (tempest.log2), where these two tests have been moved to negative tests and modified to assert a policy exception because a tenant is trying to get export location info for a non-public share that it doesn't own.

ok, here's the tempest.log2

Sorry, tempest.log2 didn't include the new negative tests, which tempest.log3 does ...

Here's the patch with the tempest changes.

After several pair-wise discussions with reviewers it is clear that we don't yet have convergence of opinion on what tempest tests are needed here, so I am following Valeriy's suggesion and posting a minimal patch set: proposed functional change plus a skip of the tempest tests that will (properly) fail with the functional change. The skip decorator cites a new bug that says we need to add proper tests. So the technical debt to do these tests will be in our backlog but we can address it and drive consensus more productively in a gerrit review context after we take care of this CVE downstream.

valeriy just pointed out that the latest patch is broken b/c I'm using an unimported skip decorator module from tempest.

Attaching yet another patch, this time with skip decorator that should work. Patch is named: Do-policy-check-when-getting-export-locations.patch

As agreed in today's meeting with ganso, vponmaryov, bswartz, and myself the last patch will be extended to do a similar policy check when showing or listing export locations for share intances. With default policy there would be no issue with share instances since with that policy only admin can view share intances anyways. However, a check would need to come into play if a cloud admin made policy changes to allow normal users to see share instances: one should in that case be able to see export locations for instances where one has rights to view the share and its instances, but not for instances of non-public shares belonging to others.

Attaching new patch set for consideration, called 'Do-policy-check-when-getting-export-locations-for-shares-and-share-instances.patch'.

+1

I tested the latest patch manually, both share_export_locations and share_instance_export_locations. I also tested tempest, it skips both tests, and I couldn't find any tempest test affected by the share_instance_export_locations fix. So I am giving this my seal of approval.

+1

While openstack/manila is not a "vulnerability:managed" deliverable, if it were the OpenStack VMT would switch this bug to Public (not Public Security) and mark any security advisory task "won't fix" because per comment #1 exploiting this requires guessing or having some other means of obtaining random UUIDs for other tenants' resources. This sort of bug falls under report class C1 in our taxonomy: "Not considered a practical vulnerability (but some people might assign a CVE for it), e.g. one depending on UUID guessing" https://security.openstack.org/vmt-process.html#incident-report-taxonomy

Adding the "security" bug tag after making the bug Public will cause the security mailing list to get notified too, in case the Security Team want to draft a future note to document this risk.

Hi,

Completely cognizant of the fact that my update will re-trigger communication on this three year old bug report. I was recently added to this core-sec group, and Jeremy pointed out that this bug needs some resolution.

I agree with previous findings by Ben Swartzlander, Tom Barron and Rodrigo Barbieri that it is going to be really hard to exploit this vulnerability and do harm. If an attacker is able to grab export locations via UUID, they'd still need to gain access for their specific clients (by IP/user/cert/cephx rules) to be able to mount the share and do any real damage.

Tom: I'm inclined to switch this bug report to public - are you able to refresh your patches and submit them?

@goutham, sounds like a good plan.

Reviewed: https://review.opendev.org/714181
Committed: https://git.openstack.org/cgit/openstack/manila-tempest-plugin/commit/?id=c678e21b5e0d0aec2d014a40ceadcd573bac08f9
Submitter: Zuul
Branch: master

commit c678e21b5e0d0aec2d014a40ceadcd573bac08f9
Author: Goutham Pacha Ravi <email address hidden>
Date: Fri Mar 20 11:13:47 2020 -0700

    Fix export locations tests

    The tempest tests
    - api.admin.test_export_locations.ExportLocationsTest#test_list_share_export_locations_by_member
    - api.admin.test_export_locations.ExportLocationsTest#test_get_share_export_location_by_member

    were both written with the assumption that a user within a
    project creates a share, and the share's export locations
    are available to other users in the project. In this specific
    context, the user creating the share has an "admin" role -
    but that is just circumstantial. Any user with the ability
    to create shares in a project can do so, and expect that
    those shares are accessible to other users by virtue of default
    policy.

    However, tempest test projects each have only one user,
    and the private share in both these test cases is created
    within the original user's project, and is not supposed to
    be accessible across projects. This behavior is called out in
    LP bug #1654598.

    So, enhance the test infra to create a user within the same
    project applying roles specified within tempest.conf and test
    accessibility with such a user.

    Once bug #1654598 has been resolved, we can use the existing
    test behavior of testing access via a different project as
    negative test cases (See: https://review.opendev.org/710661/)

    Related-Bug: #1654598
    Partial-Bug: #1655427
    Change-Id: I12a1df36e8d928c54c03ed644dd60557f349ddb3
    Signed-off-by: Goutham Pacha Ravi <email address hidden>

Reviewed: https://review.opendev.org/710639
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=84daeb481d852d6531df11e842df1a70672d938c
Submitter: Zuul
Branch: master

commit 84daeb481d852d6531df11e842df1a70672d938c
Author: Tom Barron <email address hidden>
Date: Sun Mar 1 13:12:08 2020 +0100

    Enforce policy checks for share export locations

    Closes-bug: #1654598
    Change-Id: I5f358266739f1c42343d5a0c5ec8109c8fcaac4d

Fix proposed to branch: stable/train
Review: https://review.opendev.org/714674

Fix proposed to branch: stable/stein
Review: https://review.opendev.org/714734

Reviewed: https://review.opendev.org/714674
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=02fd716bf83849873f0bccb78b851196e6acf2b7
Submitter: Zuul
Branch: stable/train

commit 02fd716bf83849873f0bccb78b851196e6acf2b7
Author: Tom Barron <email address hidden>
Date: Sun Mar 1 13:12:08 2020 +0100

    Enforce policy checks for share export locations

    Closes-bug: #1654598
    Change-Id: I5f358266739f1c42343d5a0c5ec8109c8fcaac4d
    (cherry picked from commit 84daeb481d852d6531df11e842df1a70672d938c)

Reviewed: https://review.opendev.org/714734
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=aa5e1f65cd61c57178fb864ac16bcb5c9eefb7c8
Submitter: Zuul
Branch: stable/stein

commit aa5e1f65cd61c57178fb864ac16bcb5c9eefb7c8
Author: Tom Barron <email address hidden>
Date: Sun Mar 1 13:12:08 2020 +0100

    Enforce policy checks for share export locations

    Closes-bug: #1654598
    Change-Id: I5f358266739f1c42343d5a0c5ec8109c8fcaac4d
    (cherry picked from commit 84daeb481d852d6531df11e842df1a70672d938c)
    (cherry picked from commit 02fd716bf83849873f0bccb78b851196e6acf2b7)

Fix proposed to branch: stable/rocky
Review: https://review.opendev.org/714993

This issue was fixed in the openstack/manila 9.1.1 release.

This issue was fixed in the openstack/manila 8.1.1 release.

Reviewed: https://review.opendev.org/714993
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=875cb87328665629778dd20951cc28e1e66d3190
Submitter: Zuul
Branch: stable/rocky

commit 875cb87328665629778dd20951cc28e1e66d3190
Author: Tom Barron <email address hidden>
Date: Sun Mar 1 13:12:08 2020 +0100

    Enforce policy checks for share export locations

    Closes-bug: #1654598
    Change-Id: I5f358266739f1c42343d5a0c5ec8109c8fcaac4d
    (cherry picked from commit 84daeb481d852d6531df11e842df1a70672d938c)
    (cherry picked from commit 02fd716bf83849873f0bccb78b851196e6acf2b7)
    (cherry picked from commit aa5e1f65cd61c57178fb864ac16bcb5c9eefb7c8)

Fix proposed to branch: stable/pike
Review: https://review.opendev.org/715666

Fix proposed to branch: stable/queens
Review: https://review.opendev.org/715687

Reviewed: https://review.opendev.org/715687
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=d2c72aff8a38502d97e74e839866627c30ed7b31
Submitter: Zuul
Branch: stable/queens

commit d2c72aff8a38502d97e74e839866627c30ed7b31
Author: Tom Barron <email address hidden>
Date: Sun Mar 1 13:12:08 2020 +0100

    Enforce policy checks for share export locations

    Closes-bug: #1654598
    Change-Id: I5f358266739f1c42343d5a0c5ec8109c8fcaac4d
    (cherry picked from commit 84daeb481d852d6531df11e842df1a70672d938c)
    (cherry picked from commit 02fd716bf83849873f0bccb78b851196e6acf2b7)
    (cherry picked from commit aa5e1f65cd61c57178fb864ac16bcb5c9eefb7c8)
    (cherry picked from commit 875cb87328665629778dd20951cc28e1e66d3190)

Reviewed: https://review.opendev.org/710661
Committed: https://git.openstack.org/cgit/openstack/manila-tempest-plugin/commit/?id=b5ed5dfaa5ed4989bdefc30abb00902a46052951
Submitter: Zuul
Branch: master

commit b5ed5dfaa5ed4989bdefc30abb00902a46052951
Author: Tom Barron <email address hidden>
Date: Sun Mar 1 21:27:26 2020 +0100

    Fix export location negative tests

    When running as a regular user, attempts to get share export
    locations for a share belonging to another user should be
    forbidden.

    Share instance export locations are not available to regular
    users by virtue of default policy.

    Related-bug: #1654598
    Closes-bug: #1655427
    Change-Id: Iabe7fb68facd0ddffec738ab4e98d1de3a704ee4
    Signed-off-by: Goutham Pacha Ravi <email address hidden>

Change abandoned by Goutham Pacha Ravi (<email address hidden>) on branch: stable/pike
Review: https://review.opendev.org/715666

This issue was fixed in the openstack/manila queens-eol release.

This issue was fixed in the openstack/manila rocky-eol release.