From a0abb99f762169a91de6d1afb0dc838204ff32ab Mon Sep 17 00:00:00 2001 From: Loris S <91723853+loris-s-sonarsource@users.noreply.github.com> Date: Fri, 18 Aug 2023 11:31:06 +0200 Subject: [PATCH] Modify S2596(javascript): Convert to LayC (#2901) This PR also removes the java folder because it is not implemented and has no implementation plan. This PR was made spontaneously during Daniel's onboarding. --------- Co-authored-by: daniel-teuchert-sonarsource <141642369+daniel-teuchert-sonarsource@users.noreply.github.com> --- .../header_names/allowed_framework_names.adoc | 2 + rules/S2598/comments-and-links.adoc | 11 --- rules/S2598/common/fix/allowed-folder.adoc | 16 ++++ rules/S2598/description.adoc | 6 -- rules/S2598/impact.adoc | 43 +++++++++ rules/S2598/java/metadata.json | 30 ------- rules/S2598/java/rule.adoc | 36 -------- .../javascript/how-to-fix-it/formidable.adoc | 29 ++++++ .../javascript/how-to-fix-it/multer.adoc | 50 +++++++++++ rules/S2598/javascript/rule.adoc | 88 +++++-------------- rules/S2598/message.adoc | 4 - rules/S2598/metadata.json | 4 + rules/S2598/rationale.adoc | 8 ++ rules/S2598/rule.adoc | 5 -- rules/S2598/see.adoc | 7 -- 15 files changed, 175 insertions(+), 164 deletions(-) delete mode 100644 rules/S2598/comments-and-links.adoc create mode 100644 rules/S2598/common/fix/allowed-folder.adoc delete mode 100644 rules/S2598/description.adoc create mode 100644 rules/S2598/impact.adoc delete mode 100644 rules/S2598/java/metadata.json delete mode 100644 rules/S2598/java/rule.adoc create mode 100644 rules/S2598/javascript/how-to-fix-it/formidable.adoc create mode 100644 rules/S2598/javascript/how-to-fix-it/multer.adoc delete mode 100644 rules/S2598/message.adoc create mode 100644 rules/S2598/rationale.adoc delete mode 100644 rules/S2598/rule.adoc delete mode 100644 rules/S2598/see.adoc diff --git a/docs/header_names/allowed_framework_names.adoc b/docs/header_names/allowed_framework_names.adoc index e5ebb9458e..464f31266c 100644 --- a/docs/header_names/allowed_framework_names.adoc +++ b/docs/header_names/allowed_framework_names.adoc @@ -54,6 +54,8 @@ * DOM API * jsonwebtoken * libxmljs +* Formidable +* Multer // PHP * Core PHP * Guzzle diff --git a/rules/S2598/comments-and-links.adoc b/rules/S2598/comments-and-links.adoc deleted file mode 100644 index 8eeae09b6c..0000000000 --- a/rules/S2598/comments-and-links.adoc +++ /dev/null @@ -1,11 +0,0 @@ -=== on 21 Jan 2021, 15:37:26 Pierre-Loup Tristant wrote: -This rule is likely not implementable for C#. - -ASP.NET Core is not providing any high level interface to help developper manage uploaded files. - -There is no temporary storage of uploaded file by default. The file stays in memory and it's up to the developper to chose the end location. - -Verifying file extention can be done in many different ways. - - - diff --git a/rules/S2598/common/fix/allowed-folder.adoc b/rules/S2598/common/fix/allowed-folder.adoc new file mode 100644 index 0000000000..0de5e702f9 --- /dev/null +++ b/rules/S2598/common/fix/allowed-folder.adoc @@ -0,0 +1,16 @@ +==== Use pre-approved folders + +Create a special folder where untrusted data should be stored. This folder +should be classified as untrusted and have the following characteristics: + +* It should have specific read and write permissions that belong to the right people or organizations. +* It should have a size limit or its size should be monitored. +* It should contain backup copies if it contains data that belongs to users. + +This folder should not be located in `/tmp`, `/var/tmp` or in the Windows +directory `%TEMP%`. + +These folders are usually "world-writable", can be manipulated, and can be +accidentally deleted by the system. + +Also, the original file names and extensions should be changed to controlled +strings to prevent unwanted code from being executed based on the file names. diff --git a/rules/S2598/description.adoc b/rules/S2598/description.adoc deleted file mode 100644 index 2687462538..0000000000 --- a/rules/S2598/description.adoc +++ /dev/null @@ -1,6 +0,0 @@ -These minimum restrictions should be applied when handling file uploads: - -* the file upload folder to restrict untrusted files to a specific folder. -* the file extension of the uploaded file to prevent remote code execution. - -Also the size of the uploaded file should be limited to prevent denial of service attacks. This requirement is covered by the rule S5693. diff --git a/rules/S2598/impact.adoc b/rules/S2598/impact.adoc new file mode 100644 index 0000000000..d4b82fd510 --- /dev/null +++ b/rules/S2598/impact.adoc @@ -0,0 +1,43 @@ + +=== What is the potential impact? + +After discovering this vulnerability, attackers may attempt to upload as many +different file types as possible, such as javascript files, bash scripts, +malware, or malicious configuration files targeting potential processes. + +Below are some real-world scenarios that illustrate the potential impact of an +attacker exploiting the vulnerability. + +==== Full application compromise + +In the worst-case scenario, the attackers succeed in uploading a file recognized +by in an internal tool, triggering code execution. + +Depending on the attacker, code execution can be used with different +intentions: + +* Download the internal server's data, most likely to sell it. +* Modify data, install malware, for instance, malware that mines cryptocurrencies. +* Stop services or exhaust resources, for instance, with fork bombs. + +==== Server Resource Exhaustion + +By repeatedly uploading large files, an attacker can consume excessive server +resources, resulting in a denial of service. + +If the component affected by this vulnerability is not a bottleneck that acts +as a single point of failure (SPOF) within the application, the denial of +service can only affect the attacker who caused it. + +Even though a denial of service might have little direct impact, it can have +secondary impact in architectures that use containers and container +orchestrators. For example, it can cause unexpected container failures or +overuse of resources. + +In some cases, it is also possible to force the product to "fail open" when +resources are exhausted, which means that some security features are disabled +in an emergency. + +These threats are particularly insidious if the attacked organization does not +maintain a disaster recovery plan (DRP). + diff --git a/rules/S2598/java/metadata.json b/rules/S2598/java/metadata.json deleted file mode 100644 index 9e1c5e4a9f..0000000000 --- a/rules/S2598/java/metadata.json +++ /dev/null @@ -1,30 +0,0 @@ -{ - "tags": [ - "cwe", - "cert" - ], - "securityStandards": { - "CERT": [ - "IDS56-J." - ], - "CWE": [ - 434, - 400 - ], - "OWASP Top 10 2021": [ - "A4" - ], - "PCI DSS 3.2": [ - "6.5.8" - ], - "PCI DSS 4.0": [ - "6.2.4" - ], - "ASVS 4.0": [ - "12.1.1", - "12.2.1", - "12.5.2", - "13.1.5" - ] - } -} diff --git a/rules/S2598/java/rule.adoc b/rules/S2598/java/rule.adoc deleted file mode 100644 index 667e12988d..0000000000 --- a/rules/S2598/java/rule.adoc +++ /dev/null @@ -1,36 +0,0 @@ -== Why is this an issue? - -include::../description.adoc[] - -=== Noncompliant code example - -[source,java] ----- -public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException { - - if (ServletFileUpload.isMultipartContent(request)) { - - FileItemFactory factory = new DiskFileItemFactory(); - ServletFileUpload upload = new ServletFileUpload(factory); // Noncompliant - // ... ----- - -include::../see.adoc[] - -* https://wiki.sei.cmu.edu/confluence/display/java/IDS56-J.+Prevent+arbitrary+file+upload[CERT, IDS56-J.] - Prevent arbitrary file upload - -ifdef::env-github,rspecator-view[] - -''' -== Implementation Specification -(visible only on this page) - -include::../message.adoc[] - -''' -== Comments And Links -(visible only on this page) - -include::../comments-and-links.adoc[] - -endif::env-github,rspecator-view[] diff --git a/rules/S2598/javascript/how-to-fix-it/formidable.adoc b/rules/S2598/javascript/how-to-fix-it/formidable.adoc new file mode 100644 index 0000000000..75b9e26e7b --- /dev/null +++ b/rules/S2598/javascript/how-to-fix-it/formidable.adoc @@ -0,0 +1,29 @@ +== How to fix it in Formidable + +=== Code examples + +==== Noncompliant code example + +[source,javascript,diff-id=1,diff-type=noncompliant] +---- +const Formidable = require('formidable'); + +const form = new Formidable(); // Noncompliant +form.uploadDir = "/tmp/"; +form.keepExtensions = true; +---- + +==== Compliant solution + +[source,javascript,diff-id=1,diff-type=compliant] +---- +const Formidable = require('formidable'); + +const form = new Formidable(); +form.uploadDir = "/uploads/"; +form.keepExtensions = false; +---- + +=== How does this work? + +include::../../common/fix/allowed-folder.adoc[] diff --git a/rules/S2598/javascript/how-to-fix-it/multer.adoc b/rules/S2598/javascript/how-to-fix-it/multer.adoc new file mode 100644 index 0000000000..81a520754d --- /dev/null +++ b/rules/S2598/javascript/how-to-fix-it/multer.adoc @@ -0,0 +1,50 @@ +== How to fix it in Multer + +=== Code examples + +==== Noncompliant code example + +The following code sample is vulnerable because it implicitly uses `/tmp` or +`/var/tmp` as upload directory. + +[source,javascript,diff-id=2,diff-type=noncompliant] +---- +const crypto = require('node:crypto'); +const multer = require('multer'); + +let diskStorage = multer.diskStorage({ + filename: (req, file, cb) => { + const buf = crypto.randomBytes(20); + cb(null, buf.toString('hex')) + } +}); // Noncompliant + +let diskUpload = multer({ + storage: diskStorage, +}); +---- + +==== Compliant code example + +[source,javascript,diff-id=2,diff-type=compliant] +---- +const multer = require('multer'); + +let diskStorage = multer.diskStorage({ + filename: (req, file, cb) => { + const buf = crypto.randomBytes(20); + cb(null, buf.toString('hex')) + }, + destination: (req, file, cb) => { + cb(null, '/uploads/') + } +}); + +let diskUpload = multer({ + storage: diskStorage, +}); +---- + +=== How does this work? + +include::../../common/fix/allowed-folder.adoc[] diff --git a/rules/S2598/javascript/rule.adoc b/rules/S2598/javascript/rule.adoc index 862223f9ee..3c6f5ee122 100644 --- a/rules/S2598/javascript/rule.adoc +++ b/rules/S2598/javascript/rule.adoc @@ -1,74 +1,22 @@ + + == Why is this an issue? -include::../description.adoc[] +include::../rationale.adoc[] -=== Noncompliant code example +include::../impact.adoc[] -https://www.npmjs.com/package/formidable[formidable] module: +include::how-to-fix-it/formidable.adoc[] -[source,javascript] ----- -const Formidable = require('formidable'); +include::how-to-fix-it/multer.adoc[] -const form = new Formidable(); // Noncompliant, this form is not safe -form.uploadDir = ""; // because upload dir is not defined (by default os temp dir: /var/tmp or /tmp) -form.keepExtensions = true; // and file extensions are kept ----- +== Resources -https://www.npmjs.com/package/multer[multer] (Express.js middleware) module: +* https://owasp.org/Top10/A04_2021-Insecure_Design/[OWASP Top 10 2021 Category A4] - Insecure Design +* https://cwe.mitre.org/data/definitions/434[MITRE, CWE-434] - Unrestricted Upload of File with Dangerous Type +* https://cwe.mitre.org/data/definitions/400[MITRE, CWE-400] - Uncontrolled Resource Consumption +* https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload[OWASP Unrestricted File Upload] - Unrestricted File Upload -[source,javascript] ----- -const multer = require('multer'); - -let diskStorage = multer.diskStorage({ // Noncompliant: no destination specified - filename: (req, file, cb) => { - const buf = crypto.randomBytes(20); - cb(null, buf.toString('hex')) - } -}); - -// This upload is not safe as no destination specified, /var/tmp or /tmp will be used -let diskupload = multer({ - storage: diskStorage, -}); ----- - -=== Compliant solution - -https://www.npmjs.com/package/formidable[formidable] module: - -[source,javascript] ----- -const Formidable = require('formidable'); - -const form = new Formidable(); // Compliant -form.uploadDir = "./uploads/"; -form.keepExtensions = false; ----- - -https://www.npmjs.com/package/multer[multer] (Express.js middleware) module: - -[source,javascript] ----- -const multer = require('multer'); - -let diskStorage = multer.diskStorage({ // Compliant - filename: (req, file, cb) => { - const buf = crypto.randomBytes(20); - cb(null, buf.toString('hex')) - }, - destination: (req, file, cb) => { - cb(null, './uploads/') - } -}); - -let diskupload = multer({ - storage: diskStorage, -}); ----- - -include::../see.adoc[] ifdef::env-github,rspecator-view[] @@ -76,12 +24,22 @@ ifdef::env-github,rspecator-view[] == Implementation Specification (visible only on this page) -include::../message.adoc[] +=== Message + +Restrict [the extension|folder destination] of uploaded files. ''' == Comments And Links (visible only on this page) -include::../comments-and-links.adoc[] +=== on 21 Jan 2021, 15:37:26 Pierre-Loup Tristant wrote: +This rule is likely not implementable for C#. ASP.NET Core is not providing +any high level interface to help developper manage uploaded files. + +There is no temporary storage of uploaded file by default. The file stays in +memory and it's up to the developper to chose the end location. + +Verifying file extention can be done in many different ways. + endif::env-github,rspecator-view[] diff --git a/rules/S2598/message.adoc b/rules/S2598/message.adoc deleted file mode 100644 index e19e434c09..0000000000 --- a/rules/S2598/message.adoc +++ /dev/null @@ -1,4 +0,0 @@ -=== Message - -Restrict [the extension|folder destination] of uploaded files. - diff --git a/rules/S2598/metadata.json b/rules/S2598/metadata.json index 32668cd5cd..196a81cdd0 100644 --- a/rules/S2598/metadata.json +++ b/rules/S2598/metadata.json @@ -51,5 +51,9 @@ "defaultQualityProfiles": [ "Sonar way" ], + "educationPrinciples": [ + "defense_in_depth", + "never_trust_user_input" + ], "quickfix": "unknown" } diff --git a/rules/S2598/rationale.adoc b/rules/S2598/rationale.adoc new file mode 100644 index 0000000000..42c352ae69 --- /dev/null +++ b/rules/S2598/rationale.adoc @@ -0,0 +1,8 @@ +If the file upload feature is implemented without proper folder restriction, it +will result in an implicit trust violation within the server, as trusted files +will be implicitly stored alongside third-party files that should be considered +untrusted. + +This can allow an attacker to disrupt the security of an internal server +process or the running application. + diff --git a/rules/S2598/rule.adoc b/rules/S2598/rule.adoc deleted file mode 100644 index 7a31714c71..0000000000 --- a/rules/S2598/rule.adoc +++ /dev/null @@ -1,5 +0,0 @@ -== Why is this an issue? - -include::description.adoc[] - -include::see.adoc[] diff --git a/rules/S2598/see.adoc b/rules/S2598/see.adoc deleted file mode 100644 index 8d94d3e59d..0000000000 --- a/rules/S2598/see.adoc +++ /dev/null @@ -1,7 +0,0 @@ -== Resources - -* https://owasp.org/Top10/A04_2021-Insecure_Design/[OWASP Top 10 2021 Category A4] - Insecure Design -* https://cwe.mitre.org/data/definitions/434[MITRE, CWE-434] - Unrestricted Upload of File with Dangerous Type -* https://cwe.mitre.org/data/definitions/400[MITRE, CWE-400] - Uncontrolled Resource Consumption -* https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload[OWASP Unrestricted File Upload] - Unrestricted File Upload -* https://www.sans.org/top25-software-errors/#cat1[SANS Top 25] - Insecure Interaction Between Components