Skip to content

Commit

Permalink
Update RSPECs as part of the LayC second migration (batch#6) (#7546)
Browse files Browse the repository at this point in the history
  • Loading branch information
gregory-paidis-sonarsource committed Jul 6, 2023
1 parent 9499b84 commit 8679b2e
Show file tree
Hide file tree
Showing 16 changed files with 144 additions and 107 deletions.
77 changes: 45 additions & 32 deletions analyzers/rspec/cs/S2053.html
Original file line number Diff line number Diff line change
@@ -1,44 +1,57 @@
<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>
public void Hash(string password)
{
var salt = Encoding.UTF8.GetBytes("Hardcoded salt");
var fromHardcoded = new Rfc2898DeriveBytes(password, salt); // Noncompliant, salt is hardcoded
<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 .NET</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">
using System.Security.Cryptography;

salt = Encoding.UTF8.GetBytes(password);
var fromPassword = new Rfc2898DeriveBytes(password, salt); // Noncompliant, password should not be used as a salt as it makes it predictable

var shortSalt = new byte[8];
RandomNumberGenerator.Create().GetBytes(shortSalt);
var fromShort = new Rfc2898DeriveBytes(password, shortSalt); // Noncompliant, salt is too short (should be at least 16 bytes, not 8)
public static void hash(string password)
{
var salt = Encoding.UTF8.GetBytes("salty");
var hashed = new Rfc2898DeriveBytes(password, salt); // Noncompliant
}
</pre>
<h3>Compliant solution</h3>
<pre>
public DeriveBytes Hash(string password)
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
using System.Security.Cryptography;

public static void hash(string password)
{
return new Rfc2898DeriveBytes(password, 16);
var hashed = new Rfc2898DeriveBytes(password, 16);
}
</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>In the case of the code sample, the class automatically takes care of generating a secure salt if none is specified.</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>

55 changes: 43 additions & 12 deletions analyzers/rspec/cs/S2123.html
Original file line number Diff line number Diff line change
@@ -1,26 +1,57 @@
<h2>Why is this an issue?</h2>
<p>A value that is incremented or decremented and then not stored is at best wasted code and at worst a bug.</p>
<h3>Noncompliant code example</h3>
<pre>
public int PickNumber()
<p>When using the <a
href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/arithmetic-operators#postfix-increment-operator">postfix
increment</a> operator, it is important to know that the result of the expression <code>x++</code> is the value <strong>before</strong> the operation
<code>x</code>.</p>
<p>This means that in some cases, the result might not be what you expect:</p>
<ul>
<li> When assigning <code>x++</code> to <code>x</code>, it’s the same as assigning <code>x</code> to itself, since the value is assigned before the
increment takes place </li>
<li> When returning <code>x++</code>, the returning value is <code>x</code>, not <code>x+1</code> </li>
</ul>
<p>The same applies to the postfix and prefix <a
href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/arithmetic-operators#decrement-operator---">decrement</a>
operators.</p>
<h2>How to fix it</h2>
<p>To solve the issue in assignments, eliminate the assignment, since <code>x\++</code> mutates <code>x</code> anyways.</p>
<p>To solve the issue in return statements, consider using the <a
href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/arithmetic-operators#prefix-increment-operator">prefix
increment</a> operator, since it works in reverse: the result of the expression <code>++x</code> is the value <strong>after</strong> the operation,
which is <code>x+1</code>, as one might expect.</p>
<p>The same applies to the postfix and prefix <a
href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/arithmetic-operators#decrement-operator---">decrement</a>
operators.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
int PickNumber()
{
int i = 0;
int j = 0;

i = i++; // Noncompliant; i is still zero

return j++; // Noncompliant; 0 returned
i = i++; // Noncompliant: i is still 0
return j--; // Noncompliant: returns 0
}
</pre>
<h3>Compliant solution</h3>
<pre>
public int PickNumber()
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
int PickNumber()
{
int i = 0;
int j = 0;

i++;
return ++j;
i++; // Compliant: i is incremented to 1
return --j; // Compliant: returns -1
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/arithmetic-operators">Arithmetic
operators (C# reference)</a> </li>
</ul>
<h3>Articles &amp; blog posts</h3>
<ul>
<li> StackOverflow - <a href="https://stackoverflow.com/a/3346729">"What is the difference between i and i in C#?" - Eric Lippert’s answer</a> </li>
</ul>

2 changes: 1 addition & 1 deletion analyzers/rspec/cs/S2123.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@
"ruleSpecification": "RSPEC-2123",
"sqKey": "S2123",
"scope": "All",
"quickfix": "unknown"
"quickfix": "targeted"
}
3 changes: 1 addition & 2 deletions analyzers/rspec/cs/S2259.html
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ <h4>Validated value by Debug.Assert</h4>
}
</pre>
<h4>Validated value by IDE-specific attributes</h4>
<p>Like with <a href="#null-analysis-attribute">nullable analysis attributes</a>, potential <code>null</code> values validated by one of the following
IDE-specific attributes will not raise</p>
<p>Like with null-analysis-attribute, potential <code>null</code> values validated by one of the following IDE-specific attributes will not raise</p>
<h5>Visual Studio</h5>
<ul>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.validatednotnullattribute">ValidatedNotNullAttribute</a> (The attribute is
Expand Down
3 changes: 1 addition & 2 deletions analyzers/rspec/cs/S2995.html
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ <h3>Documentation</h3>
href="https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/how-to-define-value-equality-for-a-type#struct-example">How to define value equality for a class or struct (C# Programming Guide)</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct">Structure types (C#
reference)</a> </li>
<li> {rule:csharpsquid:S3898} - <a href="https://rules.sonarsource.com/csharp/RSPEC-3898/">Value types should implement "IEquatable&lt;T&gt;"</a>
</li>
<li> {rule:csharpsquid:S3898} - Value types should implement "IEquatable&lt;T&gt;" </li>
</ul>

3 changes: 1 addition & 2 deletions analyzers/rspec/cs/S3453.html
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ <h4>Compliant solution</h4>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://rules.sonarsource.com/csharp/RSPEC-1118">{rule:csharpsquid:S1118} - Utility classes should not have public constructors</a>
</li>
<li> {rule:csharpsquid:S1118} - Utility classes should not have public constructors </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle">SafeHandle Class</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/standard/native-interop/pinvoke">Platform Invoke (P/Invoke)</a> </li>
<li> Microsoft Learn - <a
Expand Down
2 changes: 1 addition & 1 deletion analyzers/rspec/cs/S3603.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ <h4>Noncompliant code example</h4>
{
private int age;

[Pure] // Noncompliant. The method makes a state change
[Pure] // Noncompliant: The method makes a state change
void ConfigureAge(int age) =&gt;
this.age = age;
}
Expand Down
30 changes: 9 additions & 21 deletions analyzers/rspec/cs/S3887.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,25 @@ <h2>How to fix it</h2>
collection</a> or remove the <code>readonly</code> field to clarify the behavior.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre>
<pre data-diff-id="1" data-diff-type="noncompliant">
public class MyClass
{
public readonly string[] strings; // Noncompliant

public readonly string[] strings1; // Noncompliant
public readonly string[] strings2; // Noncompliant
public readonly string[] strings3; // Noncompliant
// ...
}
</pre>
<h4>Compliant solution</h4>
<pre>
public class MyClass
{
public string[] strings;

// ...
</pre>
<p>or</p>
<pre>
<pre data-diff-id="1" data-diff-type="compliant">
public class MyClass
{
public readonly ImmutableArray&lt;string&gt; strings;

// ...
</pre>
<p>or</p>
<pre>
public class MyClass
{
private readonly string[] strings;
public string[] strings1; // Compliant: remove readonly modifier
public readonly ImmutableArray&lt;string&gt; strings; // Compliant: use an Immutable collection
private readonly string[] strings; // Compliant: reduced accessibility to private

// ...
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
Expand Down
25 changes: 14 additions & 11 deletions analyzers/rspec/cs/S3927.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@ <h2>Why is this an issue?</h2>
<p>A method is designated a serialization event handler by applying one of the following serialization event attributes:</p>
<ul>
<li> <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.onserializingattribute"><code>System.Runtime.Serialization.OnSerializingAttribute</code></a> </li>
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.onserializingattribute"><code>OnSerializingAttribute</code></a>
</li>
<li> <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.onserializedattribute"><code>System.Runtime.Serialization.OnSerializedAttribute</code></a> </li>
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.onserializedattribute"><code>OnSerializedAttribute</code></a> </li>
<li> <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.ondeserializingattribute"><code>System.Runtime.Serialization.OnDeserializingAttribute</code></a> </li>
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.ondeserializingattribute"><code>OnDeserializingAttribute</code></a>
</li>
<li> <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.ondeserializedattribute"><code>System.Runtime.Serialization.OnDeserializedAttribute</code></a> </li>
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.ondeserializedattribute"><code>OnDeserializedAttribute</code></a>
</li>
</ul>
<p>Serialization event handlers take a single parameter of type <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.streamingcontext"><code>System.Runtime.Serialization.StreamingContext</code></a>,
return <code>void`</code>, and have <code>private</code> visibility.</p>
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.streamingcontext"><code>StreamingContext</code></a>, return
<code>void</code>, and have <code>private</code> visibility.</p>
<p>This rule raises an issue when any of these constraints are not respected.</p>
<h2>How to fix it</h2>
<h3>Code examples</h3>
Expand All @@ -24,19 +27,19 @@ <h4>Noncompliant code example</h4>
public class Foo
{
[OnSerializing]
public void OnSerializing(StreamingContext context) {} // Noncompliant should be private
public void OnSerializing(StreamingContext context) {} // Noncompliant: should be private

[OnSerialized]
int OnSerialized(StreamingContext context) {} // Noncompliant should return void
int OnSerialized(StreamingContext context) {} // Noncompliant: should return void

[OnDeserializing]
void OnDeserializing() {} // Noncompliant should have a single parameter of type StreamingContext
void OnDeserializing() {} // Noncompliant: should have a single parameter of type StreamingContext

[OnSerializing]
public void OnSerializing2&lt;T&gt;(StreamingContext context) {} // Noncompliant should have no type parameters
public void OnSerializing2&lt;T&gt;(StreamingContext context) {} // Noncompliant: should have no type parameters

[OnDeserialized]
void OnDeserialized(StreamingContext context, string str) {} // Noncompliant should have a single parameter of type StreamingContext
void OnDeserialized(StreamingContext context, string str) {} // Noncompliant: should have a single parameter of type StreamingContext
}
</pre>
<h4>Compliant solution</h4>
Expand Down
4 changes: 2 additions & 2 deletions analyzers/rspec/vbnet/S1048.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ <h2>How to fix it</h2>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
Class MyClass
Class Sample
Protected Overrides Sub Finalize()
Throw New NotImplementedException() ' Noncompliant: Finalize method throws an exception
End Sub
End Class
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
Class MyClass
Class Sample
Protected Overrides Sub Finalize()
' Noncompliant: Finalize method does not throw an exception
End Sub
Expand Down
4 changes: 2 additions & 2 deletions analyzers/rspec/vbnet/S2259.html
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ <h4>Validated value by Debug.Assert</h4>
End Sub
</pre>
<h4>Validated value by IDE-specific attributes</h4>
<p>Like with <a href="#null-analysis-attribute">nullable analysis attributes</a>, potential <code>Nothing</code> values validated by one of the
following IDE-specific attributes will not raise</p>
<p>Like with null-analysis-attribute, potential <code>Nothing</code> values validated by one of the following IDE-specific attributes will not
raise</p>
<h5>Visual Studio</h5>
<ul>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.validatednotnullattribute">ValidatedNotNullAttribute</a> (The attribute is
Expand Down
2 changes: 1 addition & 1 deletion analyzers/rspec/vbnet/S3453.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ <h4>Compliant solution</h4>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://rules.sonarsource.com/csharp/RSPEC-1118">{rule:vbnet:S1118} - Utility classes should not have public constructors</a> </li>
<li> {rule:vbnet:S1118} - Utility classes should not have public constructors </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle">SafeHandle Class</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/standard/native-interop/pinvoke">Platform Invoke (P/Invoke)</a> </li>
<li> Microsoft Learn - <a
Expand Down
12 changes: 7 additions & 5 deletions analyzers/rspec/vbnet/S3603.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ <h2>Why is this an issue?</h2>
attribute indicates that the method doesn’t make any visible state changes. Therefore, a <code>Pure</code> method should return a result otherwise it
indicates a no-operation call.</p>
<p>Using <code>Pure</code> on a <code>void</code> method is either by mistake or the method is not doing a meaningful task.</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">
Class Person
Private age As Integer

&lt;Pure&gt; ' Noncompliant. The method makes a state change
&lt;Pure&gt; ' Noncompliant: The method makes a state change
Private Sub ConfigureAge(ByVal age As Integer)
Me.age = age
End Sub
Expand All @@ -20,8 +22,8 @@ <h3>Noncompliant code example</h3>

End Class
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
Class Person
Private age As Integer

Expand Down
Loading

0 comments on commit 8679b2e

Please sign in to comment.