Skip to content

Commit

Permalink
fix(lockdown): change commit-debug.js to unsafe-debug (#2359)
Browse files Browse the repository at this point in the history
*BREAKING*: Importing `@endo/lockdown/commit-debug.js` will now endanger integrity as well, in order to get better line numbers. `@endo/lockdown/commit-debug.js` should normally be used only for development/debugging/testing scenarios, where it is not potentially in contact with any genuinely malicious code. For those cases, there is no BREAKING change here. But we're flagging a potentially breaking change in case anyone is importing `@endo/lockdown/commit-debug.js` in scenarios where potentially malicious code is a concern. Such uses should be revised to avoid setting `errorTaming: 'unsafe-debug'`.


Closes: #XXXX
Refs: #2355
Agoric/agoric-sdk#9711

## Description

In #2355 , we introduced a new `errorTaming` setting, `'unsafe-debug'`,
to sacrifice more security for a better debugging experience. In many
ways it would have been more convenient to modify `'unsafe'` to do this,
rather than introduce a new setting. But we did not because the
`'unsafe'` setting was already documented as threatening only
confidentiality, not integrity. Many production scenarios don't need the
`'safe'` level of confidentiality defense, and so have been using
`errorTaming: 'unsafe'` in production, as shown below. Thus, a less-safe
form had to be a new mode, so the loss of safety was purely opt-in.

However, for uses that were clearly development-only uses, especially
those that are explicitly about testing/debugging, they often inherit
these lockdown settings from a long chain of imports bottoming out in
```js
import '@endo/lockdown/commit-debug.js';
```

Since this import is explicitly named something-debug, and since none of
the production uses of `errorTaming: 'unsafe'` that we are aware of
inherit it from `endo/lockdown/commit-debug.js`, it seems we could
recover the convenience for those testing/debugging uses by modifying
this file in place, with an acceptable security risk. This PR does so.


### Some existing uses of `errorTaming: 'unsafe'`

Some of these are in production code


https://github.com/LavaMoat/LavaMoat/blob/1cb057b281d46d2564872f53c2769bf4c4cb0ba5/packages/webpack/src/plugin.js#L54


https://github.com/LavaMoat/LavaMoat/blob/1cb057b281d46d2564872f53c2769bf4c4cb0ba5/packages/core/src/kernelTemplate.js#L60


https://github.com/LavaMoat/LavaMoat/blob/1cb057b281d46d2564872f53c2769bf4c4cb0ba5/packages/webpack/src/plugin.js#L54


https://github.com/MetaMask/metamask-extension/blob/7b3450a294a2fe5751726a9c33d6fa0b564f03dd/app/scripts/lockdown-run.js#L6


https://github.com/MetaMask/snaps/blob/0a265dcf9de73a16a7b50cc47681fe6da179383a/packages/snaps-utils/src/eval-worker.ts#L14


https://github.com/MetaMask/snaps/blob/0a265dcf9de73a16a7b50cc47681fe6da179383a/packages/snaps-execution-environments/src/common/lockdown/lockdown.ts#L15

From the [psm.inter.trade](https://psm.inter.trade/) console
![psm inter
trade-debug-console](https://github.com/user-attachments/assets/cd01501c-9a0c-43d6-a529-67fd1ca3ae43)


### Security Considerations

As explained above. If there are any production uses that get their
lockdown settings by importing `@endo/lockdown/commit-debug.js`, they
will experience a silent loss of security when they upgrade to depend on
this more recent version. We are not aware of any such cases. Because of
the explicit "debug" in the name, we expect such cases to be rare. But
we have no way to confirm they do not exist.

Reviewers, please let me know if you'd like me to change from a `fix:`
to a `fix!` because of this silent loss of security on upgrading the
dependency.

### Scaling Considerations

none
### Documentation Considerations

#2355 documents the differences between `'unsafe'` and `'unsafe-debug'`
for both security and functionality.

- [ ] Somewhere we need to explain that `endo/lockdown/commit-debug.js`
has changed in place, explaining the difference.

### Testing Considerations

Most of our tests inherit their `lockdown` settings from importing
`@endo/lockdown/commit-debug.js`, so these test would experience both
the security loss --- which should not matter for testing --- and
functionality gain of seeing correct line-numbers into transpiled code,
such as TypeScript sources.

### Compatibility Considerations

For development purposes, practically none, since the loss of security
in question is unlikely to matter. The improvement in line numbers will
help developers looking at stack traces, but is unlikely to affect any
code.

This change is not compatible with production code that imports
`@endo/lockdown/commit-debug.js`. But as the name indicates, production
code should not have been importing this file anyway.

### Upgrade Considerations

No upgrade issues

- [ ] Include `*BREAKING*:` in the commit message with migration
instructions for any breaking change.

- [x] Update `NEWS.md` for user-facing changes.
  • Loading branch information
erights authored Jul 17, 2024
1 parent 5b969bd commit c7a80fd
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 1 deletion.
19 changes: 19 additions & 0 deletions packages/lockdown/NEWS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
User-visible changes to `@endo/lockdown`:

# Next release

- Changed `@endo/lockdown/commit-debug.js` so that it now sets
the `lockdown` option `errorTaming: 'unsafe-debug'` instead of
just `errorTaming: 'unsafe'`. This is a further loss of safety in
exchange for a better development experience. For testing and debugging
purposes during development, this is usually the right tradeoff.

In particular,
`errorTaming: 'unsafe'` endangered only confidentiality, whereas
`errorTaming: 'unsafe-debug'` also endangers integrity, essentially by
directly exposing the (non-standard and dangerous) v8 `Error`
constructor API.

In exchange, stack traces will more often have accurate line numbers into
the sources of transpiled code, such as TypeScript sources. See
[`errorTaming` Options](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md#errortaming-options) for more on these tradeoffs.
2 changes: 1 addition & 1 deletion packages/lockdown/commit-debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ lockdown({
// NOTE TO REVIEWERS: If you see the following line commented out,
// this may be a development accident that should be fixed before merging.
//
errorTaming: 'unsafe',
errorTaming: 'unsafe-debug',

// The default `{stackFiltering: 'concise'}` setting usually makes for a
// better debugging experience, by severely reducing the noisy distractions
Expand Down

0 comments on commit c7a80fd

Please sign in to comment.