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

[Rodrigo Barbieri]

Currently, the share export locations API is allowing any tenant to obtain export locations of any tenant's share.

See the below URL:

http://172.24.47.101:8786/v2/64350ec996cb4d91bfaa728fd7199313/shares/e93eb079-58fb-4758-9d95-a9a645b0250a/export_locations

64350ec996cb4d91bfaa728fd7199313: this is a non-admin tenant ID

e93eb079-58fb-4758-9d95-a9a645b0250a: this is an admin's share ID

This is because the API layer of the share export locations controller is going directly to the database to obtain the export locations of the supplied share ID.

The ownership check is performed at the Share/API layer, which is not invoked in this workflow.

Most surprisingly of all, the tempest tests:

- test_export_locations.ExportLocationsTest.test_list_share_export_locations_by_member
- test_export_locations.ExportLocationsTest.test_get_share_export_location_by_member

... should not be passing at all (and should be negative tests), as they are testing if a non-admin tenant is able to obtain and list export locations of a share created by the admin_client used by tempest.

Affected releases:
- Liberty
- Mitaka
- Newton
- Ocata

[Ben Swartzlander (bswartz)]

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.

[Tom Barron (tpb)]

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.

[Tom Barron (tpb)]

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.

[Tom Barron (tpb)]

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.

[Tom Barron (tpb)]

ok, here's the tempest.log2

[Tom Barron (tpb)]

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

[Tom Barron (tpb)]

Here's the patch with the tempest changes.

[Tom Barron (tpb)]

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.

[Tom Barron (tpb)]

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

[Tom Barron (tpb)]

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

[Tom Barron (tpb)]

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'.

[Ben Swartzlander (bswartz)]

+1

[Rodrigo Barbieri (rodrigo-barbieri2010)]

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.

[Valeriy Ponomaryov (vponomaryov)]

+1

[Jeremy Stanley (fungi)]

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.

[Goutham Pacha Ravi (gouthamr)]

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?

[Tom Barron (tpb)]

@goutham, sounds like a good plan.

[OpenStack Infra (hudson-openstack)]

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>

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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>

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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