Skip to content
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

fix(connector-besu/quorum/xdai): Unvalidated dynamic method call #1911

Closed
petermetz opened this issue Mar 14, 2022 · 0 comments · Fixed by #1912
Closed

fix(connector-besu/quorum/xdai): Unvalidated dynamic method call #1911

petermetz opened this issue Mar 14, 2022 · 0 comments · Fixed by #1912
Assignees
Labels
Besu dependencies Pull requests that update a dependency file P1 Priority 1: Highest Quorum Security Related to existing or potential security vulnerabilities Xdai Tasks/bugs related to the Xdai network and the corresponding ledger connector plugin in Cactus

Comments

@petermetz
Copy link
Member

Invocation of method with name may dispatch to unexpected target and cause an exception.

https://github.com/hyperledger/cactus/security/code-scanning/23?query=ref%3Arefs%2Fpull%2F1839%2Fhead
https://github.com/hyperledger/cactus/security/code-scanning/24?query=ref%3Arefs%2Fpull%2F1839%2Fhead
https://github.com/hyperledger/cactus/security/code-scanning/25?query=ref%3Arefs%2Fpull%2F1839%2Fhead
https://github.com/hyperledger/cactus/security/code-scanning/26?query=ref%3Arefs%2Fpull%2F1839%2Fhead

Tool
CodeQL
Rule ID
js/unvalidated-dynamic-method-call
Query
View source

JavaScript makes it easy to look up object properties dynamically at runtime. In particular, methods can be looked up by name and then called. However, if the method name is user-controlled, an attacker could choose a name that makes the application invoke an unexpected method, which may cause a runtime exception. If this exception is not handled, it could be used to mount a denial-of-service attack.

For example, there might not be a method of the given name, or the result of the lookup might not be a function. In either case the method call will throw a TypeError at runtime.

Another, more subtle example is where the result of the lookup is a standard library method from Object.prototype, which most objects have on their prototype chain. Examples of such methods include valueOf, hasOwnProperty and defineSetter. If the method call passes the wrong number or kind of arguments to these methods, they will throw an exception.
Recommendation

It is best to avoid dynamic method lookup involving user-controlled names altogether, for instance by using a Map instead of a plain object.

If the dynamic method lookup cannot be avoided, consider whitelisting permitted method names. At the very least, check that the method is an own property and not inherited from the prototype object. If the object on which the method is looked up contains properties that are not methods, you should additionally check that the result of the lookup is a function. Even if the object only contains methods, it is still a good idea to perform this check in case other properties are added to the object later on.
Example

In the following example, an HTTP request parameter action property is used to dynamically look up a function in the actions map, which is then invoked with the payload parameter as its argument.

var express = require('express');
var app = express();

var actions = {
  play(data) {
    // ...
  },
  pause(data) {
    // ...
  }
}

app.get('/perform/:action/:payload', function(req, res) {
  let action = actions[req.params.action];
  // BAD: `action` may not be a function
  res.end(action(req.params.payload));
});

The intention is to allow clients to invoke the play or pause method, but there is no check that action is actually the name of a method stored in actions. If, for example, action is rewind, action will be undefined and the call will result in a runtime error.

The easiest way to prevent this is to turn actions into a Map and using Map.prototype.has to check whether the method name is valid before looking it up.

var express = require('express');
var app = express();

var actions = new Map();
actions.put("play", function play(data) {
  // ...
});
actions.put("pause", function pause(data) {
  // ...
});

app.get('/perform/:action/:payload', function(req, res) {
  if (actions.has(req.params.action)) {
    let action = actions.get(req.params.action);
    // GOOD: `action` is either the `play` or the `pause` function from above
    res.end(action(req.params.payload));
  } else {
    res.end("Unsupported action.");
  }
});

If actions cannot be turned into a Map, a hasOwnProperty check should be added to validate the method name:

var express = require('express');
var app = express();

var actions = {
  play(data) {
    // ...
  },
  pause(data) {
    // ...
  }
}

app.get('/perform/:action/:payload', function(req, res) {
  if (actions.hasOwnProperty(req.params.action)) {
    let action = actions[req.params.action];
    if (typeof action === 'function') {
      // GOOD: `action` is an own method of `actions`
      res.end(action(req.params.payload));
      return;
    }
  }
  res.end("Unsupported action.");
});

