advanced-security / codeql-sap-js

CodeQL models for SAP JavaScript frameworks CAP, UI5 and XSJS
MIT License
1 stars 0 forks source link

Insufficient authorization query #100

Open mbaluda opened 8 months ago

mbaluda commented 8 months ago

Relevant sources:

jeongsoolee09 commented 8 months ago

A standard library query: User-controlled bypass of security check. Related CWEs:

jeongsoolee09 commented 8 months ago

Authorization enforcement in Node.js:

service CustomerService @(requires: 'authenticated-user'){
  entity Orders @(restrict: [
    { grant: ['READ','WRITE'], to: 'admin' },
  ]){/*...*/}
  entity Approval @(restrict: [
    { grant: 'WRITE', where: '$user.level > 2' }
  ]){/*...*/}
}

is equivalent to

const cds = require('@sap/cds')
cds.serve ('CustomerService') .with (function(){
  this.before ('*', req =>
   req.user.is('authenticated') || req.reject(403)
  )
  this.before (['READ', 'CREATE'], 'Orders', req =>
    req.user.is('admin') || req.reject(403)
  )
  this.before ('*', 'Approval', req =>
    req.user.attr.level > 2 || req.reject(403)
  )
})

Note that before registration function is used to intercept the request and check the request parameter for authorization before it gets processed.

jeongsoolee09 commented 8 months ago

Sample CDS @restrict annotations scraped from CAPire:

entity Orders @(restrict: [
    { grant: 'READ', to: 'Auditor', where: 'AuditBy = $user' }
]) {/*...*/}

entity Reviews @(restrict: [
    { grant:['READ', 'WRITE'], to: ['Reviewer', 'Customer'] }
]) {/*...*/}

entity Orders @(restrict: [
    { grant: ['READ','WRITE'], to: 'Admin' },
    { grant: 'READ', where: 'buyer = $user' }
]) {/*...*/}

entity Orders @(restrict: [
    { grant: 'READ', to: 'Auditor', where: 'country = $user.country' },
    { grant: ['READ','WRITE'], where: 'CreatedBy = $user' },
]) {/*...*/}

service CatalogService {
    entity Products as projection on db.Products { ... }
    actions {
        @(requires: 'Admin')
        action addRating (stars: Integer);
    }
    function getViewsCount @(restrict: [{ to: 'Admin' }]) () returns Integer;
}

service CustomerService @(requires: 'authenticated-user') {
    entity Products @(restrict: [
        { grant: 'READ' },
        { grant: 'WRITE', to: 'Vendor' },
        { grant: 'addRating', to: 'Customer'}
    ]) {/*...*/}
    actions {
        action addRating (stars: Integer);
    }
    entity Orders @(restrict: [
        { grant: '*', to: 'Customer', where: 'CreatedBy = $user' }
    ]) {/*...*/}
    action monthlyBalance @(requires: 'Vendor') ();
}

Key takeaways:

So checking if the service is protected in terms of authentication requires:

jeongsoolee09 commented 4 months ago

My misunderstanding: The difference between @restrict and @requires isn't in the kind of CDL elements they can be attached to; rather, it's in in the granularity. @requires can only specify the roles that are permitted to access the resource, whereas @restrict can also specify the roles with access levels with where.

jeongsoolee09 commented 4 months ago

The services / entities / actions / functions can be protected with JS implementation code as well.

Protection with this.before

The conditional here lacks a meaningful success branch. Examples:

this.before("*", (req) => {
  if (!req.user.is("authenticated")) {
    req.reject(403);
  }
});

this.before("*", (req) => {
  req.user.is("authenticated") ? undefined : req.reject(403);
});

this.before("*", (req) => {
  req.user.is("authenticated") && true || req.reject(403);
});

Dual:

this.before("*", (req) => {
  if (req.user.is("authenticated")) {
  } else {
    req.reject(403);
  }
});

this.before("*", (req) => {
  !req.user.is("authenticated") ? req.reject(403) : undefined;
});

this.before("*", (req) => {
  (!req.user.is("authenticated") && req.reject(403)) || false;
});

Protection with this.on

The conditional here has both success branch and failure branch. Examples:

this.on("*", (req) => {
  if (req.user.is("authenticated")) {
    /* Do something */
  } else req.reject(403);
});

this.on("*", (req) => {
  req.user.is("authenticated") ? /* Do something */ true : req.reject(403);
});

this.on("*", (req) => {
  (req.user.is("authenticated") && /* Do something */ true) || req.reject(403);
});

Dual:

this.on("*", (req) => {
  if (!req.user.is("authenticated")) {
    req.reject(403);
  } else {
    /* Do something */
  }
});

this.on("*", (req) => {
  !req.user.is("authenticated") ? req.reject(403) : /* Do something */ true;
});

this.on("*", (req) => {
  (!req.user.is("authenticated") && req.reject(403)) || /* Do something */ true;
});