fix(security): close DDoS vulnerability in eth tx consistenty strategy

[petermetz]

Description

The application performs some repetitive tasks in a loop and defines the number of times to perform the loop according to user input. A very high value could cause the application to get stuck in the loop and to be unable to continue to other operations.

Affected files are all the eth flavored connector plugins that have the consistencyStrategy request parameter when executing transactions.

• packages/cactus-plugin-ledger-connector-besu/src/main/typescript/plugin-ledger-connector-besu.ts
• packages/cactus-plugin-ledger-connector-xdai/src/main/typescript/plugin-ledger-connector-xdai.ts
• packages/cactus-plugin-ledger-connector-quorum/src/main/typescript/plugin-ledger-connector-quorum.ts

    do {
      tries++;
      timedOut = Date.now() >= startedAt.getTime() + timeoutMs;
      if (timedOut) {
        break;
      }

      txReceipt = await this.web3.eth.getTransactionReceipt(txHash);
      if (!txReceipt) {
        continue;
      }

      const latestBlockNo = await this.web3.eth.getBlockNumber();
      confirmationCount = latestBlockNo - txReceipt.blockNumber;
    } while (confirmationCount >= consistencyStrategy.blockConfirmations);

Impact

An attacker could input a very high value, potentially causing a denial of service (DoS).

Remediation Recommendation

Don't base a loop on loosely validated user-provided data. The range should be limited. Always include a maximum value for each user input in the openapi.json specs.

Change the maximum for allowed block count to 20 thousand (a little over what the ethereum main net confirms in a 72 hour period according to the latest statistics at time of this writing: https://ycharts.com/indicators/ethereum_blocks_per_day

Breaking Change Discussion

Fixing this will be a breaking change, because previously valid requests will now get rejected by the API server.
An argument could be made that it's only a breaking change for those who were previously sending malicious requests and for no one else and therefore it is not really a breaking change even though it technically is.

@izuru0 @takeutak @jagpreetsinghsasan Please weigh in on the question presented in Breaking Change Discussion

[jagpreetsinghsasan]

Can we fix it by having a specified time delay in-between requests instead of limiting the range?

[petermetz]

Can we fix it by having a specified time delay in-between requests instead of limiting the range?

@jagpreetsinghsasan A rate-limiter would be a partial solution because it can be defeated. For example if it rate limits based on IP address then one can use a set of proxies to avoid it entirely. With that said, everything else is also just a partial solution because nothing eliminate the possibility of DoS completely. It is a cat and mouse game that has to be played forever.

Layering defenses is almost always a good idea, so IMO the strongest protection would be to do both.

[jagpreetsinghsasan]

Thanks @petermetz for descriptive reasoning. And I agree 100% with the fact that, layering the security measures is the best way out.

[aldousalvarez]

Hello @petermetz Can you assign me on this one? Thanks

[petermetz]

Hello @petermetz Can you assign me on this one? Thanks

@aldousalvarez Done, than kyou!

[petermetz]

BLOCKER/ISSUE: packages/cactus-plugin-ledger-connector-quorum/src/main/typescript/plugin-ledger-connector-quorum.ts - this file doesnt have any consistencyStrategy imports or usage.

@aldousalvarez My bad, packages/cactus-plugin-ledger-connector-quorum/src/main/typescript/plugin-ledger-connector-quorum.ts can be skipped.