-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Java: Big Decimal DOS #6730
Conversation
java/ql/src/experimental/Security/BigDecimalDOS/BigDecimalDOS.ql
Outdated
Show resolved
Hide resolved
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } | ||
|
||
override predicate isSink(DataFlow::Node sink) { | ||
exists(Method method, MethodAccess call | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a member of this project, so feel free to consider these comments only as suggestion, but maybe they are helpful nontheless.
java/ql/src/experimental/Security/BigDecimalDOS/BigDecimalDOS.qhelp
Outdated
Show resolved
Hide resolved
|
||
|
||
<overview> | ||
<p>Directly incorporating user input into an BigDecimal Operation Function without validating the input |
There was a problem hiding this comment.
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)
<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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
?
There was a problem hiding this comment.
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.
java/ql/src/experimental/Security/BigDecimalDOS/BigDecimalDOS.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/BigDecimalDOS/BigDecimalDOS.qhelp
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,38 @@ | |||
/** | |||
* @id java/BigDecimalDOS | |||
* @name Java-BigDecimal-DOS-Vulnerability |
There was a problem hiding this comment.
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
java/ql/src/experimental/Security/BigDecimalDOS/BigDecimalDOS.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/BigDecimalDOS/BigDecimalDOS.ql
Outdated
Show resolved
Hide resolved
@ResponseBody | ||
// GOOD: | ||
public Long demo02(@RequestParam(name = "num") String num) { | ||
if (num.length() > 33 || num.matches("(?i)e")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be:
if (num.length() > 33 || num.matches("(?i)e")) { | |
if (num.length() > 33 || num.matches("(?i).*e.*")) { |
(or similar)
} | ||
Long startTime = System.currentTimeMillis(); | ||
BigDecimal num1 = new BigDecimal(0.005); | ||
System.out.println(num1.add(num)); |
There was a problem hiding this comment.
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
Co-authored-by: Tony Torralba <[email protected]>
Co-authored-by: Marcono1234 <[email protected]>
Co-authored-by: Marcono1234 <[email protected]>
Co-authored-by: Marcono1234 <[email protected]>
Co-authored-by: Marcono1234 <[email protected]>
Co-authored-by: Marcono1234 <[email protected]>
Note that tests are failing:
|
Directly incorporating user input into an BigDecimal Operation Function without validating the input
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%.
This issue often occurs in scenarios that require scientific computing, such as e-commerce platforms and electronic payments.