ishangsf/rspec
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Modify S4684: Change text to education framework format (APPSEC-1104) (#3149)

Browse Source
This commit is contained in:
gaetan-ferry-sonarsource 2023-09-28 14:44:41 +02:00 committed by GitHub
parent 828d958459
commit 8ab8c5bf10
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 61 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
rules/S4684/java/rule.adoc
View File

@ -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? == 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.
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. === 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.
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. Depending on the affected objects and properties, the consequences can vary.
==== Privilege escalation
For this reason, using ``++@Entity++`` or ``++@Document++`` objects as arguments of methods annotated with ``++@RequestMapping++`` should be avoided. 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
In addition to ``++@RequestMapping++``, this rule also considers the annotations introduced in Spring Framework 4.3: ``++@GetMapping++``, ``++@PostMapping++``, ``++@PutMapping++``, ``++@DeleteMapping++``, ``++@PatchMapping++``. 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.
=== Noncompliant code example == How to fix it in Java EE
[source,java] === Code examples
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.
==== Noncompliant code example
[source,java,diff-id=1,diff-type=noncompliant]
---- ----
import javax.persistence.Entity; import javax.persistence.Entity;
@ -38,24 +66,18 @@ import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMapping;
@Controller @Controller
public class WishListController { public class PurchaseOrderController {
@PostMapping(path = "/saveForLater")
public String saveForLater(Wish wish) {
session.save(wish);
}
@RequestMapping(path = "/saveForLater", method = RequestMethod.POST) @RequestMapping(path = "/saveForLater", method = RequestMethod.POST)
public String saveForLater(Wish wish) { public String saveForLater(Wish wish) { // Noncompliant
session.save(wish); session.save(wish);
} }
} }
---- ----
==== Compliant solution
=== Compliant solution [source,java,diff-id=1,diff-type=compliant]
[source,java]
---- ----
public class WishDTO { public class WishDTO {
Long productId; Long productId;
@ -69,36 +91,40 @@ import org.springframework.web.bind.annotation.RequestMapping;
@Controller @Controller
public class PurchaseOrderController { 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) @RequestMapping(path = "/saveForLater", method = RequestMethod.POST)
public String saveForLater(WishDTO wish) { public String saveForLater(WishDTO wish) {
Wish persistentWish = new 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); 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
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. 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.
== Resources == Resources
* https://owasp.org/Top10/A08_2021-Software_and_Data_Integrity_Failures/[OWASP Top 10 2021 Category A8] - Software and Data Integrity Failures === Documentation
* 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 * OWASP - https://cheatsheetseries.owasp.org/cheatsheets/Mass_Assignment_Cheat_Sheet.html[Mass Assignment Cheat Sheet]
* 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]
=== 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
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[] ifdef::env-github,rspecator-view[]