Skip to content

Commit

Permalink
SONARJAVA-3697 Update rules metadata
Browse files Browse the repository at this point in the history
  • Loading branch information
alban-auzeill authored and quentin-jaquier-sonarsource committed Feb 22, 2021
1 parent b79d4a9 commit 51c3ad4
Show file tree
Hide file tree
Showing 59 changed files with 255 additions and 319 deletions.
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
<p>The Java Language Specification recommends listing modifiers in the following order:</p>
<p>1. Annotations</p>
<p>2. public</p>
<p>3. protected</p>
<p>4. private</p>
<p>5. abstract</p>
<p>6. static</p>
<p>7. final</p>
<p>8. transient</p>
<p>9. volatile</p>
<p>10. synchronized</p>
<p>11. native</p>
<p>12. strictfp</p>
<ol>
<li> Annotations </li>
<li> public </li>
<li> protected </li>
<li> private </li>
<li> abstract </li>
<li> static </li>
<li> final </li>
<li> transient </li>
<li> volatile </li>
<li> synchronized </li>
<li> native </li>
<li> strictfp </li>
</ol>
<p>Not following this convention has no technical impact, but will reduce the code's readability because most developers are used to the standard
order.</p>
<h2>Noncompliant Code Example</h2>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
<p>An exception in a <code>throws</code> declaration in Java is superfluous if it is:</p>
<p> * listed multiple times</p>
<p> * a subclass of another listed exception</p>
<p> * completely unnecessary because the declared exception type cannot actually be thrown</p>
<ul>
<li> listed multiple times </li>
<li> a subclass of another listed exception </li>
<li> completely unnecessary because the declared exception type cannot actually be thrown </li>
</ul>
<h2>Noncompliant Code Example</h2>
<pre>
void foo() throws MyException, MyException {} // Noncompliant; should be listed once
Expand All @@ -14,11 +16,13 @@ <h2>Compliant Solution</h2>
</pre>
<h2>Exceptions</h2>
<p>The rule will not raise any issue for exceptions that cannot be thrown from the method body:</p>
<p> * in overriding and implementation methods</p>
<p> * in interface <code>default</code> methods</p>
<p> * in non-private methods that only <code>throw</code>, have empty bodies, or a single return statement.</p>
<p> * in overridable methods (non-final, or not member of a final class, non-static, non-private), if the exception is documented with a proper
JavaDoc</p>
<ul>
<li> in overriding and implementation methods </li>
<li> in interface <code>default</code> methods </li>
<li> in non-private methods that only <code>throw</code>, have empty bodies, or a single return statement. </li>
<li> in overridable methods (non-final, or not member of a final class, non-static, non-private), if the exception is documented with a proper
JavaDoc </li>
</ul>
<p>Also, the rule won't raise issues on <code>RuntimeException</code>, or one of its descendants, because explicating runtime exceptions which could
be thrown can ultimately help the method's users, and can even be considered as good practice.</p>
<pre>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ <h2>Noncompliant Code Example</h2>
}
}
}

</pre>
<h2>Compliant Solution</h2>
<pre>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<ol>
<li> <code>x.clone() != x</code> </li>
<li> <code>x.clone().getClass() == x.getClass()</code> </li>
<li> <code>x.clone().equals\(x\)</code> </li>
<li> <code>x.clone().equals(x)</code> </li>
</ol>
<p>Obtaining the object that will be returned by calling <code>super.clone()</code> helps to satisfy those invariants:</p>
<ol>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ <h2>Compliant Solution</h2>

Foo foo = new Foo();
foo.getName()

</pre>
<h2>Exceptions</h2>
<p>When the type of the field is the containing class and that field is static, no issue is raised to allow singletons named like the type. </p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ <h2>Noncompliant Code Example</h2>
synchronized(listLock) { // Noncompliant
// ...
}

</pre>
<h2>Compliant Solution</h2>
<pre>
Expand All @@ -60,6 +59,8 @@ <h2>Compliant Solution</h2>
}
</pre>
<h2>See</h2>
<p> * <a href="https://wiki.sei.cmu.edu/confluence/x/1zdGBQ">CERT, LCK01-J.</a> - Do not synchronize on objects that may be reused</p>
<p> * <a href="https://openjdk.java.net/jeps/390">JEP-390.</a> - JEP 390: Warnings for Value-Based Classes</p>
<ul>
<li> <a href="https://wiki.sei.cmu.edu/confluence/x/1zdGBQ">CERT, LCK01-J.</a> - Do not synchronize on objects that may be reused </li>
<li> <a href="https://openjdk.java.net/jeps/390">JEP-390.</a> - JEP 390: Warnings for Value-Based Classes </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ <h2>Compliant Solution</h2>
else if (param == 3)
moveWindowToTheBackground();
}

