APPSEC-2486 S4036: Improve description

rules/S4036/ask-yourself.adoc

@@ -1,5 +1,5 @@
 == Ask Yourself Whether
 
-* The directories in the PATH environment variable may be defined by not trusted entities. 
+* The PATH environment variable only contains fixed, trusted directories.
 
-There is a risk if you answered yes to this question.
+There is a risk if you answered no to this question.

rules/S4036/csharp/rule.adoc

@@ -6,6 +6,7 @@ include::../recommended.adoc[]
 
 == Sensitive Code Example
 
+[source,csharp]
 ----
 Process p = new Process();
 p.StartInfo.FileName = "binary"; // Sensitive
@@ -16,7 +17,7 @@ p.StartInfo.FileName = "binary"; // Sensitive
 [source,csharp]
 ----
 Process p = new Process();
-p.StartInfo.FileName = @"C:\Apps\binary.exe"; // Compliant
+p.StartInfo.FileName = @"C:\Apps\binary.exe";
 ----
 
 include::../see.adoc[]

rules/S4036/description.adoc

@@ -1 +1,22 @@
-When executing an OS command and unless you specify the full path to the executable, then the locations in your application's ``++PATH++`` environment variable will be searched for the executable. That search could leave an opening for an attacker if one of the elements in ``++PATH++`` is a directory under his control. 
+When you run an OS command, it is always important to protect yourself against
+the risk of accidental or malicious replacement of the executables in the
+production system.
+
+To do so, the methodology consists in verifying and being intentional about
+whether the executable that is going to be used **is actually** the one you
+expect to be used.
+
+For example, if you call ``++git++`` (without specifying a path), the operating
+system will search for the executable in the directories specified in the
+``++PATH++`` environment variable. +
+An attacker could have added, in a permissive directory covered by ``++PATH++``
+, another executable called ``++git++``, but with a completely different
+behavior, for example exfiltrating data or exploiting a vulnerability in your
+own code.
+
+However, if you call ``++/usr/bin/git++`` or ``++../git++`` (relative path)
+directly, the operating system will intentionally use the executable you intend
+to use. +
+Note that you still need to make sure that the executable is not world-writeable
+and potentially overwritten. This is not the scope of this rule.
+

rules/S4036/java/rule.adoc

@@ -22,16 +22,16 @@ The command is defined by its full path:
 
 [source,java]
 ----
-Runtime.getRuntime().exec("/usr/bin/make");  // Compliant
-Runtime.getRuntime().exec(new String[]{"~/bin/make"});  // Compliant
+Runtime.getRuntime().exec("/usr/bin/make");
+Runtime.getRuntime().exec(new String[]{"~/bin/make"});
 
-ProcessBuilder builder = new ProcessBuilder("./bin/make");  // Compliant
-builder.command("../bin/make");  // Compliant
-builder.command(Arrays.asList("..\bin\make", "-j8")); // Compliant
+ProcessBuilder builder = new ProcessBuilder("./bin/make");
+builder.command("../bin/make");
+builder.command(Arrays.asList("..\bin\make", "-j8"));
 
-builder = new ProcessBuilder(Arrays.asList(".\make"));  // Compliant
-builder.command(Arrays.asList("C:\bin\make", "-j8"));  // Compliant
-builder.command(Arrays.asList("\\SERVER\bin\make"));  // Compliant
+builder = new ProcessBuilder(Arrays.asList(".\make"));
+builder.command(Arrays.asList("C:\bin\make", "-j8"));
+builder.command(Arrays.asList("\\SERVER\bin\make"));
 ----
 
 include::../see.adoc[]

rules/S4036/javascript/rule.adoc

@@ -6,6 +6,7 @@ include::../recommended.adoc[]
 
 == Sensitive Code Example
 
+[source,javascript]
 ----
 const cp = require('child_process');
 cp.exec('file.exe'); // Sensitive
@@ -16,7 +17,7 @@ cp.exec('file.exe'); // Sensitive
 [source,javascript]
 ----
 const cp = require('child_process');
-cp.exec('/usr/bin/file.exe'); // Compliant
+cp.exec('/usr/bin/file.exe');
 ----
 
 include::../see.adoc[]

rules/S4036/message.adoc

@@ -1,4 +1,4 @@
 === Message
 
-Make sure the "PATH" used to find this command includes only what you intend.
+Make sure the "PATH" variable only contains fixed, unwriteable directories.
 

rules/S4036/php/rule.adoc

@@ -6,6 +6,7 @@ include::../recommended.adoc[]
 
 == Sensitive Code Example
 
+[source,php]
 ----
 $output = shell_exec("file"); // Sensitive
 ----
@@ -14,7 +15,7 @@ $output = shell_exec("file"); // Sensitive
 
 [source,php]
 ----
-$output = shell_exec("/usr/bin/file"); // Compliant
+$output = shell_exec("/usr/bin/file");
 ----
 
 include::../see.adoc[]

rules/S4036/python/rule.adoc

@@ -6,6 +6,7 @@ include::../recommended.adoc[]
 
 == Sensitive Code Example
 
+[source,python]
 ----
 params = ["binary", "arg"]
 subprocess.call(params)  # Sensitive
@@ -16,7 +17,7 @@ subprocess.call(params)  # Sensitive
 [source,python]
 ----
 params = ["/usr/bin/binary", "arg"]
-subprocess.call(params)  # Compliant
+subprocess.call(params)
 ----
 
 include::../see.adoc[]

rules/S4036/recommended.adoc

@@ -1,3 +1,25 @@
 == Recommended Secure Coding Practices
 
-Fully qualified/absolute path should be used to specify the OS command to execute.
+If you wish to rely on the ``++PATH++`` environment variable to locate the OS
+command, make sure that each of its listed directories is fixed, not susceptible
+to change, and not writable by unprivileged users.
+
+If you determine that these folders cannot be altered, and that you are sure
+that the git program you intended to use will be used, then you can determine
+that these risks are under your control.
+
+A good practice you can use is to also hardcode the ``++PATH++`` variable you
+want to use, if you can do so in the framework you use.
+
+However, if these steps are too long to do, or business logic of your
+organization blocks you from following them, then consider using the absolute
+path of the command instead.
+
+[source,bash]
+----
+$ whereis git
+git: /usr/bin/git /usr/share/man/man1/git.1.gz
+$ ls -l /usr/bin/git
+-rwxr-xr-x 1 root root 3376112 Jan 28 10:13 /usr/bin/git
+----
+

rules/S4036/vbnet/rule.adoc

@@ -6,6 +6,7 @@ include::../recommended.adoc[]
 
 == Sensitive Code Example
 
+[source,vbnet]
 ----
 Dim p As New Process()
 p.StartInfo.FileName = "binary" ' Sensitive
@@ -16,7 +17,7 @@ p.StartInfo.FileName = "binary" ' Sensitive
 [source,vbnet]
 ----
 Dim p As New Process()
-p.StartInfo.FileName = "C:\Apps\binary.exe" ' Compliant
+p.StartInfo.FileName = "C:\Apps\binary.exe"
 ----
 
 include::../see.adoc[]