Skip to content

Implement UnnecessarilyGrantedPrivilegedAccessRights #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 48 commits into from
Sep 5, 2024

Conversation

jeongsoolee09
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 commented Aug 14, 2024

What this PR contributes

This PR aims to:

  1. Add UnnecessarilyGrantedPrivilegedAccessRights.ql (previously DynamicallyGeneratedPrivileged.ql).
  2. Fix wrong assumptions on cds.requires in test cases: previously cds.requires had service-1 and service-2.

Adding UnnecessarilyGrantedPrivilegedAccessRights

This query was previously named DynamicallyGeneratedPrivileged but renamed to UnnecessarilyGrantedPrivilegedAccessRights to better illustrate its core concerns: the fact that a privileged user is dynamically generated is itself not a problem, but that it's used to grant elevated access rights is.

UnnecessarilyGrantedPrivilegedAccessRights aims to find transactions that accesses an entity which requires nontrivial access rights (i.e. not trivial ones such as @(restrict: [{grant: 'READ'}])) in the context of cds.User.Privileged. This is a powerful role that bypasses all authorization checks that may be abused in the real world; a programmer may wish to access local entities without restrictions since they can already be accessed by other services in the same application. However, from the perspective of authorization this is an unnecessarily elevated access right and therefore a mistake in access control.

Fixing wrong assumptions

cds.requires in package.json of each test case in bad-authn-authz had service-1 and service-2 that pointed to the implementation details:

{
  "cds": {
    "requires": {
      "service-1": {
        "impl": "srv/service1.js"
      },
      "service-2": {
        "impl": "srv/service2.js"
      }
    }
  }
}

and the name "service-1" and "service-2" were used as handles to connect to the associated service:

const Service2 = await cds.connect.to("service-2");

However, some back-and-forth with a CAP expert from SAP revealed that this is not only necessary but wrong in some cases. Therefore, this PR remove such cds.requires section and changes the argument "service-2" to "Service2", to directly reference the service without an intermediary handle.

Others

  • Refactor CDS.qll to make ServiceInstance a subclass of DataFlow::SourceNode, not DataFlow::Node, and use it to simplify all cases of ServiceInstance as well as its key member predicate getASrvMethodCall. Move this member predicate to the abstract class level.
  • Clean up CDS.qll by moving type trackers into their own module.
  • Add class EntityReference that models references to entities which are either variables containing a value of a property of srv.entities, or string/template literals which resolve to the entity: Service1.Service1Entity1.

@jeongsoolee09 jeongsoolee09 changed the title TBD Implement UnnecessarilyGrantedPrivilegedAccessRights Aug 28, 2024
@jeongsoolee09 jeongsoolee09 requested a review from mbaluda August 28, 2024 19:37
@jeongsoolee09 jeongsoolee09 marked this pull request as ready for review September 3, 2024 22:36
Copy link
Contributor

@mbaluda mbaluda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first review and left a few comments

return this.tx(
{ user: new cds.User.Privileged("privileged-user-4") },
(tx) =>
tx.run(
SELECT.from(Service2Entity1) // Declared in service2.cds
SELECT.from(Service2Entity2) // Declared in service2.cds

Check warning

Code scanning / CodeQL

Access rights to an entity is unnecessarily elevated to privileged Medium test

This entity is accessed with unnecessarily privileged rights that requires authorization.
{ user: new cds.User.Privileged("privileged-user-5") },
(tx) =>
tx.run(
SELECT.from`RemoteEntity` // Assume that it's declared in @example/sample
SELECT.from`RemoteEntity` // Assume that it's declared in @advanced-security/remote-service

Check warning

Code scanning / CodeQL

Access rights to an entity is unnecessarily elevated to privileged Medium test

This entity is accessed with unnecessarily privileged rights that may require authorization.
tx.run(
INSERT.into("Service2Entity").entries({
INSERT.into("Service2Entity2").entries({

Check warning

Code scanning / CodeQL

Access rights to an entity is unnecessarily elevated to privileged Medium test

This entity is accessed with unnecessarily privileged rights that requires authorization.
tx.run(
INSERT.into("Service2Entity").entries({
INSERT.into("Service2Entity2").entries({

Check warning

Code scanning / CodeQL

Access rights to an entity is unnecessarily elevated to privileged Medium test

This entity is accessed with unnecessarily privileged rights that requires authorization.
Copy link
Contributor

@mbaluda mbaluda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it! 🚀

@mbaluda mbaluda merged commit 67b71a3 into main Sep 5, 2024
5 checks passed
@mbaluda mbaluda deleted the jeongsoolee09/fix-package-json-assumptions branch September 5, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants