Skip to content

Security: Fix command injection in NFS mount check and CD burn status#514

Merged
Narflex merged 1 commit intogoogle:masterfrom
r3352:fix/command-injection-properties
Apr 17, 2026
Merged

Security: Fix command injection in NFS mount check and CD burn status#514
Narflex merged 1 commit intogoogle:masterfrom
r3352:fix/command-injection-properties

Conversation

@r3352
Copy link
Copy Markdown
Contributor

@r3352 r3352 commented Apr 6, 2026

Summary

  • Two code paths pass configuration property values into sh -c shell commands, enabling command injection via SetServerProperty API
  • Replaces shell-based execution with safe alternatives that don't interpret shell metacharacters

Vulnerability Details

CWE-78 (OS Command Injection)

Two code paths use exec2(new String[]{"sh", "-c", ...}) with unsanitized property values concatenated into the command string:

  1. IOUtils.java:1147 — NFS mount check: "mount -t nfs | grep -i \"" + localPath + "\"" — the localPath from linux/nfs_mounts property can escape the double quotes
  2. MediaFile.java:3688 — CD burn status: burnPath + " -toc dev=" + devPath — both linux/cd_burn and default_burner_device properties are concatenated directly

Since SetServerProperty (Configuration.java:831) allows any authenticated client to set any property with no validation, an attacker can inject shell metacharacters (;, $()) into these properties to execute arbitrary commands.

Note: Other code paths in LinuxUtils.java (gateway, SSID, WiFi key) use exec2(String) which calls Runtime.exec(String) — this tokenizes via StringTokenizer without invoking a shell, so those are NOT vulnerable to shell metacharacter injection.

Changes

  • IOUtils.java: Replaced sh -c "mount | grep" with running mount -t nfs via array exec and performing the case-insensitive match in Java. Eliminates the shell entirely.
  • MediaFile.java: Replaced sh -c concatenation with the String[] array form of exec2(), which passes arguments directly to the OS without shell interpretation.

@google-cla
Copy link
Copy Markdown

google-cla bot commented Apr 6, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@r3352 r3352 force-pushed the fix/command-injection-properties branch from 12964a2 to de604f3 Compare April 6, 2026 00:48
@Narflex
Copy link
Copy Markdown
Collaborator

Narflex commented Apr 16, 2026

This one looks fine. :) Although these code paths are never executed AFAIK, they were from things we never actually released (but they were functional, so there may be some way to enable them).

@r3352
Copy link
Copy Markdown
Contributor Author

r3352 commented Apr 16, 2026

Thanks! And good context — I noticed those code paths seemed like they might be dormant. Even if they're not actively triggered today, the fix is cheap insurance in case someone does find a way to enable them (or if they get wired up in a future change). Appreciate you reviewing these.

@Narflex
Copy link
Copy Markdown
Collaborator

Narflex commented Apr 16, 2026

In thinking about this more...I may have used 'sh' to execute these because of path issues.

Do you have a Linux version running in a desktop environment where you can check to see if simply ExecuteProcess("mount") (or whatever the right syntax is) actually works?

@r3352
Copy link
Copy Markdown
Contributor Author

r3352 commented Apr 17, 2026

I actually spun this up on a Linux box to check. Both Runtime.exec(new String[]{"mount", "-t", "nfs"}) and Runtime.exec(new String[]{"sh", "-c", "mount -t nfs"}) produce identical results. Java uses execvp() under the hood so it searches PATH the same way a shell would. mount resolved to /usr/bin/mount without any issues. So the array form should be a safe drop-in replacement without PATH concerns.

@Narflex Narflex merged commit 981adfa into google:master Apr 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants