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

core: Use proper sandbox type per SWF #17756

Merged
merged 9 commits into from
Sep 13, 2024
Merged

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Sep 4, 2024

It's the first step of implementing the security sandbox and its filesystem/network separation policy. Before that Ruffle assigned localTrusted sandbox type for all movies except web, where remote was used. The sandbox type also was assigned to the player, and not SWFs as it should. This PR does not introduce any sandbox policies based on the sandbox type, just the proper sandbox type detection. It is also not possible to specify trusted movies, and AIR applications don't use the application type yet.

The logic goes as follows. It is detected whether a SWF is local or remote based on its URL (URLs with scheme file are treated as local). Every remote SWF receives the remote sandbox type. For local SWFs, their FileAttributes are checked for useNetwork, which if true will assign localWithNetwork and if false will assign localWithFile sandbox type.

This behavior is covered by the following set of tests:

  • avm*/sandbox_type_local_networklocalWithNetwork type in AVM1/AVM2,
  • avm*/sandbox_type_local_filelocalWithFile type in AVM1/AVM2,
  • avm*/sandbox_type_remoteremote type in AVM1/AVM2, a localWithNetwork SWF loads another one from the network.

The cases from the tests above were the only cases I found where implementing sandbox policies was not needed for the test to pass.

@kjarosh kjarosh added security Pull requests that address a security vulnerability waiting-on-review Waiting on review from a Ruffle team member labels Sep 4, 2024
@Croworbit
Copy link

this sounds good, how does this help?
(not saying it won't, I know sandboxing can be good but don't actually know much)

@evilpie
Copy link
Collaborator

evilpie commented Sep 8, 2024

I am really quite curious about how this is going to work in practice. e.g. can SWFs with localWithNetwork load SWFs with localWithFile? I am mostly thinking about the boundary between different SWFs with different sandbox levels.

core/src/sandbox.rs Outdated Show resolved Hide resolved
@@ -92,30 +104,38 @@ impl SwfMovie {
/// `LoaderInfo.bytesTotal` is set to the actual value, but no data is available,
/// and `LoaderInfo.parameters` is empty.
pub fn fake_with_compressed_len(swf_version: u8, compressed_len: usize) -> Self {
let url = "file:///".to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pretty sure this is going to be a problem in the future. We need something like #17717.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, added TODOs for now, but this will need a lot more tests for sure

@kjarosh
Copy link
Member Author

kjarosh commented Sep 8, 2024

I am really quite curious about how this is going to work in practice. e.g. can SWFs with localWithNetwork load SWFs with localWithFile? I am mostly thinking about the boundary between different SWFs with different sandbox levels.

From my testing (if I remember correctly):

  • localWithFile can load localWithFile,
  • localWithFile cannot load localWithNetwork,
  • localWithNetwork cannot load localWithFile,
  • localWithNetwork can load localWithNetwork despite not having access to the filesystem,
  • localWithNetwork can load remote,
  • localWithNetwork cannot interact with objects from remote,
  • localWithFile cannot load remote,
  • remote cannot load localWithFile and localWithNetwork.

I'll be adding some tests (known failures) related to these behaviors along with the thrown exceptions so hopefully I'll cover these cases and even some more.

In AVM1 IIRC no exceptions are thrown, actions are canceled/prevented and security sandbox messages are traced.

It's the first step of implementing the security sandbox and its
filesystem/network separation policy.
Before that Ruffle assigned `localTrusted` sandbox type for
all movies except web, where `remote` was used.
The sandbox type also was assigned to the player,
and not SWFs as it should.
This patch does not introduce any sandbox policies based
on the sandbox type, just the proper sandbox type detection.
It is also not possible to specify trusted movies,
and AIR applications don't use the `application` type yet.
Verifies the sandbox type of a local SWF with network disabled.
Verifies the sandbox type of a local SWF with network enabled.
Verifies the sandbox type of SWFs loaded through network.
Verifies the sandbox type of a local SWF with network disabled.
Verifies the sandbox type of a local SWF with network enabled.
Verifies the sandbox type of SWFs loaded through network.
@kjarosh kjarosh enabled auto-merge (rebase) September 13, 2024 15:25
@kjarosh kjarosh removed the waiting-on-review Waiting on review from a Ruffle team member label Sep 13, 2024
@kjarosh kjarosh merged commit 24b0c8b into ruffle-rs:master Sep 13, 2024
17 checks passed
@kjarosh kjarosh deleted the sandbox-per-swf branch September 13, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants