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.

Sign up for GitHub

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

feat(lavamoat/lavadome): update integration to improve security #25653

Merged
merged 9 commits into from
Jan 21, 2025
+9 −5
Merged

feat(lavamoat/lavadome): update integration to improve security #25653

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8ffe855
draft - require lavadome early enough to win security
weizman Jul 3, 2024
fc8f9dc
add font-src to csp v3
weizman Jul 3, 2024
965044f
add font-src to csp v2
weizman Jul 3, 2024
ad0e483
Merge branch 'develop' into weizman-patch-lavadome-try
weizman Oct 27, 2024
7d9fbd8
support brave too
weizman Jan 14, 2025
a1b9251
Merge branch 'main' into weizman-patch-lavadome-try
weizman Jan 14, 2025
bfb52b7
Merge branch 'main' into weizman-patch-lavadome-try
weizman Jan 14, 2025
b45f3c4
fix comments
weizman Jan 20, 2025
12c8181
Merge branch 'main' into weizman-patch-lavadome-try
weizman Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/manifest/v2/chrome.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"content_security_policy": "frame-ancestors 'none'; script-src 'self' 'wasm-unsafe-eval'; object-src 'none'",
"content_security_policy": "frame-ancestors 'none'; script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; font-src 'self';",
weizman marked this conversation as resolved.
Show resolved Hide resolved
"externally_connectable": {
"matches": ["https://metamask.io/*"],
"ids": ["*"]
Expand Down
4 changes: 2 additions & 2 deletions app/manifest/v3/chrome.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"content_security_policy": {
"extension_pages": "script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; frame-ancestors 'none';",
"sandbox": "sandbox allow-scripts; script-src 'self' 'unsafe-inline' 'unsafe-eval'; object-src 'none'; default-src 'none'; connect-src *;"
"extension_pages": "script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; frame-ancestors 'none'; font-src 'self';",
"sandbox": "sandbox allow-scripts; script-src 'self' 'unsafe-inline' 'unsafe-eval'; object-src 'none'; default-src 'none'; connect-src *; font-src 'self';"
},
"externally_connectable": {
"matches": ["https://metamask.io/*"],
Expand Down
4 changes: 4 additions & 0 deletions app/scripts/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
// Disabled to allow setting up initial state hooks first

// This import sets up safe intrinsics required for LavaDome to function securely.
// It must be run first so that no other code can undermine it.
import '@lavamoat/lavadome-react';
Copy link
Contributor

Choose a reason for hiding this comment

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

are the intrinsic not sufficiently protected by lavamoat?

Copy link
Member Author

Choose a reason for hiding this comment

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

no because LavaMoat protects TC39 JS intrinsics without taking platform oriented intrinsics into account (such as dom/web apis)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI: there are 8 or 9 other files that already run before this "first" thing here (not including the lavamoat runtime wrapper):

  • _initialize.ts (in webpack build only)
  • sentry-install
  • @lavamoat/snow/snow.prod
  • use-snow
  • lockdown-install
  • lockdown-run
  • lockdown-more
  • init-globals
  • runtime-cjs

And the next line after this import '@lavamoat/lavadome-react'; is:

// This import sets up global functions required for Sentry to function.
// It must be run first in case an error is thrown later during initialization.
import './lib/setup-initial-state-hooks';

So it is looking like we have a few files that already run before our "first" files, and we have two files that now "must be run first" (that won't be). I don't mind having a "first" comment in this file (despite it not actually being first in the context of the entire application), but we should certainly have only one of these comments in this file.

My current thinking is that ./lib/setup-initial-state-hooks should be run before lavadome-react unless you can guarantee that @lavamoat/lavadome-react will absolutely never be the cause of an error.

In either case (setup-initial-state-hooks or lavadoem-react running "first"), can you update the comments to reflect their actual "firstness"? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the phrasing b45f3c4.

Regarding sentry before lavadome, i understand your concern, but eventually, ./lib/setup-initial-state-hooks transitively requires tons of stuff that can end up undermining crucial LavaMoat related protections. This includes Snow, LavaMoat and LavaDome too - all of which we decided to trust to run before any other code we can't 100% trust. Just like LavaMoat and Snow, LavaDome can potentially cause an error, but we take all the measures needed to mitigate this risk as much as possible, and live with any small risk left of causing error, knowing that it's worth deploying powerful defense to our runtime environment.


// This import sets up global functions required for Sentry to function.
// It must be run first in case an error is thrown later during initialization.
import './lib/setup-initial-state-hooks';
Expand Down