From 8ab8c5bf10cbb5a2399ed9af69a68801e3ea9794 Mon Sep 17 00:00:00 2001 From: gaetan-ferry-sonarsource <112399173+gaetan-ferry-sonarsource@users.noreply.github.com> Date: Thu, 28 Sep 2023 14:44:41 +0200 Subject: [PATCH] Modify S4684: Change text to education framework format (APPSEC-1104) (#3149) --- rules/S4684/java/rule.adoc | 94 ++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 34 deletions(-) diff --git a/rules/S4684/java/rule.adoc b/rules/S4684/java/rule.adoc index f2e2ad16a6a..79bbe2ba41b 100644 --- a/rules/S4684/java/rule.adoc +++ b/rules/S4684/java/rule.adoc @@ -1,22 +1,50 @@ +With Spring, when a request mapping method is configured to accept bean objects +as arguments, the framework will automatically bind HTTP parameters to those +objects' properties. If the targeted beans are also persistent entities, the +framework will also store those properties in the storage backend, usually the +application's database. + == Why is this an issue? -On one side, Spring MVC automatically bind request parameters to beans declared as arguments of methods annotated with ``++@RequestMapping++``. Because of this automatic binding feature, it's possible to feed some unexpected fields on the arguments of the ``++@RequestMapping++`` annotated methods. +By accepting persistent entities as method arguments, the application allows +clients to manipulate the object's properties directly. + +=== What is the potential impact? + +Attackers could forge malicious HTTP requests that will alter unexpected +properties of persistent objects. This can lead to unauthorized modifications of +the entity's state. This is known as a *mass assignment* attack. -On the other end, persistent objects (``++@Entity++`` or ``++@Document++``) are linked to the underlying database and updated automatically by a persistence framework, such as Hibernate, JPA or Spring Data MongoDB. +Depending on the affected objects and properties, the consequences can vary. +==== Privilege escalation -These two facts combined together can lead to malicious attack: if a persistent object is used as an argument of a method annotated with ``++@RequestMapping++``, it's possible from a specially crafted user input, to change the content of unexpected fields into the database. +If the affected object is used to store the client's identity or permissions, +the attacker could alter it to change their entitlement on the application. This +can lead to horizontal or vertical privilege escalation. +==== Security checks bypass -For this reason, using ``++@Entity++`` or ``++@Document++`` objects as arguments of methods annotated with ``++@RequestMapping++`` should be avoided. +Because persistent objects are modified directly without prior logic, attackers +could exploit this issue to bypass security measures otherwise enforced by the +application. For example, an attacker might be able to change their e-mail +address to an invalid one by directly setting it without going through the +application's email validation process. +The same could also apply to passwords that attackers could change without +complexity validation or knowledge of their current value. -In addition to ``++@RequestMapping++``, this rule also considers the annotations introduced in Spring Framework 4.3: ``++@GetMapping++``, ``++@PostMapping++``, ``++@PutMapping++``, ``++@DeleteMapping++``, ``++@PatchMapping++``. +== How to fix it in Java EE +=== Code examples -=== Noncompliant code example +The following code is vulnerable to a mass assignment attack because it allows +modifying the `User` persistent entities thanks to maliciously forged `Wish` +object properties. -[source,java] +==== Noncompliant code example + +[source,java,diff-id=1,diff-type=noncompliant] ---- import javax.persistence.Entity; @@ -38,24 +66,18 @@ import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.RequestMapping; @Controller -public class WishListController { - - @PostMapping(path = "/saveForLater") - public String saveForLater(Wish wish) { - session.save(wish); - } +public class PurchaseOrderController { @RequestMapping(path = "/saveForLater", method = RequestMethod.POST) - public String saveForLater(Wish wish) { + public String saveForLater(Wish wish) { // Noncompliant session.save(wish); } } ---- +==== Compliant solution -=== Compliant solution - -[source,java] +[source,java,diff-id=1,diff-type=compliant] ---- public class WishDTO { Long productId; @@ -69,36 +91,40 @@ import org.springframework.web.bind.annotation.RequestMapping; @Controller public class PurchaseOrderController { - @PostMapping(path = "/saveForLater") - public String saveForLater(WishDTO wish) { - Wish persistentWish = new Wish(); - // do the mapping between "wish" and "persistentWish" - [...] - session.save(persistentWish); - } - @RequestMapping(path = "/saveForLater", method = RequestMethod.POST) public String saveForLater(WishDTO wish) { Wish persistentWish = new Wish(); - // do the mapping between "wish" and "persistentWish" - [...] + persistentWish.productId = wish.productId + persistentWish.quantity = wish.quantity + persistentWish.client = getClientById(with.clientId) session.save(persistentWish); } } ---- +=== How does this work? -=== Exceptions +The compliant code implements a Data Transfer Object (DTO) layer. Instead of +accepting a persistent `Wish` entity as a parameter, the vulnerable method +accepts a `WishDTO` object with a safe, minimal set of properties. It then +instantiates a persistent entity and initializes it based on the DTO properties' +values. The resulting object can safely be persisted in the database. -No issue is reported when the parameter is annotated with ``++@PathVariable++`` from Spring Framework, since the lookup will be done via id, the object cannot be forged on client side. +== Resources +=== Documentation -== Resources +* OWASP - https://cheatsheetseries.owasp.org/cheatsheets/Mass_Assignment_Cheat_Sheet.html[Mass Assignment Cheat Sheet] + +=== Standards + +* OWASP - https://owasp.org/Top10/A08_2021-Software_and_Data_Integrity_Failures/[Top 10 2021 - Category A8 - Software and Data Integrity Failures] +* OWASP - https://owasp.org/www-project-top-ten/2017/A5_2017-Broken_Access_Control[Top 10 2017 - Category A5 - Broken Access Control] +* CWE - https://cwe.mitre.org/data/definitions/915[CWE-915 - Improperly Controlled Modification of Dynamically-Determined Object Attributes] + +=== Articles & blog posts -* https://owasp.org/Top10/A08_2021-Software_and_Data_Integrity_Failures/[OWASP Top 10 2021 Category A8] - Software and Data Integrity Failures -* https://owasp.org/www-project-top-ten/2017/A5_2017-Broken_Access_Control[OWASP Top 10 2017 Category A5] - Broken Access Control -* https://cwe.mitre.org/data/definitions/915[MITRE, CWE-915] - Improperly Controlled Modification of Dynamically-Determined Object Attributes -* https://o2platform.files.wordpress.com/2011/07/ounce_springframework_vulnerabilities.pdf[Two Security Vulnerabilities in the Spring Framework’s MVC by Ryan Berg and Dinis Cruz] +OWASP O2 Platform Blog - https://o2platform.files.wordpress.com/2011/07/ounce_springframework_vulnerabilities.pdf[Two Security Vulnerabilities in the Spring Framework's MVC] ifdef::env-github,rspecator-view[]