VirusTotal / yara-x

A rewrite of YARA in Rust.
https://virustotal.github.io/yara-x/
BSD 3-Clause "New" or "Revised" License
619 stars 48 forks source link

Are thread properties modified within yara-x ? #182

Closed qjerome closed 1 week ago

qjerome commented 2 weeks ago

Sorry that is me again !

It turns that I want to use yara-x in a very specific use case where I need to use setns syscall in the thread I am running yara-x. The purpose is to be able to scan files in specific mnt namespaces.

Could you tell me if your are touching thread properties in yara-x ?

Since I integrated it my mnt namespace switching routine (using setns) stopped working. I basically fail at using an already opened and cached namespace through its file descriptor.

As soon as I remove any code using yara-x everything starts working again !

plusvic commented 2 weeks ago

yara-x's code doesn't do anything like that explicitly, but I don't know if the Rust runtime or some third-party library is doing tricky things with threads. Can you provide some additional details about your setup? Are you executing the yr binary? Or you are using the yara-x library directly?

qjerome commented 2 weeks ago

I am using yara-x library and APIs.

Rust has no runtime so I doubt there are side effects on that end.

I was thinking maybe to wasmtime again ! What is super strange is that some of my old code (not touched by yara-x integration) stopped working after using yara-x.

My integration code is very simple, the interesting part follows. The rest is one line of code to initialize the Scanner.

        // we switch to a cached namespace that holds opened
        // file descriptors to namespaces
        entry.switcher.enter()?;

        // we get a PathBuf as path ownership is passed down
        let pb = path.to_path_buf();
        // we create key to check if we already have cached
        // signatures for that file
        let key = Key::from_path_with_ns(&ns, path)?;

        let sigs = match self.signatures.get(&key) {
            // we have caches signatures
            Some(sig) => sig.clone(),
            // we don't have cached signatures
            None => {
                let mut sigs = vec![];
                // scanner.scanner() returns a Option<&mut yara_x::Scanner>
                // we use yara_x::Scanner::scan_file to scan the file we want
                let sr = scanner.scanner().unwrap().scan_file(pb).unwrap();
                // we extend the list of signatures
                sigs.extend(sr.matching_rules().map(|m| m.identifier().to_string()));
                // we update our cache
                self.signatures.insert(key.clone(), sigs.clone());
                sigs
            }
        };

        // we must be sure that we restore our namespace
        entry.switcher.exit().expect("failed to restore namespace");

As soon as I comment the code initializing the yara_x::Scanner to basically not use yara-x in the thread, everything works as expected.

plusvic commented 2 weeks ago

I'm not sure what to say. I'm afraid you are going to need to start making changes yara-x's code to narrow down possible root causes. What's the problem you are experiencing? Some error message that you can share?

qjerome commented 2 weeks ago

Alright, I'll try to make a minimal repro code and repository on monday so that I can eventually share it with you if you want to look at it on your side. Before touching the code, I'll try to strace the stuff to identify the root cause.

qjerome commented 2 weeks ago

I might have a clue why it is not working.

With strace I see yara-x spawning many threads with:

[pid 3317108] clone3({flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, child_tid=0x7a444e654990, parent_tid=0x7a444e654990, exit_signal=0, stack=0x7a444e454000, stack_size=0x1ffa00, tls=0x7a444e6546c0} <unfinished ...>

Meaning that my thread shares CLONE_FS with the yara-x childrens. The setns syscall returns an error if current thread shares CLONE_FS with other threads.

Extract from man setns

       EINVAL The caller shares filesystem (CLONE_FS) state (in particular, the  root  directory)  with
              other processes and tried to join a new user namespace.
qjerome commented 2 weeks ago

Ok, this is a confirmed issue. Using unshare(CLONE_FS) in my thread does not break my existing code anymore. I initially do that unshare thing in the setup of my tokio.rs runtime but the fact that yara-x spawns its own threads (from within my threads) kind of break that upstream unshare(CLONE_FS) I am doing at thread creation.

Do you know whether that would be possible to set the clone3 params used by yara-x (very likely wasmtime) so that I can exclude CLONE_FS ?

plusvic commented 1 week ago

Great investigation. I don't have control over how those threads are spawned by wasmtime but it looks like the threads are used for parallel compilation, and the good news is that parallel compilation can be disabled. Remove the parallel-compilation feature from here:

https://github.com/VirusTotal/yara-x/blob/1e7c28852c742da788540b160822ddf0d1ec0950/lib/Cargo.toml#L215

When I do this a don't see any calls to the clone3 syscall anymore. If everything works fine for you, I will add a new feature to yara-x itself for disabling it. The drawback is that compilation will be slower when you are compiling a large number of rules.

Currently this feature must be disabled at compile time, but I will evaluate the possibility of disabling it at runtime.

Please use the latest commit in the master branch, I recently made changes for minimizing the number of wasmtime features that yara-x is using.

plusvic commented 1 week ago

Another thing to take into account: if you specify a timeout for your scans, that will also spawn a new thread. I do have control over that thread, but the default behaviour of std::thread::spawn is passing all those flags to clone3.

I'll investigate if the rust API offers some option for tweaking this settings, but I'm afraid that would require Linux-specific code that calls unshare .

qjerome commented 1 week ago

Thank for your feedback, I'll only be able to try this out on monday :) For your custom thread maybe you could expose an API to initialize thread in the same way tokio.rs is doing: https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.on_thread_start This is the API I am using to unshare(CLONE_FS).

qjerome commented 1 week ago

Hey @plusvic,

I had time to test your stuff out this morning ! It seems it doesn't work with latest git commit ! The reason is still the same I guess, as running this little POC code under strace still shows many clone3 syscalls with CLONE_FS flag.

fn main() {
    // Create a compiler.
    let mut compiler = yara_x::Compiler::new();

    // Add some YARA source code to compile.
    compiler
        .add_source(
            r#"
    rule lorem_ipsum {
      strings:
        $ = "Lorem ipsum"
      condition:
        all of them
    }
"#,
        )
        .unwrap();

    // Obtain the compiled YARA rules.
    let rules = compiler.build();

    // Create a scanner that uses the compiled rules.
    let mut scanner = yara_x::Scanner::new(&rules);

    // Scan some data.
    let results = scanner.scan("Lorem ipsum".as_bytes()).unwrap();

    assert_eq!(results.matching_rules().len(), 1);
}
plusvic commented 1 week ago

Did you remove the parallel-compilation feature from this file?

https://github.com/VirusTotal/yara-x/blob/1e7c28852c742da788540b160822ddf0d1ec0950/lib/Cargo.toml#L215

After that change I don't see any clone3 call using your test program.

plusvic commented 1 week ago

I've introduced a new feature in 40e9d1720a02fe03c473757de67fdc9ba3749250. With that it's easier for you to turn off parallel compilation.

qjerome commented 1 week ago

Ok, it seems to work nice without parallel-compilation \o/

I just looked at your commit ! Don't you think it would make sense to make any sort of parallelization an optional feature ?

I personally see hidden parallelization something not wanted in a library especially in single threaded applications. Moreover I think it doesn't seem in line with what is done in other Rust crates where only the minimal needed is default and all the rest of the features are opt in.

plusvic commented 1 week ago

That's a good point. I'll gather more data about performance with and without parallel-compilation, if it's reasonably good without parallelization for a reasonable amount of rules, I'll make it optional.

However, that wouldn't make the library 100% single-threaded, as timeouts require a separate thread anyways.

qjerome commented 1 week ago

It could somehow ... if you make API leading to multi-threading behavior (such as Scanner::set_timeout I guess) available only if a parallelization feature is enabled.

I don't think you should bother with performance tests, if you write things down clearly in documentation. People who want better perfs or timeout should enable parallelization and you are done.

I you want to make parallelization behavior to remain the default you could also make a single-thread feature set such as:

parallel-compilation = ["wasmtime/parallel-compilation"]

single-thread = [
    "constant-folding",
    "exact-atoms",
    "fast-regexp",
    "console-module",
    "dotnet-module",
    "elf-module",
    "macho-module",
    "math-module",
    "hash-module",
    "pe-module",
    "string-module",
    "time-module",
    "lnk-module",
    "test_proto2-module",
    "test_proto3-module",
]

multi-threads = ["parallel-compilation", "single-thread"]

# Features that are enabled by default.
default = ["multi-threads"]

If you do that you can also disable APIs leading to parallelization if multi-threads feature is missing.

qjerome commented 1 week ago

Just so you know, if you don't have time to handle this issue, I can do it (under your directives) ;).

plusvic commented 1 week ago

@qjerome you could help by making the following experiment:

Using the syscalls create, call unshare(CLONE_FS) from the thread below and see it that fixes your original issue. I think we can unshare everything, because that threads is pretty simply and doesn't need to share any resources with its parent.

https://github.com/VirusTotal/yara-x/blob/5b1c3487688b1b8044aa292858c7752c1cef848e/lib/src/scanner/mod.rs#L542-L551

If everything works fine you don't need to worry about setting a timeout.

qjerome commented 1 week ago

I will try and let you know ! I don't know if it will work as the unshare call will happen after the thread is actually spawned with CLONE_FS flag. I it all depends how the linux kernel handle the thing ...

I was thinking about something (not knowing anything about how scanner is implemented in wasm). Isn't it possible to set a kind of timer within the scanner, which would be self checking against a start time ? Preventing this timeout thing to be managed in a separate thread.

This is a kind of pseudo code

let start = now()
// let suppose this loop is scanning input
loop {
  if now() - start > timeout {
    return TimeoutError;
  }
  // continue scanning input
  ...
}
qjerome commented 1 week ago

@plusvic, I confirm unsharing in the child thread works \o/

plusvic commented 1 week ago

@qjerome can you send a PR with those changes?

qjerome commented 1 week ago

For sure, if you could just let me know what to include or not in the PR, so that we're both sure it is gonna be accepted :)

plusvic commented 1 week ago

No, I'm just looking for something simple, calling unshare(CLONE_FS) (and everything that can be unshared because it's not needed by that thread) from the timeout thread. Of course, this must be included only in Linux.

qjerome commented 1 week ago

Yet that wouldn't solve the parallel-compilation thing required by wasmtime do you plan to fix that in another PR ?

plusvic commented 1 week ago

My purpose with this change is that the timeout thread doesn't cause issues in the future. For now I don't plan to disable the set_timeout API. The parallel-compilation feature will control only the use of threads for compiling with wasmtime. Whether this feature will be enabled by default or disabled by default is still under consideration.

qjerome commented 1 week ago

Thank, for the merge.

Do you plan to make a patch release for this fix ? Just to know whether I should use git version in my project dependency or if I should wait a bit.

plusvic commented 1 week ago

The project is changing so fast that I'm not making patch releases at this time, only minor releases once or twice a month. I expect to release version 0.8.0 in the next two weeks at most.

plusvic commented 4 days ago

Unfortunately PR #189 is causing issues:

https://github.com/VirusTotal/yara-x/actions/runs/10775030489/job/29878522401#step:9:1147

The unshare syscall is returning EPERM when run inside a Docker container.

EPERM : The calling process did not have the required privileges for this operation.

I will have to revert #189.

qjerome commented 2 days ago

Isn't it a seccomp issue in the container ?

plusvic commented 2 days ago

I'm not sure what's the root cause, but failing inside a simple Docker container is problematic. If you can investigate and shed some light on what's happening it would be great.

qjerome commented 2 days ago

Very likely there is some seccomp or LSM for docker children preventing to unshare. Maybe because if you unshare(CLONE_FS) inside a docker container you can navigate to other mnt namespaces on the host, which can be considered as container escaping. If you have a container running as root + unshare(CLONE_FS) I think it could even modify the host's file system with root privileges which in this case would be some form of LPE. These are many reason why it wouldn't be safe to allow unshare within containers.

Just found out this doc https://docs.docker.com/engine/security/seccomp/ stating that unshare is blocked by the default docker profile

qjerome commented 2 days ago

Isn't it possible to implement this timeout thing without spawning a separate thread ?