Defense against prototype pollution and supply chain attacks

kriskowal commented 4 years ago

At @agoric, we’ve begun investigating how compatible CosmJS is with SES such that projects using CosmJS can use tools like SES and LavaMoat to mitigate supply chain and prototype pollution attacks.

To evaluate whether a module and its transitive dependencies can be initialized under SES, I’ve locally run the following script in each of the CosmJS library packages. These are the packages that all have a single entry-module named ./build/index.js.

require("ses/lockdown");
lockdown();
console.log(require("./build/index.js"));

This provides some basic assurance that these libraries could be used in a project using SES. Likely incompatibilities usually arise when a library depends on a shim or polyfill, and these can usually be fixed by using the corresponding “ponyfill”, like colors/safe instead of colors. (Though I would note, CosmJS depends much more heavily on the equivalent ansi-colors module that does not perform any ~~prototype attacks~~ monkey-patching.)

[x] encoding
[x] math
[x] proto-signing
[x] utils
[x] tendermint-rpc
[x] stargate
[x] launchpad
[x] crypto
[x] cosmwasm

The following depend on a broken ponyfill, observable-symbol/ponyfill, which we haved worked with Ben Lesh to fix. This fix comes in a new major version, so an explicit upgrade should get the remaining libraries working. https://github.com/benlesh/symbol-observable/pull/48

[x] json-rpc
[x] socket
[x] stream

Of the two applications, faucet and cli, the faucet appears to work with the SES lockdown snippet at the head of its bin.

#!/usr/bin/env node
require("ses/lockdown");
lockdown();

[x] faucet
[ ] cli

The CLI runs into this mysterious error, far far from any likely cause:

  cosmjs/node_modules/ast-types/lib/types.js:281
                      throw new Error("missing name");
                      ^

  [Error <Object <[Object: null prototype] {}>>: missing name
    at Object.from (cosmjs/node_modules/ast-types/lib/types.js:281:27)
    at cosmjs/node_modules/ast-types/lib/types.js:247:71
    at Array.map (<anonymous>)
    at or (cosmjs/node_modules/ast-types/lib/types.js:247:37)
    at default_1 (cosmjs/node_modules/ast-types/def/core.js:267:25)
    at use (cosmjs/node_modules/ast-types/fork.js:47:31)
    at Array.forEach (<anonymous>)
    at Object.default_1 [as default] (cosmjs/node_modules/ast-types/fork.js:14:10)
    at Object.<anonymous> (cosmjs/node_modules/ast-types/main.js:18:24)
    at Module._compile (internal/modules/cjs/loader.js:1201:30)]

This is a great place to start. From this position, we would ideally add SES lockdown to the testing aparatus for each of these packages, so every pull request against them would be verified to remain compatible.

To this end, there are a number of ways that Jasmine and Karma are themselves not yet ready to run under environmental lockdown, largely because they use the infamous colors package, which in addition to monkey-patching String.prototype upon initialization, provides a “themes” feature that enables users to indirectly punch String.prototype at runtime.

Karma should work with this proposed and accepted fix to use colors/safe instead. It would be nice if it used ansi-colors just to reduce duplication among your transitive dependencies, but harmless otherwise. https://github.com/karma-runner/karma/pull/3548

[x] proposed changes
[x] landed
[x] released (requires karma > 5.2.1)
[x] integrated in cosmjs

Jasmine itself is largely compatible, because the only primordial it modifies is globalThis, and SES lockdown leaves this as an exercise to the user.

Jasmine Test Reporter makes uses of colors themes. To overcome this obstacle, we would need either an alternate test reporter plugin or a major version bump and an architecture more closely aligned with dependency injection. I’ve proposed the changes. The author finds them amenable. Work would need to be planned. https://github.com/bcaudan/jasmine-spec-reporter/issues/528

[x] proposed design
[x] implemented design
[x] landed
[x] released
[x] integrated in cosmjs

The nature of SES compatibility exploration is that it is easier to keep than to obtain. There may be further obstacles behind these first exceptions I’ve encountered, but overall, it seems CosmJS is very nearly already compatible with SES and could encourage users to use it (and LavaMoat) to protect their applications from these kinds of attack.

webmaster128 commented 4 years ago

Thanks a lot for all the effort @kriskowal. Happy to support this work where possible.

For the observable-symbol update we need https://github.com/staltz/xstream/issues/312. For the merged patch in karma, we need a release > 5.2.1.

kriskowal commented 4 years ago

I’m delighted to share, jasmine-test-reporter@6 relieves the dependency on String.prototype pollution from colors. There is a minor API change, where the color theme must be expressly provided to the reporter.

kriskowal commented 4 years ago

Thanks a lot for all the effort @kriskowal. Happy to support this work where possible.

