Skip to content

Commit

Permalink
Update rules metadata before release(#4471)
Browse files Browse the repository at this point in the history
  • Loading branch information
leonardo-pilastri-sonarsource authored Oct 2, 2023
1 parent e3e67c8 commit 511de4b
Show file tree
Hide file tree
Showing 38 changed files with 1,257 additions and 619 deletions.
Original file line number Diff line number Diff line change
@@ -1,23 +1,38 @@
<h2>Why is this an issue?</h2>
<p>When logging a message there are several important requirements which must be fulfilled:</p>
<p>When logging a message, the following requirements should be fulfilled:</p>
<ul>
<li> The user must be able to easily retrieve the logs </li>
<li> The format of all logged message must be uniform to allow the user to easily read the log </li>
<li> Logged data must actually be recorded </li>
<li> Sensitive data must only be logged securely </li>
<li> The user can easily record logged data and retrieve the logs. </li>
<li> The format of all logged messages is uniform and provides context about the message creator. This improves the readability of the logs. </li>
<li> Sensitive data is logged securely. </li>
</ul>
<p>If a program directly writes to the standard outputs, there is absolutely no way to comply with those requirements. That’s why defining and using a
dedicated logger is highly recommended.</p>
<p>Simply printing messages to <code>System.out</code> or <code>System.err</code> does not fulfill these needs. A dedicated logger should be used
instead.</p>
<h3>Noncompliant code example</h3>
<pre>
System.out.println("My Message"); // Noncompliant
<pre data-diff-id="1" data-diff-type="noncompliant">
class PrintMessage {
public void print() {
System.out.println("My Message"); // Noncompliant, output directly to System.out without a logger
}
}
</pre>
<h3>Compliant solution</h3>
<pre>
logger.log("My Message");
<pre data-diff-id="1" data-diff-type="compliant">
import java.util.logging.Logger;

class PrintMessage {

Logger logger = Logger.getLogger(getClass().getName());

public void print() {
logger.info("My Message"); // Compliant, output via logger
}
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://docs.oracle.com/javase/7/docs/api/java/util/logging/Logger.html">Java SE 7 API Specification: java.util.logging.Logger</a>
</li>
<li> <a href="https://owasp.org/Top10/A09_2021-Security_Logging_and_Monitoring_Failures/">OWASP Top 10 2021 Category A9</a> - Security Logging and
Monitoring 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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Classes should not be coupled to too many other classes (Single Responsibility Principle)",
"title": "Classes should not be coupled to too many other classes",
"type": "CODE_SMELL",
"code": {
"impacts": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ <h4>Noncompliant code example</h4>
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
<pre data-diff-id="1" data-diff-type="compliant">
int end = foo();
for (int i = 0; i &lt; end; i++) { // Compliant, "end" does not change during loop execution
// ...
Expand All @@ -35,7 +35,7 @@ <h4>Noncompliant code example</h4>
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="3" data-diff-type="compliant">
<pre data-diff-id="2" data-diff-type="compliant">
int i = 0;
while (i++ &lt; 10) { // Compliant
// ...
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<h2>Why is this an issue?</h2>
<p><code>switch</code> statements are useful when there are many different cases depending on the value of the same expression.</p>
<p>For just one or two cases however, the code will be more readable with <code>if</code> statements.</p>
<p>For just one or two cases, however, the code will be more readable with <code>if</code> statements.</p>
<h3>Noncompliant code example</h3>
<pre>
switch (variable) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ <h3>Noncompliant code example</h3>
<h3>Compliant solution</h3>
<pre>
if (i &gt; 10) {
System.out.println(("yes");
System.out.println("yes");
} else {
System.out.println("no");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,5 @@
563
]
},
"quickfix": "unknown"
"quickfix": "targeted"
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
<h2>Why is this an issue?</h2>
<p>Having two <code>cases</code> in a <code>switch</code> statement or two branches in an <code>if</code> chain with the same implementation is at
best duplicate code, and at worst a coding error. If the same logic is truly needed for both instances, then in an <code>if</code> chain they should
be combined, or for a <code>switch</code>, one should fall through to the other.</p>
<h3>Noncompliant code example</h3>
<pre>
best duplicate code, and at worst a coding error.</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
switch (i) {
case 1:
doFirstThing();
Expand All @@ -19,7 +17,8 @@ <h3>Noncompliant code example</h3>
default:
doTheRest();
}

</pre>
<pre data-diff-id="2" data-diff-type="noncompliant">
if (a &gt;= 0 &amp;&amp; a &lt; 10) {
doFirstThing();
doTheThing();
Expand All @@ -35,9 +34,44 @@ <h3>Noncompliant code example</h3>
doTheRest();
}
</pre>
<p>If the same logic is truly needed for both instances, then:</p>
<ul>
<li> in an <code>if</code> chain they should be combined </li>
</ul>
<pre data-diff-id="2" data-diff-type="compliant">
if ((a &gt;= 0 &amp;&amp; a &lt; 10) || (a &gt;= 20 &amp;&amp; a &lt; 50)) { // Compliant
doFirstThing();
doTheThing();
}
else if (a &gt;= 10 &amp;&amp; a &lt; 20) {
doTheOtherThing();
}
else {
doTheRest();
}
</pre>
<ul>
<li> for a <code>switch</code>, one should fall through to the other. </li>
</ul>
<pre data-diff-id="1" data-diff-type="compliant">
switch (i) {
case 1:
case 3: // Compliant
doFirstThing();
doSomething();
break;
case 2:
doSomethingDifferent();
break;
default:
doTheRest();
}
</pre>
<p>When all blocks are identical, either this rule will trigger if there is no default clause or rule {rule:java:S3923} will raise if there is a
default clause.</p>
<h3>Exceptions</h3>
<p>Blocks in an <code>if</code> chain that contain a single line of code are ignored, as are blocks in a <code>switch</code> statement that contain a
single line of code with or without a following <code>break</code>.</p>
<p>Unless all blocks are identical, blocks in an <code>if</code> chain that contain a single line of code are ignored. The same applies to blocks in a
<code>switch</code> statement that contains a single line of code with or without a following <code>break</code>.</p>
<pre>
if (a == 1) {
doSomething(); //no issue, usually this is done on purpose to increase the readability
Expand All @@ -47,14 +81,9 @@ <h3>Exceptions</h3>
doSomething();
}
</pre>
<p>But this exception does not apply to <code>if</code> chains without <code>else</code>-s, or to <code>switch</code>-es without default clauses when
all branches have the same single line of code. In case of <code>if</code> chains with <code>else</code>-s, or of <code>switch</code>-es with default
clauses, rule {rule:java:S3923} raises a bug.</p>
<pre>
if (a == 1) {
doSomething(); //Noncompliant, this might have been done on purpose but probably not
} else if (a == 2) {
doSomething();
}
</pre>
<h2>Resources</h2>
<h3>Related rules</h3>
<ul>
<li> {rule:java:S3923} - All branches in a conditional structure should not have exactly the same implementation </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@
772
]
},
"quickfix": "unknown"
"quickfix": "infeasible"
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,64 @@
<p>When accessing a database, an empty password should be avoided as it introduces a weakness.</p>
<h2>Why is this an issue?</h2>
<p>When relying on the password authentication mode for the database connection, a secure password should be chosen.</p>
<p>This rule raises an issue when an empty password is used.</p>
<h3>Noncompliant code example</h3>
<pre>
Connection conn = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true", "login", "");
<p>When a database does not require a password for authentication, it allows anyone to access and manipulate the data stored within it. Exploiting
this vulnerability typically involves identifying the target database and establishing a connection to it without the need for any authentication
credentials.</p>
<h3>What is the potential impact?</h3>
<p>Once connected, an attacker can perform various malicious actions, such as viewing, modifying, or deleting sensitive information, potentially
leading to data breaches or unauthorized access to critical systems. It is crucial to address this vulnerability promptly to ensure the security and
integrity of the database and the data it contains.</p>
<h4>Unauthorized Access to Sensitive Data</h4>
<p>When a database lacks a password for authentication, it opens the door for unauthorized individuals to gain access to sensitive data. This can
include personally identifiable information (PII), financial records, intellectual property, or any other confidential information stored in the
database. Without proper access controls in place, malicious actors can exploit this vulnerability to retrieve sensitive data, potentially leading to
identity theft, financial loss, or reputational damage.</p>
<h4>Compromise of System Integrity</h4>
<p>Without a password requirement, unauthorized individuals can gain unrestricted access to a database, potentially compromising the integrity of the
entire system. Attackers can inject malicious code, alter configurations, or manipulate data within the database, leading to system malfunctions,
unauthorized system access, or even complete system compromise. This can disrupt business operations, cause financial losses, and expose the
organization to further security risks.</p>
<h4>Unwanted Modifications or Deletions</h4>
<p>The absence of a password for database access allows anyone to make modifications or deletions to the data stored within it. This poses a
significant risk, as unauthorized changes can lead to data corruption, loss of critical information, or the introduction of malicious content. For
example, an attacker could modify financial records, tamper with customer orders, or delete important files, causing severe disruptions to business
processes and potentially leading to financial and legal consequences.</p>
<p>Overall, the lack of a password configured to access a database poses a serious security risk, enabling unauthorized access, data breaches, system
compromise, and unwanted modifications or deletions. It is essential to address this vulnerability promptly to safeguard sensitive data, maintain
system integrity, and protect the organization from potential harm.</p>
<h2>How to fix it in Java SE</h2>
<h3>Code examples</h3>
<p>The following code uses an empty password to connect to a Postgres database.</p>
<p>The vulnerability can be fixed by using a strong password retrieved from Properties. This <code>database.password</code> property is set during
deployment. Its value should be strong and different for each database.</p>
<h4>Noncompliant code example</h4>
<pre data-diff-id="201" data-diff-type="noncompliant">
Connection conn = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true", "login", ""); // Noncompliant
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="201" data-diff-type="compliant">
String password = System.getProperty("database.password");
Connection conn = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true", "login", password);
</pre>
<h3>Pitfalls</h3>
<h4>Hard-coded passwords</h4>
<p>It could be tempting to replace the empty password with a hard-coded one. Hard-coding passwords in the code can pose significant security risks.
Here are a few reasons why it is not recommended:</p>
<ol>
<li> Security Vulnerability: Hard-coded passwords can be easily discovered by anyone who has access to the code, such as other developers or
attackers. This can lead to unauthorized access to the database and potential data breaches. </li>
<li> Lack of Flexibility: Hard-coded passwords make it difficult to change the password without modifying the code. If the password needs to be
updated, it would require recompiling and redeploying the code, which can be time-consuming and error-prone. </li>
<li> Version Control Issues: Storing passwords in code can lead to version control issues. If the code is shared or stored in a version control
system, the password will be visible to anyone with access to the repository, which is a security risk. </li>
</ol>
<p>To mitigate these risks, it is recommended to use secure methods for storing and retrieving passwords, such as using environment variables,
configuration files, or secure key management systems. These methods allow for better security, flexibility, and separation of sensitive information
from the codebase.</p>
<h2>Resources</h2>
<ul>
<li> <a href="https://docs.oracle.com/javase/tutorial/essential/environment/properties.html">Java Properties</a> </li>
</ul>
<h3>Standards</h3>
<ul>
<li> <a href="https://owasp.org/Top10/A07_2021-Identification_and_Authentication_Failures/">OWASP Top 10 2021 Category A7</a> - Identification and
Authentication Failures </li>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"impacts": {
"SECURITY": "HIGH"
},
"attribute": "COMPLETE"
"attribute": "TRUSTWORTHY"
},
"status": "ready",
"remediation": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ <h4>Compliant solution</h4>
<h4>Noncompliant code example</h4>
<p>If the intention is to have an infinite loop or a loop terminated only by a break statement, use a <code>while</code> or a <code>do</code>
<code>while</code> statement instead.</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
<pre data-diff-id="3" data-diff-type="noncompliant">
for (int i = 0; i &lt; 0; i++) { // Noncompliant, loop is not infinite
String event = waitForNextEvent();
if (event == "terminate") break;
processEvent(event);
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
<pre data-diff-id="3" data-diff-type="compliant">
while (true) { // Compliant
String event = waitForNextEvent();
if (event == "terminate") break;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,28 +1,49 @@
<p>This function uses a session ID that is supplied by the client. Because of this, the ID may not be valid or might even be spoofed.</p>
<h2>Why is this an issue?</h2>
<p>According to the Oracle Java API, the <code>HttpServletRequest.getRequestedSessionId()</code> method:</p>
<p>According to the API documentation of the <code>HttpServletRequest.getRequestedSessionId()</code> method:</p>
<blockquote>
<p>Returns the session ID specified by the client. This may not be the same as the ID of the current valid session for this request. If the client
did not specify a session ID, this method returns null.</p>
</blockquote>
<p>The session ID it returns is either transmitted in a cookie or a URL parameter so by definition, nothing prevents the end-user from manually
updating the value of this session ID in the HTTP request.</p>
<p>Here is an example of an updated HTTP header:</p>
<pre>
GET /pageSomeWhere HTTP/1.1
Host: webSite.com
User-Agent: Mozilla/5.0
Cookie: JSESSIONID=Hacked_Session_Value'''"&gt;
</pre>
<p>The session ID it returns is either transmitted through a cookie or a URL parameter. This allows an end user to manually update the value of this
session ID in an HTTP request.</p>
<p>Due to the ability of the end-user to manually change the value, the session ID in the request should only be used by a servlet container (e.g.
Tomcat or Jetty) to see if the value matches the ID of an existing session. If it does not, the user should be considered unauthenticated. Moreover,
this session ID should never be logged as is but logged using a one-way hash to prevent hijacking of active sessions.</p>
<h3>Noncompliant code example</h3>
<pre>
if (isActiveSession(request.getRequestedSessionId())) {
// ...
Tomcat or Jetty) to see if the value matches the ID of an existing session. If it does not, the user should be considered unauthenticated.</p>
<h3>What is the potential impact?</h3>
<p>Using a client-supplied session ID to manage sessions on the server side can potentially have an impact on the security of the application.</p>
<h4>Impersonation (through session fixation)</h4>
<p>If an attacker succeeds in fixing a user’s session to a session identifier that they know, then they can impersonate this victim and gain access to
their account without providing valid credentials. This can result in unauthorized actions, such as modifying personal information, making
unauthorized transactions, or even performing malicious activities on behalf of the victim. An attacker can also manipulate the victim into performing
actions they wouldn’t normally do, such as revealing sensitive information or conducting financial transactions on the attacker’s behalf.</p>
<h2>How to fix it in Java EE</h2>
<h3>Code examples</h3>
<p>In both examples, a session ID is used to check whether a user’s session is still active. In the noncompliant example, the session ID supplied by
the user is used. In the compliant example, the session ID defined by the server is used instead.</p>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
if (isActiveSession(request.getRequestedSessionId())) { // Noncompliant
// ...
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
if (isActiveSession(request.getSession().getId())) {
// ...
}
</pre>
<h3>How does this work?</h3>
<p>The noncompliant example uses <code>HttpServletRequest.getRequestedSessionId()</code> to retrieve a session ID. This ID is then used to verify if
the given session is still active. As this value is given by a user, this value is not guaranteed to be a valid ID.</p>
<p>The compliant example instead uses the server’s session ID to verify if the session is active. Additionally, <code>getSession()</code> will create
a new session if the user’s request does not contain a valid ID.</p>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> Jakarta EE Documentation - <a
href="https://jakarta.ee/specifications/platform/10/apidocs/jakarta/servlet/http/httpservletrequest#getRequestedSessionId--"><code>HttpServletRequest</code> - <code>getRequestedSessionId</code></a> </li>
</ul>
<h3>Standards</h3>
<ul>
<li> <a href="https://owasp.org/Top10/A04_2021-Insecure_Design/">OWASP Top 10 2021 Category A4</a> - Insecure Design </li>
<li> <a href="https://owasp.org/www-project-top-ten/2017/A2_2017-Broken_Authentication">OWASP Top 10 2017 Category A2</a> - Broken Authentication
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ <h3>Compliant solution</h3>
}
</pre>
<h2>Resources</h2>
<h3>Articles &amp; blog posts</h3>
<ul>
<li> <a href="https://cwe.mitre.org/data/definitions/571">MITRE, CWE-571</a> - Expression is Always True </li>
<li> <a href="https://cwe.mitre.org/data/definitions/570">MITRE, CWE-570</a> - Expression is Always False </li>
Expand Down
Loading

0 comments on commit 511de4b

Please sign in to comment.