</pre>
<h2>See</h2>
<ul>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,62 +1,18 @@
<p>Formatting strings used as SQL queries is security-sensitive. It has led in the past to the following vulnerabilities:</p>
<ul>
<li> <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-9019">CVE-2018-9019</a> </li>
<li> <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-7318">CVE-2018-7318</a> </li>
<li> <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-5611">CVE-2017-5611</a> </li>
</ul>
<p>SQL queries often need to use a hardcoded SQL string with a dynamic parameter coming from a user request. Formatting a string to add those
parameters to the request is a bad practice as it can result in an <a href="https://www.owasp.org/index.php/SQL_Injection">SQL injection</a>. The safe
way to add parameters to a SQL query is to use SQL binding mechanisms.</p>
<p>This rule raises an issue when an SQL query is built by formatting Strings, even if there is no injection. This rule does not detect SQL
injections. The goal is to guide security code reviews and to prevent a common bad practice.</p>
<p>The following method signatures from Java JDBC, JPA, JDO, Hibernate and Spring are tested: </p>
<ul>
<li> <code>org.hibernate.Session.createQuery</code> </li>
<li> <code>org.hibernate.Session.createSQLQuery</code> </li>
<li> <code>java.sql.Statement.executeQuery</code> </li>
<li> <code>java.sql.Statement.execute</code> </li>
<li> <code>java.sql.Statement.executeUpdate</code> </li>
<li> <code>java.sql.Statement.executeLargeUpdate</code> </li>
<li> <code>java.sql.Statement.addBatch</code> </li>
<li> <code>java.sql.Connection.prepareStatement</code> </li>
<li> <code>java.sql.Connection.prepareCall</code> </li>
<li> <code>java.sql.Connection.nativeSQL</code> </li>
<li> <code>javax.persistence.EntityManager.createNativeQuery</code> </li>
<li> <code>javax.persistence.EntityManager.createQuery</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.batchUpdate</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.execute</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.query</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.queryForList</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.queryForMap</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.queryForObject</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.queryForRowSet</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.queryForInt</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.queryForLong</code> </li>
<li> <code>org.springframework.jdbc.core.JdbcOperations.update</code> </li>
<li> <code>org.springframework.jdbc.core.PreparedStatementCreatorFactory.&lt;init&gt;</code> </li>
<li> <code>org.springframework.jdbc.core.PreparedStatementCreatorFactory.newPreparedStatementCreator</code> </li>
<li> <code>javax.jdo.PersistenceManager.newQuery</code> </li>
<li> <code>javax.jdo.Query.setFilter</code> </li>
<li> <code>javax.jdo.Query.setGrouping</code> </li>
</ul>
<p>If a method is defined in an interface, implementations are also tested. For example this is the case for
<code>org.springframework.jdbc.core.JdbcOperations</code> , which is usually used as <code>org.springframework.jdbc.core.JdbcTemplate</code>). </p>
<p>Formatted SQL queries can be difficult to maintain, debug and can increase the risk of SQL injection when concatenating untrusted values into the
query. However, this rule doesn't detect SQL injections (unlike rule s3649), the goal is only to highlight complex/formatted queries.</p>
<h2>Ask Yourself Whether</h2>
<ul>
<li> the SQL query is built using string formatting technics, such as concatenating variables. </li>
<li> some of the values are coming from an untrusted source and are not sanitized. </li>
<li> Some parts of the query come from untrusted values (like user inputs). </li>
<li> The query is repeated/duplicated in other parts of the code. </li>
<li> The application must support different types of relational databases. </li>
</ul>
<p>There is a risk if you answered yes to any of those questions.</p>
<h2>Recommended Secure Coding Practices</h2>
<ul>
<li> Avoid building queries manually using formatting technics. If you do it anyway, do not include user input in this building process. </li>
<li> Use <a href="https://www.owasp.org/index.php/Query_Parameterization_Cheat_Sheet">parameterized queries, prepared statements, or stored
procedures</a> whenever possible. </li>
<li> You may also use ORM frameworks such as Hibernate which, if used correctly, reduce injection risks. </li>
<li> Avoid executing SQL queries containing unsafe input in stored procedures or functions. </li>
<li> <a href="https://www.owasp.org/index.php/Input_Validation_Cheat_Sheet">Sanitize</a> every unsafe input. </li>
procedures</a> and bind variables to SQL query parameters. </li>
<li> Consider using ORM frameworks if there is a need to have an abstract layer to access data. </li>
</ul>
<p>You can also reduce the impact of an attack by using a database account with low privileges.</p>
<h2>Sensitive Code Example</h2>
<pre>
public User getUser(Connection con, String user) throws SQLException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"hibernate",
"sql"
],
"defaultSeverity": "Critical",
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-2077",
"sqKey": "S2077",
"scope": "Main",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,13 @@
<p>Databases should always be password protected. The use of a database connection with an empty password is a clear indication of a database that is
not protected.</p>
<p>This rule flags database connections with empty passwords.</p>
<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>
<h2>Noncompliant Code Example</h2>
<pre>
Connection conn = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true", "AppLogin", "");
Connection conn2 = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true?user=user&amp;password=");
Connection conn = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true", "login", "");
</pre>
<h2>Compliant Solution</h2>
<pre>
DriverManager.getConnection("jdbc:derby:memory:myDB;create=true?user=user&amp;password=password");

