Security: Fix command injection in NFS mount check and CD burn status#514
Conversation
|
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. |
12964a2 to
de604f3
Compare
de604f3 to
c95ece6
Compare
|
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). |
|
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. |
|
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? |
|
I actually spun this up on a Linux box to check. Both |
Summary
sh -cshell commands, enabling command injection viaSetServerPropertyAPIVulnerability Details
CWE-78 (OS Command Injection)
Two code paths use
exec2(new String[]{"sh", "-c", ...})with unsanitized property values concatenated into the command string:IOUtils.java:1147— NFS mount check:"mount -t nfs | grep -i \"" + localPath + "\""— thelocalPathfromlinux/nfs_mountsproperty can escape the double quotesMediaFile.java:3688— CD burn status:burnPath + " -toc dev=" + devPath— bothlinux/cd_burnanddefault_burner_deviceproperties are concatenated directlySince
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) useexec2(String)which callsRuntime.exec(String)— this tokenizes viaStringTokenizerwithout invoking a shell, so those are NOT vulnerable to shell metacharacter injection.Changes
IOUtils.java: Replacedsh -c "mount | grep"with runningmount -t nfsvia array exec and performing the case-insensitive match in Java. Eliminates the shell entirely.MediaFile.java: Replacedsh -cconcatenation with theString[]array form ofexec2(), which passes arguments directly to the OS without shell interpretation.