diff --git a/rules/S4036/ask-yourself.adoc b/rules/S4036/ask-yourself.adoc index 5c53cee5dfb..bb35b5e6831 100644 --- a/rules/S4036/ask-yourself.adoc +++ b/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. diff --git a/rules/S4036/csharp/rule.adoc b/rules/S4036/csharp/rule.adoc index 194b7f562cc..cb065045029 100644 --- a/rules/S4036/csharp/rule.adoc +++ b/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[] diff --git a/rules/S4036/description.adoc b/rules/S4036/description.adoc index 7a079dbd019..ed5e402fdc2 100644 --- a/rules/S4036/description.adoc +++ b/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. + diff --git a/rules/S4036/java/rule.adoc b/rules/S4036/java/rule.adoc index a8c38d6031e..d0aaea807a9 100644 --- a/rules/S4036/java/rule.adoc +++ b/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[] diff --git a/rules/S4036/javascript/rule.adoc b/rules/S4036/javascript/rule.adoc index 44d65004a57..506adb4d855 100644 --- a/rules/S4036/javascript/rule.adoc +++ b/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[] diff --git a/rules/S4036/message.adoc b/rules/S4036/message.adoc index 9a7aaffc5a9..06e02750c02 100644 --- a/rules/S4036/message.adoc +++ b/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. diff --git a/rules/S4036/php/rule.adoc b/rules/S4036/php/rule.adoc index 7971246beec..9b1126c2d16 100644 --- a/rules/S4036/php/rule.adoc +++ b/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[] diff --git a/rules/S4036/python/rule.adoc b/rules/S4036/python/rule.adoc index 68c74a27098..44cef1d285a 100644 --- a/rules/S4036/python/rule.adoc +++ b/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[] diff --git a/rules/S4036/recommended.adoc b/rules/S4036/recommended.adoc index 295bb60de55..b7f5370e95d 100644 --- a/rules/S4036/recommended.adoc +++ b/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 +---- + diff --git a/rules/S4036/vbnet/rule.adoc b/rules/S4036/vbnet/rule.adoc index e0b0cc83b37..ec6fb0e5b11 100644 --- a/rules/S4036/vbnet/rule.adoc +++ b/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[]