DriverManager.getConnection("jdbc:mysql://address=(host=myhost1)(port=1111)(key1=value1)(user=sandy)(password=secret),address=(host=myhost2)(port=2222)(key2=value2)(user=sandy)(password=secret)/db");

DriverManager.getConnection("jdbc:mysql://sandy:secret@[myhost1:1111,myhost2:2222]/db");

String url = "jdbc:postgresql://localhost/test";
Properties props = new Properties();
props.setProperty("user", "fred");
props.setProperty("password", "secret");
DriverManager.getConnection(url, props);
String password = System.getProperty("database.password");
Connection conn = DriverManager.getConnection("jdbc:derby:memory:myDB;create=true", "login", password);
</pre>
<h2>See</h2>
<ul>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Databases should be password-protected",
"title": "A secure password should be used when connecting to a database",
"type": "VULNERABILITY",
"status": "ready",
"remediation": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,12 @@ <h2>Noncompliant Code Example</h2>
{
String argStr = args.toString(); // Noncompliant
int argHash = args.hashCode(); // Noncompliant

</pre>
<h2>Compliant Solution</h2>
<pre>
public static void main( String[] args )
{
String argStr = Arrays.toString(args);
int argHash = Arrays.hashCode(args);

</pre>

Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ <h2>Compliant Solution</h2>
</pre>
<h2>Exceptions</h2>
<p><code>BigDecimal</code> constructor with <code>double</code> argument is ignored as using <code>valueOf</code> instead might change resulting
value. See {rule:java:S2111}.</p>
value. See {rule:java:S2111} .</p>

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<p>By contract, the <code>NullCipher</code> class provides an "identity cipher" <del></del> one that does not transform or encrypt the plaintext in
any way. As a consequence, the ciphertext is identical to the plaintext. So this class should be used for testing, and never in production code.</p>
<p>By contract, the <code>NullCipher</code> class provides an "identity cipher" - one that does not transform or encrypt the plaintext in any way. As
a consequence, the ciphertext is identical to the plaintext. So this class should be used for testing, and never in production code.</p>
<h2>Noncompliant Code Example</h2>
<pre>
NullCipher nc = new NullCipher();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ <h2>See</h2>
</li>
<li> <a href="https://cheatsheetseries.owasp.org/cheatsheets/Web_Service_Security_Cheat_Sheet.html#user-authentication">OWASP Web Service Security
Cheat Sheet</a> </li>
<li> <a href="http://cwe.mitre.org/data/definitions/522">MITRE, CWE-522</a> - Insufficiently Protected Credentials </li>
<li> <a href="https://cwe.mitre.org/data/definitions/522">MITRE, CWE-522</a> - Insufficiently Protected Credentials </li>
<li> <a href="https://www.sans.org/top25-software-errors/#cat3">SANS Top 25</a> - Porous Defenses </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
href="https://docs.oracle.com/en/java/javase/13/security/java-api-xml-processing-jaxp-security-guide.html#GUID-88B04BE2-35EF-4F61-B4FA-57A0E9102342">no effect</a> to protect the parser from XXE attacks but helps guard against excessive memory consumption from XML processing. </li>
<li> or it's just an obscur shortcut (it could set ACCESS_EXTERNAL_DTD and ACCESS_EXTERNAL_SCHEMA to "" but without guarantee). </li>
</ul>
<p>When setting <a href="https://docs.oracle.com/javase/7/docs/api/org/xml/sax/XMLReader.html#setEntityResolver(org.xml.sax.EntityResolver)">an entity
resolver</a> to <code>null</code> (eg: <code>setEntityResolver(null)</code>) the parser will use its own resolution, which is unsafe.</p>
<h2>Noncompliant Code Examples</h2>
<p><a href="https://docs.oracle.com/javase/9/docs/api/javax/xml/parsers/DocumentBuilderFactory.html">DocumentBuilderFactory</a> library:</p>
<pre>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ <h2>Compliant Solution</h2>
return strings;
}
}

