Page MenuHomePhabricator

Application Security Review Request : Wikifunctions Rust-Based Function Evaluator
Closed, ResolvedPublic

Description

Project Information

  • Name of tool/project: Wikifunctions
  • Project home page: wikifunctions.org
  • Name of team requesting review: Abstract Wikipedia
  • Primary contact: Cory Massaro
  • Target date for deployment: March 31, 2025 (end of Q3 FY25)
  • Link to code repository / patchset:

function-evaluator (rustversion)
function-schemata (rust)

Description of the tool/project:
This project is a new version of the function evaluator service (current Node version is here). It is largely a rewrite of the existing functionality, in Rust instead of JavaScript. This will afford us finer control over the WASI runtime we're using to run community-submitted code in Python and JS.

Description of how the tool will be used at WMF:
This will eventually replace the existing function evaluator.

Dependencies

List dependencies, or upstream projects that this project relies on.

The dependencies can be found in the Cargo.toml and Cargo.lock files for the two repositories. For convenience, here are the Cargo.toml files:
function-evaluator:

[dependencies]
ambient-authority = "0.0.2"
anyhow = "1.0.95"
axum = "0.8.1"
axum-test = "17.2.0"
crossbeam-channel = "0.5.13"
futures = "0.3.31"
rustix = "0.31.0"
serde_json = "1.0.132"
tokio = { version = "1.42.0", features = [ "full" ] }
tower = "0.5.2"
wasi-common = { version = "20.0.0", features = [ "tokio" ] }
wasmtime = "20.0.0"

function-schemata:

[dependencies]
hex = "0.4.3"
serde_json = "1.0.138"

[dependencies.apache-avro]
version = "0.17.0"
features = ["snappy"]

Has this project been reviewed before?

Please link to tasks or wiki pages of previous reviews.

No.

Working test environment

Please link or describe setup process for setting up a test environment.

NOTE: TODO write!

Post-deployment

Name of team responsible for tool/project after deployment and primary contact.

Abstract Wikipedia

Details

Risk Rating
Low

Event Timeline

sbassett changed the task status from Open to In Progress.Jan 9 2025, 5:24 PM
sbassett claimed this task.
sbassett triaged this task as Medium priority.
sbassett moved this task from Upcoming Quarter Planning Queue to In Progress on the secscrum board.

Hey @cmassaro et al - just wanted to confirm how close this was to review, i.e. are the dependencies/versions finalized and is there a release of the code that's in a reasonably-stable state at this point? Thanks.

Hey @cmassaro et al - just wanted to confirm how close this was to review, i.e. are the dependencies/versions finalized and is there a release of the code that's in a reasonably-stable state at this point? Thanks.

Hey! This should now be ready. There are tests and runnable binaries in both repositories. I've updated the description with lists of dependencies.

DSantamaria changed the task status from In Progress to Open.Mar 5 2025, 6:44 AM

FYI - this will likely be completed next week, due to ongoing incidents. osv-scanner did find a few vulns with the current dependency versions though, if @cmassaro wanted to address those in the interim:

╭─────────────────────────────────────┬──────┬───────────┬──────────┬─────────┬────────────╮
│ OSV URL                             │ CVSS │ ECOSYSTEM │ PACKAGE  │ VERSION │ SOURCE     │
├─────────────────────────────────────┼──────┼───────────┼──────────┼─────────┼────────────┤
│ https://osv.dev/RUSTSEC-2024-0436   │      │ crates.io │ paste    │ 1.0.15  │ Cargo.lock │
│ https://osv.dev/GHSA-7qmx-3fpx-r45m │ 2.9  │ crates.io │ wasmtime │ 20.0.0  │ Cargo.lock │
│ https://osv.dev/GHSA-q8hx-mm92-4wvg │ 6.8  │ crates.io │ wasmtime │ 20.0.0  │ Cargo.lock │
│ https://osv.dev/GHSA-c2f5-jxjv-2hh8 │ 2.3  │ crates.io │ wasmtime │ 20.0.0  │ Cargo.lock │
╰─────────────────────────────────────┴──────┴───────────┴──────────┴─────────┴────────────╯

Fixing wasmtime is tracked in T388981: In our Rust POC of the function-evaluator, switch from wasi-common to wasmtime-wasi and upgrade wasmtime to 21.0.2+ (or replace) (and AIUI not actually a risk for us, but we should fix it anyway).

Planning to post the review tomorrow or Thursday.

Security Review Summary - T379088 - 2025-04-09
Last commits reviewed: function-schemata:bc33518e77 and function-evaluator:09c83426c5

Summary

Overall, the current code looks fine, with various cleanup items detailed below and further production testing and fixing various TODOs. There doesn't seem to be a lot of obviously bad patterns here (e.g. unsafe blocks/functions/impls, raw pointers, etc.) but obviously this is doing some heavy lifting with Rust concurrency, running wasm-based interpreters and implementing a state machine. Given all of this and the inability to test in a production-like environment, I'm going to rate this a medium risk.

Vulnerable Packages - Production - function-evaluator
Risk: low.

VulnerabilityPackageServiceRemediationRisk
https://osv.dev/RUSTSEC-2024-0436paste@1.0.15osv[see advisory link] info
https://osv.dev/GHSA-7qmx-3fpx-r45mwasmtime@20.0.0osv[see advisory link] low
https://osv.dev/GHSA-q8hx-mm92-4wvgwasmtime@20.0.0osv[see advisory link] medium
https://osv.dev/GHSA-c2f5-jxjv-2hh8wasmtime@20.0.0osv[see advisory link] low

Static Analysis Findings
Risk: low.

function-evaluator

via clippy:

warning: returning the result of a `let` binding from a block
  --> src/service/mod.rs:10:5
   |
9  |     let app = Router::new().route("/info", get(endpoints::info));
   |     ------------------------------------------------------------- unnecessary `let` binding
10 |     app
   |     ^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
   = note: `#[warn(clippy::let_and_return)]` on by default
help: return the expression directly
   |
9  ~     
10 ~     Router::new().route("/info", get(endpoints::info))
   |

via semgrep (with various rustlang policies):

src/executor_example.rs
 ❱ rust.lang.security.args.args
      args should not be used for security operations. From the docs: "The first element is traditionally
      the path of the executable, but it can be set to arbitrary text, and might not even exist. This    
      means this property should not be relied upon for security purposes."                              
      Details: https://sg.run/RADN                                                                       
                                                                                                         
       37┆ let args = std::env::args().skip(1).collect::<Vec<_>>();
       
src/executor.rs
❯❱ panic-in-function-returning-result
      `expect` or `unwrap` called in function returning a `Result`
                                                                  
       56┆ let message = message_result.unwrap();
        ⋮┆----------------------------------------
       59┆ let mut locked_state_machine = try_locked_state_machine.unwrap();
        ⋮┆----------------------------------------
       83┆ let string_data = self.receiver.recv().unwrap();
        ⋮┆----------------------------------------
      154┆ let engine = Engine::new(&config).unwrap();
        ⋮┆----------------------------------------
      158┆ let module = Module::from_file(&engine, wasm_file).unwrap();
        ⋮┆----------------------------------------
      223┆ let mut locked_state_machine = try_locked_state_machine.unwrap();
                                      
src/executor_example.rs
❯❱ panic-in-function-returning-result
      `expect` or `unwrap` called in function returning a `Result`
                                                                  
       41┆ let file_contents = file_contents_result.unwrap();
        ⋮┆----------------------------------------
       42┆ let deserialized: serde_json::Value =                 
           serde_json::from_str(file_contents.as_str()).unwrap();
        ⋮┆----------------------------------------
       45┆ let to_run = to_run_result.unwrap();
        ⋮┆----------------------------------------
       58┆ let execution_result = executor.expect("").execute(to_run).await?;

function-schemata

via clippy:

warning: this `.into_iter()` call is equivalent to `.iter()` and will not consume the `array`
  --> src/lib.rs:66:42
   |
66 |     for (index, file_name) in file_names.into_iter().enumerate() {
   |                                          ^^^^^^^^^ help: call directly: `iter`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref

via semgrep (with various rustlang policies):

src/main.rs
❯❱ panic-in-function-returning-result
      `expect` or `unwrap` called in function returning a `Result`
                                                                  
       29┆ let mut the_string = Record::new(&schemata[2]).unwrap();
        ⋮┆----------------------------------------
       35┆ let mut the_code = Record::new(&schemata[0]).unwrap();
        ⋮┆----------------------------------------
       43┆ let mut the_converter = Record::new(&schemata[1]).unwrap();

General Issues and Misc.
Risk: medium.

  1. Noting that the function_evaluator code is fairly complex and the first true Rust microservice that would be introduced for Wikimedia projects
  2. The service_example seems to just be a "hello world" app right now? Loading http://0.0.0.0:5000/info just gives me the basic Json payload from the info endpoint defined by src/service/endpoints.rs.
  3. It might be nice to implement some basic fuzzing (via cargo-fuzz, etc.) for some of the code within executor.rs and executor_classes/mod.rs.
  4. I think we'll definitely want to engage a pentesting firm experienced with Rust as part of T373910, to further test the executor within a more production-like environment.
  5. Noting that, currently, there are three JS executor tests failing:
test executor_tests::test_javascript_add_with_type_converters ... FAILED
test executor_tests::test_javascript_add ... FAILED
test executor_tests::test_javascript_regexes ... FAILED

Thank you for this!

I've filed some tasks to address the things that we are currently equipped to handle. The task tree's root is here.

What else should the Abstract Wikipedia team do to follow up on this?

I've filed some tasks to address the things that we are currently equipped to handle. The task tree's root is here.

What else should the Abstract Wikipedia team do to follow up on this?

That task tree looks pretty good to me. Getting clippy and semgrep's Rust rules running in CI for the evaluator and function-schemata codebases would be great - I'm happy to help with that. Those tools encompass a lot of obvious bad Rust patterns (unsafe, raw pointers, etc.) And then I do think once we can deploy this to a more production-like environment, testing the wasm execution and Rust's concurrency would probably be a good idea, though I still think an external pentesting firm with solid experience evaluating Rust services would be our best strategy there.

I think we can close this. We've captured all of the actionable findings separately.

sbassett moved this task from In Progress to Our Part Is Done on the secscrum board.

I think we can close this. We've captured all of the actionable findings separately.

Ok, that's fine.