Skip to content

Commit

Permalink
SONARKT-347 Update rules metadata (#356)
Browse files Browse the repository at this point in the history
  • Loading branch information
leveretka authored Jul 17, 2023
1 parent 1de2033 commit e429d6c
Show file tree
Hide file tree
Showing 35 changed files with 969 additions and 345 deletions.
2 changes: 1 addition & 1 deletion sonar-kotlin-plugin/sonarpedia.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"languages": [
"KOTLIN"
],
"latest-update": "2023-05-05T15:51:13.443953Z",
"latest-update": "2023-07-17T13:46:43.368502Z",
"options": {
"no-language-in-filenames": true,
"preserve-filenames": true
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
<h2>Why is this an issue?</h2>
<p>Shared naming conventions allow teams to collaborate efficiently. This rule checks that all function names match a provided regular expression.</p>
<p>Shared naming conventions allow teams to collaborate efficiently.</p>
<p>This rule raises an issue when a function name does not match a provided regular expression.</p>
<p>For example, with the default provided regular expression <code>^[a-zA-Z][a-zA-Z0-9]*$</code>, the function:</p>
<pre>
fun _DoSomething() {...} // Noncompliant
</pre>
<p>should be renamed to</p>
<pre>
fun DoSomething() {...}
</pre>

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Method names should comply with a naming convention",
"title": "Function names should comply with a naming convention",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
<h2>Why is this an issue?</h2>
<p>Shared coding conventions allow teams to collaborate effectively. This rule allows to check that all class names match a provided regular
expression.</p>
<h3>Noncompliant code example</h3>
<p>With default provided regular expression <code>^[A-Z][a-zA-Z0-9]*$</code>:</p>
<p>Shared naming conventions allow teams to collaborate efficiently.</p>
<p>This rule raises an issue when a class name does not match a provided regular expression.</p>
<p>For example, with the default provided regular expression <code>^[A-Z][a-zA-Z0-9]*$</code>, the class:</p>
<pre>
class my_class {...}
class my_class {...} // Noncompliant
</pre>
<h3>Compliant solution</h3>
<p>should be renamed to</p>
<pre>
class MyClass {...}
</pre>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<h2>Why is this an issue?</h2>
<p>Having to scroll horizontally makes it harder to get a quick overview and understanding of any piece of code.</p>
<p>Scrolling horizontally to see a full line of code lowers the code readability.</p>

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<h2>Why is this an issue?</h2>
<p>A source file that grows too much tends to aggregate too many responsibilities and inevitably becomes harder to understand and therefore to
maintain. Above a specific threshold, it is strongly advised to refactor it into smaller pieces of code which focus on well defined tasks. Those
smaller files will not only be easier to understand but also probably easier to test.</p>
<p>A source file that grows too much tends to aggregate too many responsibilities and inevitably becomes harder to understand and, therefore, to
maintain.</p>
<p>Above a specific threshold, refactor the file into smaller files whose code focuses on well-defined tasks. Those smaller files will be easier to
understand and easier to test.</p>

Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<h2>Why is this an issue?</h2>
<p>Developers should not need to configure the tab width of their text editors in order to be able to read source code.</p>
<p>So the use of the tabulation character must be banned.</p>
<p>The tab width can differ from one development environment to another. Using tabs may require other developers to configure their environment (text
editor, preferences, etc.) to read source code.</p>
<p>That is why using spaces is preferable.</p>

Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
<h2>Why is this an issue?</h2>
<p>Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed.</p>
<h3>Noncompliant code example</h3>
<p>An empty code block is confusing. It will require some effort from maintainers to determine if it is intentional or indicates the implementation is
incomplete.</p>
<pre>
for (int i = 0; i &lt; 42; i++){} // Empty on purpose or missing piece of code ?
for (int i = 0; i &lt; 42; i++){} // Noncompliant: is the block empty on purpose, or is code missing?
</pre>
<p>Removing or filling the empty code blocks takes away ambiguity and generally results in a more straightforward and less surprising code.</p>
<h3>Exceptions</h3>
<p>When a block contains a comment, this block is not considered to be empty.</p>
<p><code>while</code> and unless loops are also exception to the rule.</p>
<p>The rule ignores:</p>
<ul>
<li> code blocks that contain comments </li>
<li> <code>while</code> loops </li>
</ul>
<pre>
while (order.processNext()); // Compliant
while (order.processNext()); // Compliant by exception
</pre>

Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ <h2>Why is this an issue?</h2>
<p><code>FIXME</code> tags are commonly used to mark places where a bug is suspected, but which the developer wants to deal with later.</p>
<p>Sometimes the developer will not have the time or will simply forget to get back to that tag.</p>
<p>This rule is meant to track those tags and to ensure that they do not go unnoticed.</p>
<h3>Noncompliant code example</h3>
<pre>
// FIXME denominator value might be 0
fun divide(numerator: Int, denominator: Int): Int = numerator / denominator
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://cwe.mitre.org/data/definitions/546">MITRE, CWE-546</a> - Suspicious Comment </li>
<li> <a href="https://cwe.mitre.org/data/definitions/546">MITRE, CWE-546 - Suspicious Comment</a> </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@
546
]
},
"quickfix": "unknown"
"quickfix": "infeasible"
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
<h2>Why is this an issue?</h2>
<p>For better readability, do not put more than one statement on a single line.</p>
<h3>Noncompliant code example</h3>
<p>Putting multiple statements on a single line lowers the code readability and makes debugging the code more complex.</p>
<pre>
foo(); bar();
foo(); bar(); // Noncompliant
</pre>
<h3>Compliant solution</h3>
<p>Write one statement per line to improve readability.</p>
<pre>
foo();
bar();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<h2>Why is this an issue?</h2>
<p>Using the same value on either side of a binary operator is almost always a mistake. In the case of logical operators, it is either a copy/paste
error and therefore a bug, or it is simply wasted code, and should be simplified. In the case of bitwise operators and most binary mathematical
operators, having the same value on both sides of an operator yields predictable results, and should be simplified.</p>
<p>Using the same value on both sides of a binary operator is a code defect. In the case of logical operators, it is either a copy/paste error and,
therefore, a bug, or it is simply duplicated code and should be simplified. In the case of bitwise operators and most binary mathematical operators,
having the same value on both sides of an operator yields predictable results and should be simplified as well.</p>
<h3>Exceptions</h3>
<p>This rule ignores <code>*</code>, <code>+</code>, and <code>=</code>.</p>
<h2>Resources</h2>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,38 +1,60 @@
<p>This vulnerability increases the likelihood that attackers are able to compute the cleartext of password hashes.</p>
<h2>Why is this an issue?</h2>
<p>In cryptography, a "salt" is an extra piece of data which is included when hashing a password. This makes <code>rainbow-table attacks</code> more
difficult. Using a cryptographic hash function without an unpredictable salt increases the likelihood that an attacker could successfully find the
hash value in databases of precomputed hashes (called <code>rainbow-tables</code>).</p>
<p>This rule raises an issue when a hashing function which has been specifically designed for hashing passwords, such as <code>PBKDF2</code>, is used
with a non-random, reused or too short salt value. It does not raise an issue on base hashing algorithms such as <code>sha1</code> or <code>md5</code>
as they should not be used to hash passwords.</p>
<h2>Recommended Secure Coding Practices</h2>
<ul>
<li> Use hashing functions generating their own secure salt or generate a secure random value of at least 16 bytes. </li>
<li> The salt should be unique by user password. </li>
</ul>
<h3>Noncompliant code example</h3>
<pre>
val salt = "notrandom".toByteArray()
<p>During the process of password hashing, an additional component, known as a "salt," is often integrated to bolster the overall security. This salt,
acting as a defensive measure, primarily wards off certain types of attacks that leverage pre-computed tables to crack passwords.</p>
<p>However, potential risks emerge when the salt is deemed insecure. This can occur when the salt is consistently the same across all users or when it
is too short or predictable. In scenarios where users share the same password and salt, their password hashes will inevitably mirror each other.
Similarly, a short salt heightens the probability of multiple users unintentionally having identical salts, which can potentially lead to identical
password hashes. These identical hashes streamline the process for potential attackers to recover clear-text passwords. Thus, the emphasis on
implementing secure, unique, and sufficiently lengthy salts in password-hashing functions is vital.</p>
<h3>What is the potential impact?</h3>
<p>Despite best efforts, even well-guarded systems might have vulnerabilities that could allow an attacker to gain access to the hashed passwords.
This could be due to software vulnerabilities, insider threats, or even successful phishing attempts that give attackers the access they need.</p>
<p>Once the attacker has these hashes, they will likely attempt to crack them using a couple of methods. One is brute force, which entails trying
every possible combination until the correct password is found. While this can be time-consuming, having the same salt for all users or a short salt
can make the task significantly easier and faster.</p>
<p>If multiple users have the same password and the same salt, their password hashes would be identical. This means that if an attacker successfully
cracks one hash, they have effectively cracked all identical ones, granting them access to multiple accounts at once.</p>
<p>A short salt, while less critical than a shared one, still increases the odds of different users having the same salt. This might create clusters
of password hashes with identical salt that can then be attacked as explained before.</p>
<p>With short salts, the probability of a collision between two users' passwords and salts couple might be low depending on the salt size. The shorter
the salt, the higher the collision probability. In any case, using longer, cryptographically secure salt should be preferred.</p>
<h2>How to fix it in Java SE</h2>
<h3>Code examples</h3>
<p>The following code contains examples of hard-coded salts.</p>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
import javax.crypto.spec.PBEParameterSpec

val cipherSpec = PBEParameterSpec(salt, 10000) // Noncompliant, predictable salt
val spec = PBEKeySpec(password, salt, 10000, 256) // Noncompliant, predictable salt
fun hash() {
val salt = "salty".toByteArray()
val cipherSpec = PBEParameterSpec(salt, 10000) // Noncompliant
}
</pre>
<h3>Compliant solution</h3>
<pre>
val random = SecureRandom()
val salt = ByteArray(16)
random.nextBytes(salt)
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
import java.security.SecureRandom
import javax.crypto.spec.PBEParameterSpec

val cipherSpec = PBEParameterSpec(salt, 10000) // Compliant
val spec = PBEKeySpec(password, salt, 10000, 256) // Compliant
fun hash() {
val random = SecureRandom()
val salt = ByteArray(16)
random.nextBytes(salt)
val cipherSpec = PBEParameterSpec(salt, 10000)
}
</pre>
<h3>How does this work?</h3>
<p>This code ensures that each user’s password has a unique salt value associated with it. It generates a salt randomly and with a length that
provides the required security level. It uses a salt length of at least 16 bytes (128 bits), as recommended by industry standards.</p>
<p>Here, the compliant code example ensures the salt is random and has a sufficient length by calling the <code>nextBytes</code> method from the
<code>SecureRandom</code> class with a salt buffer of 16 bytes. This class implements a cryptographically secure pseudo-random number generator.</p>
<h2>Resources</h2>
<h3>Standards</h3>
<ul>
<li> <a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">OWASP Top 10 2021 Category A2</a> - Cryptographic Failures </li>
<li> <a href="https://www.owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure">OWASP Top 10 2017 Category A3</a> - Sensitive Data
<li> <a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">OWASP</a> Top 10:2021 A02:2021 - Cryptographic Failures </li>
<li> <a href="https://www.owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure">OWASP</a> - Top 10 2017 - A03:2017 - Sensitive Data
Exposure </li>
<li> <a href="https://cwe.mitre.org/data/definitions/759">MITRE, CWE-759</a> - Use of a One-Way Hash without a Salt </li>
<li> <a href="https://cwe.mitre.org/data/definitions/760">MITRE, CWE-760</a> - Use of a One-Way Hash with a Predictable Salt </li>
<li> <a href="https://www.sans.org/top25-software-errors/#cat3">SANS Top 25</a> - Porous Defenses </li>
<li> <a href="https://cwe.mitre.org/data/definitions/759">CWE</a> - CWE-759: Use of a One-Way Hash without a Salt </li>
<li> <a href="https://cwe.mitre.org/data/definitions/760">CWE</a> - CWE-760: Use of a One-Way Hash with a Predictable Salt </li>
</ul>

Original file line number Diff line number Diff line change
@@ -1,21 +1,67 @@
<h2>Why is this an issue?</h2>
<p>As the <code>equals</code> method’s parameter is typed as generic <code>Any?</code>, any type of object may be passed to it. The method
implementation should not assume it will only be used to test objects of its own class type. It must instead check the parameter’s type.</p>
<h3>Noncompliant code example</h3>
<p>The <code>Any#equals(other: Any?)</code> method is used to compare two objects to see if they are equal.</p>
<p>The <code>other</code> parameter’s type is <code>Any?</code>, this means that an object of any type, as well as <code>null</code>, can be passed as
an argument to this method.</p>
<p>Any class overriding <code>Any#equals(other: Any?)</code> should respect this contract, accept any object as an argument, and return
<code>false</code> when the argument’s type differs from the expected type. The <code>other</code> parameter’s type can be checked using the
<code>is</code> operator or by comparing the <code>javaClass</code> field:</p>
<pre>
override fun equals(other: Any?): Boolean { // Noncompliant
val mc = other as MyClass
override fun equals(other: Any?): Boolean {
// ...
if (other?.javaClass != this.javaClass) {
return false
}
// ...
}
</pre>
<h3>Compliant solution</h3>
<p>However, it is an issue to assume that the <code>equals</code> method will only be used to compare objects of the same type. Casting the
<code>other</code> parameter without a prior test will throw a <code>ClassCastException</code> instead of returning false.</p>
<pre>
override fun equals(other: Any?): Boolean {
if (other?.javaClass != this.javaClass)
return false

val mc = other as MyClass
class MyClass {
override fun equals(other: Any?): Boolean {
val that = other as MyClass // may throw a ClassCastException
// ...
}
// ...
}
</pre>
<p>This rule raises an issue when <code>other</code> parameter’s type has not been tested before a cast operation.</p>
<h2>How to fix it</h2>
<p>Ensure the <code>other</code> parameter’s type is checked by comparing <code>other?.javaClass</code> and <code>this.javaClass</code>, or use the
<code>is</code> operator to test `other’s type.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
class MyClass {
override fun equals(other: Any?): Boolean {
if (this === other) {
return true
}
val that = other as MyClass // Noncompliant, may throw a ClassCastException
// ...
}
// ...
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
class MyClass {
override fun equals(other: Any?): Boolean {
if (this === other) {
return true
}
if (other?.javaClass != this.javaClass) {
return false
}
val that = other as MyClass // Compliant, other's type is MyClass
// ...
}
// ...
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-any/equals.html">Kotlin Standard Library - Any#equals(other: Any?)</a> </li>
</ul>

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "\"equals(other: Any?)\" should test argument type",
"title": "\"equals(other: Any?)\" should test the argument\u0027s type",
"type": "BUG",
"status": "ready",
"remediation": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ <h2>Why is this an issue?</h2>
code.</p>
<p>Further, because some methods require that the argument remain unmodified during the execution, passing a collection to itself can result in
undefined behavior.</p>
<h3>Noncompliant code example</h3>
<pre>
<h2>How to fix it</h2>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
val objs = mutableListOf&lt;Any&gt;()
objs.add("Hello")

Expand All @@ -14,8 +16,8 @@ <h3>Noncompliant code example</h3>
objs.removeAll(objs) // Noncompliant; confusing. Use clear() instead
objs.retainAll(objs) // Noncompliant; NOOP
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
val newList = mutableListOf&lt;Any&gt;()
val objs = mutableListOf&lt;Any&gt;()
objs.add("Hello")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@
"ruleSpecification": "RSPEC-2114",
"sqKey": "S2114",
"scope": "All",
"quickfix": "unknown"
"quickfix": "infeasible"
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,11 @@
<h2>Why is this an issue?</h2>
<p>Running finalizers on JVM exit is disabled by default. It can be enabled with <code>System.runFinalizersOnExit</code> and
<code>Runtime.runFinalizersOnExit</code>, but both methods are deprecated because they are inherently unsafe.</p>
<p>According to the Oracle Javadoc:</p>
<blockquote>
<p>It may result in finalizers being called on live objects while other threads are concurrently manipulating those objects, resulting in erratic
behavior or deadlock.</p>
</blockquote>
<p>If you really want to execute something when the virtual machine begins its shutdown sequence, you should attach a shutdown hook.</p>
<p>Enabling <code>runFinalizersOnExit</code> is unsafe as it might result in erratic behavior and deadlocks on application exit.</p>
<p>Indeed, finalizers might be force-called on live objects while other threads are concurrently manipulating them.</p>
<p>Instead, if you want to execute something when the virtual machine begins its shutdown sequence, you should attach a shutdown hook.</p>
<h3>Noncompliant code example</h3>
<pre>
fun main() {
...
System.runFinalizersOnExit(true) // Noncompliant
...
}
</pre>
<h3>Compliant solution</h3>
Expand Down
Loading

0 comments on commit e429d6c

Please sign in to comment.