openml / openml/OpenML

Security: sanitize/replace shell invocations to avoid injection

Open
#1,265 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
755
Forks
128
PR merge metrics
No merged PRs in 30d

Description

Potential command injection risk in shell invocations that include user-controlled values without shell-escaping.

Examples

  • openml_OS/helpers/api_helper.php validate_arff(): builds a sed command using $name and $did to prepend info to an ARFF file:

    $info = '% Data set "'.$name.'". ... d/'.$did;
    $string = '1s/^/'.$info.'\n/';
    $command2 = "sed -i -e '$string' $newUrl";
    exec(CMD_PREFIX . $command2, $res, $code);
    

    $name originates from dataset metadata and may contain characters that break quoting. This should avoid shell entirely.

  • openml_OS/controllers/Api_splits.php: multiple system()/exec() calls build Java commands with request-derived inputs. Some checks (is_safe, is_numeric) exist, but defense-in-depth suggests escaping args or using proc_open with argv arrays.

Proposed fix

  • Replace the sed call with pure PHP file I/O to prepend the info line.
  • When shelling out to Java, strictly validate and/or escape all args (e.g., via escapeshellarg) and prefer argv arrays.
  • Add unit tests covering edge-case names.

Acceptance criteria

  • No use of unescaped user-provided values in shell commands.
  • Prepend operation implemented without shell invocation.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with openml_OS/helpers/api_helper.php, especially validate_arff(), and inspect the command construction shown in the issue. Then review shell calls in openml_OS/controllers/Api_splits.php and the existing is_safe/is_numeric checks. Done means the prepend operation avoids shell invocation, all user-provided command values are protected, and unit tests cover edge-case names.

Written by the indexing model from the issue text.

Assessment

Tech stack
java, php
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.