References

  • OWASP: Denial of Service.
  • MDN: Map.
  • MDN: Object.prototype.
  • Common Weakness Enumeration: CWE-754.
@petermetz petermetz added Besu dependencies Pull requests that update a dependency file P1 Priority 1: Highest Quorum Security Related to existing or potential security vulnerabilities Xdai Tasks/bugs related to the Xdai network and the corresponding ledger connector plugin in Cactus labels Mar 14, 2022
@petermetz petermetz self-assigned this Mar 14, 2022
petermetz added a commit to petermetz/cacti that referenced this issue Mar 14, 2022
Added checks to make sure that the Web3 Contract instances
"methods" object has a property of their own called
the same way the method is called by the request
object. This way if someone tries to execute malicious
code by providing method names that are designed to
execute something other than the smart contract methods
we throw back an error to them instead of complying.

This is needed to fix the following CodeQL security advisories:
https://github.com/hyperledger/cactus/security/code-scanning/23
https://github.com/hyperledger/cactus/security/code-scanning/24
https://github.com/hyperledger/cactus/security/code-scanning/25
https://github.com/hyperledger/cactus/security/code-scanning/26

Todo for later: create a web3-common package that can
be used to house re-usable pieces of code such as the
function that validates if a contract really has a certain
method or not. Right now this method is copy pasted
to all 3 web3 flavored connectors which is not very nice.

Fixes hyperledger#1911

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Mar 14, 2022
Added checks to make sure that the Web3 Contract instances
"methods" object has a property of their own called
the same way the method is called by the request
object. This way if someone tries to execute malicious
code by providing method names that are designed to
execute something other than the smart contract methods
we throw back an error to them instead of complying.

This is needed to fix the following CodeQL security advisories:
https://github.com/hyperledger/cactus/security/code-scanning/23
https://github.com/hyperledger/cactus/security/code-scanning/24
https://github.com/hyperledger/cactus/security/code-scanning/25
https://github.com/hyperledger/cactus/security/code-scanning/26

Todo for later: create a web3-common package that can
be used to house re-usable pieces of code such as the
function that validates if a contract really has a certain
method or not. Right now this method is copy pasted
to all 3 web3 flavored connectors which is not very nice.

Fixes hyperledger#1911

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Mar 14, 2022
Added checks to make sure that the Web3 Contract instances
"methods" object has a property of their own called
the same way the method is called by the request
object. This way if someone tries to execute malicious
code by providing method names that are designed to
execute something other than the smart contract methods
we throw back an error to them instead of complying.

This is needed to fix the following CodeQL security advisories:
https://github.com/hyperledger/cactus/security/code-scanning/23
https://github.com/hyperledger/cactus/security/code-scanning/24
https://github.com/hyperledger/cactus/security/code-scanning/25
https://github.com/hyperledger/cactus/security/code-scanning/26

Todo for later: create a web3-common package that can
be used to house re-usable pieces of code such as the
function that validates if a contract really has a certain
method or not. Right now this method is copy pasted
to all 3 web3 flavored connectors which is not very nice.

Fixes hyperledger#1911

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit that referenced this issue Mar 14, 2022
Added checks to make sure that the Web3 Contract instances
"methods" object has a property of their own called
the same way the method is called by the request
object. This way if someone tries to execute malicious
code by providing method names that are designed to
execute something other than the smart contract methods
we throw back an error to them instead of complying.

This is needed to fix the following CodeQL security advisories:
https://github.com/hyperledger/cactus/security/code-scanning/23
https://github.com/hyperledger/cactus/security/code-scanning/24
https://github.com/hyperledger/cactus/security/code-scanning/25
https://github.com/hyperledger/cactus/security/code-scanning/26

Todo for later: create a web3-common package that can
be used to house re-usable pieces of code such as the
function that validates if a contract really has a certain
method or not. Right now this method is copy pasted
to all 3 web3 flavored connectors which is not very nice.

Fixes #1911

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Besu dependencies Pull requests that update a dependency file P1 Priority 1: Highest Quorum Security Related to existing or potential security vulnerabilities Xdai Tasks/bugs related to the Xdai network and the corresponding ledger connector plugin in Cactus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant