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(lockdown): change commit-debug.js to unsafe-debug #2359

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Jul 16, 2024

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

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 console
psm inter trade-debug-console

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.

  • Update NEWS.md for user-facing changes.

@erights erights self-assigned this Jul 16, 2024
@erights erights marked this pull request as ready for review July 16, 2024 23:21
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@erights erights requested a review from FUDCo July 17, 2024 18:27
@erights erights merged commit c7a80fd into master Jul 17, 2024
17 checks passed
@erights erights deleted the markm-fix-lockdown-commit-debug branch July 17, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants