Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: Big Decimal DOS #6730

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
@GetMapping("/tonghuaroot-DOSDemo01")
@ResponseBody
// BAD:
public Long demo(@RequestParam(name = "num") BigDecimal num) {
Long startTime = System.currentTimeMillis();
BigDecimal num1 = new BigDecimal(0.005);
System.out.println(num1.add(num));
Long endTime = System.currentTimeMillis();
Long tempTime = (endTime - startTime);
System.out.println(tempTime);
return tempTime;
}

@GetMapping("/tonghuaroot-DOSDemo02")
@ResponseBody
// GOOD:
public Long demo02(@RequestParam(name = "num") String num) {
if (num.length() > 33 || num.matches("(?i)e")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be:

Suggested change
if (num.length() > 33 || num.matches("(?i)e")) {
if (num.length() > 33 || num.matches("(?i).*e.*")) {

(or similar)

return "Input Parameter is too long."
}
Long startTime = System.currentTimeMillis();
BigDecimal num1 = new BigDecimal(0.005);
System.out.println(num1.add(num));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no BigDecimal.add(String); you would probably have to convert num it to a BigDecimal first

Long endTime = System.currentTimeMillis();
Long tempTime = (endTime - startTime);
System.out.println(tempTime);
return tempTime;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>


<overview>
<p>Directly incorporating user input into an BigDecimal Operation Function without validating the input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use the term "method" instead of "function" since that is the commonly used term in the Java context.
(And also a few lines below the term "method" is used as well)

can facilitate DOS attacks. In these attacks, the server
will consume a lot of computing resources, A typical scenario is that the CPU usage rises to close to 100%.
tonghuaroot marked this conversation as resolved.
Show resolved Hide resolved
This issue often occurs in scenarios that require scientific computing, such as e-commerce platforms and electronic payments.
</p>

</overview>
<recommendation>

<p>To guard against BigDecimal DOS attacks, you should avoid putting user-provided input
directly into a BigDecimal Method(like: add(), subtract()). Instead, In some e-commerce payment scenarios,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
directly into a BigDecimal Method(like: add(), subtract()). Instead, In some e-commerce payment scenarios,
directly into a BigDecimal method (like: add(), subtract()). Instead, in some e-commerce payment scenarios,

Maybe it would also be worth formatting these methods as code using <code>...</code>?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Marcono1234 help review, I will to do some modify about this query rule's help file.

we should verify the size of the incoming number. For example, under normal circumstances,
the number representing money will not contain "e".</p>

</recommendation>
<example>

<p>The following example shows an BigDecimal parameter being used directly pass
tonghuaroot marked this conversation as resolved.
Show resolved Hide resolved
to BigDecimal Method(like: add(), subtract()) without validating the input, which facilitates DOS attacks.
tonghuaroot marked this conversation as resolved.
Show resolved Hide resolved
It also shows how to remedy the problem by validating the user input against a known fixed string.
</p>

<sample src="BigDecimalDOS.java" />

</example>
<references>
<li>
<a href="https://www.cuijianxiong.top/2021/03/30/%E6%9A%B4%E5%8A%9B%E7%9A%84%E6%94%BB%E5%87%BBDos/">暴力的攻击-Dos</a>
</li>
<li>
<a href="https://mp.weixin.qq.com/s/4YX7XA34fShdCpGawO-Q_Q">BigDecimal DOS</a>
Comment on lines +35 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add "(Chinese)" or a similar indicator to the link text; otherwise it might be confusing that the pages are in Chinese (?) because the query help text is in English.

Not sure if there are any good blog posts written in English, but maybe the following is relevant?
https://www.baeldung.com/javax-bigdecimal-validation

</li>

</references>
</qhelp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* @id java/BigDecimalDOS
* @name Java-BigDecimal-DOS-Vulnerability
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name does not have to use hyphens, see query metadata style guide

* @description When user-controllable data is pass to the relevant Methods in BigDecimal, it may cause DOS issues when computing resources are limited. This issue is common in business scenarios such as e-commerce platforms.
tonghuaroot marked this conversation as resolved.
Show resolved Hide resolved
* @kind path-problem
* @problem.severity error
*/

import java
import semmle.code.java.dataflow.FlowSources
import DataFlow::PathGraph

class BigDecimalDOSConfig extends TaintTracking::Configuration {
BigDecimalDOSConfig() { this = "Java-BigDecimal-DOS-Vulnerability-Config" }

override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }

override predicate isSink(DataFlow::Node sink) {
exists(Method method, MethodAccess call |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered adding other methods of the BigDecimal class that could be equally problematic, e.g. divide, multiply or pow?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @atorralba for helping review this part of the code.
Yes, I will do more tests and add these methods!

method.getDeclaringType().hasQualifiedName("java.math", "BigDecimal") and
method.hasName("subtract") and
call.getMethod() = method and
sink.asExpr() = call.getArgument(0)
)
or
exists(Method method, MethodAccess call |
method.getDeclaringType().hasQualifiedName("java.math", "BigDecimal") and
method.hasName("add") and
call.getMethod() = method and
sink.asExpr() = call.getArgument(0)
)
tonghuaroot marked this conversation as resolved.
Show resolved Hide resolved
}
}

from BigDecimalDOSConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select source.getNode(), source, sink, "Potential Java BigDecimal DOS Issue due to $@.",
tonghuaroot marked this conversation as resolved.
Show resolved Hide resolved
source.getNode(), "a user-provided value"
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
edges
| BigDecimalDOS.java:12:28:12:69 | num : BigDecimal | BigDecimalDOS.java:14:30:14:32 | num |
nodes
| BigDecimalDOS.java:12:28:12:69 | num : BigDecimal | semmle.label | num : BigDecimal |
| BigDecimalDOS.java:14:30:14:32 | num | semmle.label | num |
subpaths
#select
| BigDecimalDOS.java:12:28:12:69 | num | BigDecimalDOS.java:12:28:12:69 | num : BigDecimal | BigDecimalDOS.java:14:30:14:32 | num | Potential Java BigDecimal DOS Issue due to $@. | BigDecimalDOS.java:12:28:12:69 | num | a user-provided value |
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package com.example.servingwebcontent;

import org.springframework.stereotype.Controller;
import org.springframework.ui.Model;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;

import java.math.BigDecimal;
import java.math.MathContext;

@Controller
public class GreetingController {
@GetMapping("/tonghuaroot-DOSDemo01")
@ResponseBody
// BAD:
public Long demo(@RequestParam(name = "num") BigDecimal num) {
Long startTime = System.currentTimeMillis();
BigDecimal num1 = new BigDecimal(0.005);
System.out.println(num1.add(num));
Long endTime = System.currentTimeMillis();
Long tempTime = (endTime - startTime);
System.out.println(tempTime);
return tempTime;
}

@GetMapping("/tonghuaroot-DOSDemo02")
@ResponseBody
// GOOD:
public Long demo02(@RequestParam(name = "num") String num) {
if (num.length() > 33 || num.matches("(?i)e")) {
return "Input Parameter is too long."
}
Long startTime = System.currentTimeMillis();
BigDecimal num1 = new BigDecimal(0.005);
System.out.println(num1.add(num));
Long endTime = System.currentTimeMillis();
Long tempTime = (endTime - startTime);
System.out.println(tempTime);
return tempTime;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
experimental/Security/BigDecimalDOS/BigDecimalDOS.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/