For the observable-symbol update we need staltz/xstream#312. For the merged patch in karma, we need a release > 5.2.1.

I’ve taken the liberty to propose this change with https://github.com/staltz/xstream/pull/315

kriskowal commented 4 years ago

[ ] xstream needs more TypeScript definitions from observable-symbol. https://github.com/benlesh/symbol-observable/pull/50
[ ] most, another dependency of xstream, needs an observable-symbol version bump. https://github.com/cujojs/most

webmaster128 commented 4 years ago

most, another dependency of xstream, needs an observable-symbol version bump. https://github.com/cujojs/most

This is just a devDependency of xstream. Does this really matter for as as users of xstream?

kriskowal commented 4 years ago

This is just a devDependency of xstream. Does this really matter for as as users of xstream?

I suspect that it will be hard to land the version bump to xstream without also bumping most, since these presumably need to agree for xstream tests to pass. I could be mistaken.

kriskowal commented 4 years ago

Ben Lesh released symbol-observable@2.0.2 this morning with new TypeScript definitions.

I’ve posted a change for MostJS that brings it up to speed with symbol-observable and switches it to use the ponyfill so it too can be used under SES. https://github.com/cujojs/most/pull/541

kriskowal commented 4 years ago

And, attempting to upgrade xstream, evidently I forgot to add ponyfill.d.ts to the files mask in package.json. Turning the crank again: https://github.com/benlesh/symbol-observable/pull/51

kriskowal commented 4 years ago

Summary of what’s happened so far:

The karma test runner released with a fix for SES compatibility https://github.com/karma-runner/karma/pull/3548
The jasmine-spec-reporter released a major upgrade with SES compatibility https://github.com/bcaudan/jasmine-spec-reporter/pull/538
symbol-observable@2.0.3 has been released. This version can run under SES with some small changes to how it’s used. CosmJS uses this through its dependency on xstream, which in turn uses most, which is only dependent upon it to the extent it’s necessary to pass its own tests. Once these dependencies use the new version, we’ll be able to upgrade them in CosmJS which will make CosmJS combine well with SES. xstream in particular needed TypeScript support for the symbol-observable change.
I have an open change to most in review at https://github.com/cujojs/most/pull/541. This entailed an upgrade for symbol-observable, changes to how the library uses symbol-observable, and an additional dependency on globalthis to facilitate that.
I have a draft change to xstream https://github.com/staltz/xstream/pull/315 This is pending a release of most with the above changes, and will require a similar treatment as the change to most.

The pending work, in steps, is:

[x] Await a new release of most. I anticipate no further work.
[x] Finish integrating most into xstream and await a new release.
[x] Upgrade the new xstream in cosmjs packages and propose changes. At this point cosmjs is SES compatible.
[x] Upgrade the karma and jasmine-spec-reporter dependencies of cosmjs and propose changes. At this point, cosmjs will have continuous verification of SES compatibility.

webmaster128 commented 3 years ago

@kriskowal could you double check karma and jasmine-spec-reporter. I think I updated them after your patched.

kriskowal commented 3 years ago

@webmaster128 I’ll be sure to. The next, and I presume last, step is for me to propose a change that adds SES to the test scaffold to confirm that CosmJS is now SES-compatible and to prove against regressions going forward.

webmaster128 commented 3 years ago

@kriskowal what do you mean by "test scaffold"? Would it be possible for us to pick up your work from here or does this require some work in the SES-shim repo?

kriskowal commented 3 years ago

@webmaster128 There’s an opportunity in the jasmine-test-runner scripts in each package to import a module early in initialization that in turn imports ses and calls lockdown(). This would be the final step to ensuring that all of CosmJS can be executed in a SES programming environment, that is, with immutable shared prototypes.

willclarktech commented 3 years ago

Investigating the error from the @cosmjs/cli package, I believe it’s introduced by the recast dependency because this line causes the error to happen even if the babylon parser options are removed:

https://github.com/cosmos/cosmjs/blob/4b7a060/packages/cli/src/async.ts#L9

Looking into recast a little bit, I had a hunch the problem might come from their dependency on private (https://github.com/benjamn/private), but I tried updating recast to the latest version which doesn’t have that dependency any more (but does have a new dependency on tslib - https://github.com/microsoft/tslib) and the problem remained.

webmaster128 commented 3 years ago

The missing name that the error complains about has those 3 different values when running without SES:

[Function: default_1]
Function name: string | number | boolean | null | undefined
Function name: number >= 1
Function name: number >= 0

This does not make much sense to me. Why is string | number | boolean | null | undefined a name, which looks like a TypeScript type? Why is number >= 1 a name which looks like a function body.

I guess this requires a larger debugging session.

cosmos / cosmjs

Defense against prototype pollution and supply chain attacks #412