Fix role escalation prevention by hsato03 · Pull Request #7853 · apache/cloudstack

hsato03 · 2023-08-10T19:33:34Z

Description

While checking an account access to prevent the role escalation, the checkAccess method is using both DynamicRoleBasedAPIAccessChecker and StaticRoleBasedAPIAccessChecker (legacy) classes and that is causing some inconsistences in the validations, consequently allowing the role escalation.

For this reason, the StaticRoleBasedAPIAccessChecker was disabled if DynamicRoleBasedAPIAccessChecker is enabled.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

I have created a domain admin custom role (DA2) and allowed all API's to it using the * regex;
Removed an API that it had in common with default domain admin role;
Tried to create a default domain admin account using a DA2 account;
Received an exception from the role eslacation prevention.

yadvr · 2023-08-11T07:26:11Z

Hi @hsato03 thanks for the PR. The dynamic checker is used by default and we don't ship commands.properties with cloudstack for several years now, how can we test/verify your claims?

hsato03 · 2023-08-11T17:59:38Z

Hello @rohityadavcloud. You can test it by doing the same tests I did. Before the changes, the account will be created and no exception will be thrown.

Create a domain admin custom role (DA2) and allow all APIs to it using the * regex;
Still in DA2, remove an API that it has in common with the default domain admin role (listProjects for an example);
Login into a DA2 account and try to create an account that has the default domain admin role;
The account should not be created as the caller role (DA2) does not have a permission (listProjects) that the role's account being created has (default domain admin).

kiranchavala · 2023-08-14T04:57:02Z

@blueorangutan package

codecov · 2023-08-14T07:44:48Z

Codecov Report

Merging #7853 (a4d0f20) into 4.18 (b4032d9) will increase coverage by 0.00%.
Report is 23 commits behind head on 4.18.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               4.18    #7853   +/-   ##
=========================================
  Coverage     13.02%   13.02%           
- Complexity     9029     9039   +10     
=========================================
  Files          2720     2720           
  Lines        257010   257096   +86     
  Branches      40083    40093   +10     
=========================================
+ Hits          33467    33489   +22     
- Misses       219342   219403   +61     
- Partials       4201     4204    +3

Files Changed	Coverage Δ
...loudstack/acl/StaticRoleBasedAPIAccessChecker.java	`0.00% <0.00%> (ø)`

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaanHoogland · 2023-08-14T11:38:41Z

+        if (isEnabled()) {
+            return true;
+        }


if this prevents the escalation talked about fine, but it seems illogical to return true to prevent access. would return false not make more sense?

@DaanHoogland I don't think so. Actually this isEnabled method checks if DynamicRoleBasedAPIAccessChecker is enabled. Returning true here is just passing the check responsibility to another class.

yes @hsato03 I think you are right. I consider it a bug that the StaticRoleBasedAPIAccessChecker.isEnabled() would return true if it is disabled (but the dynamic one is enabled). Maybe this is a discussion out of scope for your change. As said, "if this prevents the escalation talked about (it is) fine"

yes, it looks like a bug @DaanHoogland
we should change the return value of isEnabled in this class

cc @rohityadavcloud ^

weizhouapache

code lgtm

It should be better to change the return value of method isEnabled, but it might have large impact.

weizhouapache · 2023-08-15T11:54:16Z

@blueorangutan package

blueorangutan · 2023-08-15T11:56:03Z

@weizhouapache a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-08-15T12:53:06Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6785

weizhouapache · 2023-08-16T06:57:47Z

@blueorangutan test

blueorangutan · 2023-08-16T06:58:03Z

@weizhouapache a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

weizhouapache · 2023-08-16T18:51:20Z

@blueorangutan package

blueorangutan · 2023-08-16T18:52:03Z

@weizhouapache a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-08-16T19:45:02Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6810

weizhouapache · 2023-08-16T20:09:05Z

@blueorangutan test

blueorangutan · 2023-08-16T20:10:03Z

@weizhouapache a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-08-16T20:41:21Z

[SF] Trillian test result (tid-7434)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47901 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7853-t7434-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_list_vms_metrics_admin	`Error`	3608.79	test_metrics_api.py
test_list_vms_metrics_history	`Error`	5.39	test_metrics_api.py
test_list_volumes_metrics_history	`Error`	7.45	test_metrics_api.py

blueorangutan · 2023-08-17T07:29:37Z

[SF] Trillian test result (tid-7451)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39277 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7853-t7451-kvm-centos7.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

weizhouapache · 2023-08-17T09:22:37Z

@DaanHoogland @rohityadavcloud
@hsato03 has changed the return value of isEnabled method in the class, the code lgtm and trillian test results look good. do you have any other concerns ?

DaanHoogland

code looks good

boring-cyborg · 2023-08-18T07:33:08Z

Awesome work, congrats on your first merged pull request!

fix role escalation prevention

9d395b0

yadvr requested review from DaanHoogland and weizhouapache August 14, 2023 10:56

yadvr added the status:needs-testing label Aug 14, 2023

yadvr added this to the 4.18.1.0 milestone Aug 14, 2023

yadvr added the Severity:Critical Critical bug label Aug 14, 2023

DaanHoogland reviewed Aug 14, 2023

View reviewed changes

weizhouapache approved these changes Aug 15, 2023

View reviewed changes

change isEnabled return

a4d0f20

weizhouapache requested a review from yadvr August 17, 2023 11:26

DaanHoogland approved these changes Aug 18, 2023

View reviewed changes

DaanHoogland merged commit 5b33967 into apache:4.18 Aug 18, 2023

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

hsato03 commented Aug 10, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

yadvr commented Aug 11, 2023

Uh oh!

hsato03 commented Aug 11, 2023

Uh oh!

kiranchavala commented Aug 14, 2023

Uh oh!

codecov bot commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

hsato03 Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

weizhouapache Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

weizhouapache commented Aug 16, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

weizhouapache commented Aug 16, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

weizhouapache commented Aug 16, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

blueorangutan commented Aug 17, 2023

Uh oh!

weizhouapache commented Aug 17, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

boring-cyborg bot commented Aug 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Aug 14, 2023 •

edited

Loading