</pre>
<h2>See</h2>
<ul>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<p> A cookie's domain specifies which websites should be able to read it. Left blank, browsers are supposed to only send the cookie to sites that
<p>A cookie's domain specifies which websites should be able to read it. Left blank, browsers are supposed to only send the cookie to sites that
exactly match the sending domain. For example, if a cookie was set by <em>lovely.dream.com</em>, it should only be readable by that domain, and not by
<em>nightmare.com</em> or even <em>strange.dream.com</em>. If you want to allow sub-domain access for a cookie, you can specify it by adding a dot in
front of the cookie's domain, like so: <em>.dream.com</em>. But cookie domains should always use at least two levels.</p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ <h2>Noncompliant Code Example</h2>
</pre>
<h2>Exceptions</h2>
<p>This rule ignores instances of assigning <code>this</code> directly to a <code>static</code> field of the same class because that case is covered
by {rule:java:S3010}.</p>
by {rule:java:S3010} .</p>
<h2>See</h2>
<ul>
<li> <a href="https://wiki.sei.cmu.edu/confluence/x/iDdGBQ">CERT, TSM01-J.</a> - Do not let the this reference escape during object construction
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,6 @@ <h2>Compliant Solution</h2>
</pre>
<h2>See</h2>
<ul>
<li> <a href="https://www.securecoding.cert.org/confluence/x/wQA1">CERT, FIO47-C.</a> - Use valid format strings </li>
<li> <a href="https://wiki.sei.cmu.edu/confluence/x/J9YxBQ">CERT, FIO47-C.</a> - Use valid format strings </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ <h2>Ask Yourself Whether</h2>
<li> The directories in the PATH environment variable may be defined by not trusted entities. </li>
</ul>
<p>There is a risk if you answered yes to this question.</p>
<p> </p>
<h2>Recommended Secure Coding Practices</h2>
<p>Fully qualified/absolute path should be used to specify the OS command to execute.</p>
<h2>Sensitive Code Example</h2>
Expand All @@ -15,6 +14,7 @@ <h2>Sensitive Code Example</h2>
<pre>
Runtime.getRuntime().exec("make"); // Sensitive
Runtime.getRuntime().exec(new String[]{"make"}); // Sensitive

ProcessBuilder builder = new ProcessBuilder("make"); // Sensitive
builder.command("make"); // Sensitive
</pre>
Expand All @@ -23,9 +23,11 @@ <h2>Compliant Solution</h2>
<pre>
Runtime.getRuntime().exec("/usr/bin/make"); // Compliant
Runtime.getRuntime().exec(new String[]{"~/bin/make"}); // Compliant

ProcessBuilder builder = new ProcessBuilder("./bin/make"); // Compliant
builder.command("../bin/make"); // Compliant
builder.command(Arrays.asList("..\bin\make", "-j8")); // Compliant

builder = new ProcessBuilder(Arrays.asList(".\make")); // Compliant
builder.command(Arrays.asList("C:\bin\make", "-j8")); // Compliant
builder.command(Arrays.asList("\\SERVER\bin\make")); // Compliant
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ <h2>See</h2>
<ul>
<li> <a href="https://www.owasp.org/index.php/Top_10-2017_A6-Security_Misconfiguration">OWASP Top 10 2017 Category A6</a> - Security
Misconfiguration </li>
<li> <a href="http://cwe.mitre.org/data/definitions/330.html">MITRE, CWE-330</a> - Use of Insufficiently Random Values </li>
<li> <a href="http://cwe.mitre.org/data/definitions/332.html">MITRE, CWE-332</a> - Insufficient Entropy in PRNG </li>
<li> <a href="http://cwe.mitre.org/data/definitions/336.html">MITRE, CWE-336</a> - Same Seed in Pseudo-Random Number Generator (PRNG) </li>
<li> <a href="http://cwe.mitre.org/data/definitions/337.html">MITRE, CWE-337</a> - Predictable Seed in Pseudo-Random Number Generator (PRNG) </li>
<li> <a href="https://cwe.mitre.org/data/definitions/330.html">MITRE, CWE-330</a> - Use of Insufficiently Random Values </li>
<li> <a href="https://cwe.mitre.org/data/definitions/332.html">MITRE, CWE-332</a> - Insufficient Entropy in PRNG </li>
<li> <a href="https://cwe.mitre.org/data/definitions/336.html">MITRE, CWE-336</a> - Same Seed in Pseudo-Random Number Generator (PRNG) </li>
<li> <a href="https://cwe.mitre.org/data/definitions/337.html">MITRE, CWE-337</a> - Predictable Seed in Pseudo-Random Number Generator (PRNG) </li>
<li> <a href="https://wiki.sei.cmu.edu/confluence/display/java/MSC63-J.+Ensure+that+SecureRandom+is+properly+seeded">CERT, MSC63J.</a> - Ensure that
SecureRandom is properly seeded </li>
</ul>
Expand Down
Loading

0 comments on commit 51c3ad4

Please sign in to comment.