Kingdom: Code Quality

Poor code quality leads to unpredictable behavior. From a user's perspective that often manifests itself as poor usability. For an attacker it provides an opportunity to stress the system in unexpected ways.

94 items found
Weaknesses
Abstract
The readObject() method within the class calls a function that may be overridden.
Explanation
During deserialization, readObject() acts like a constructor, so object initialization is not complete until this function ends. Therefore when a readObject() function of a Serializable class calls an overridable function, this may provide the overriding method access to the object's state prior to it being fully initialized.

Example 1: The following readObject() function calls a method that can be overridden.


...
private void readObject(final ObjectInputStream ois) throws IOException, ClassNotFoundException {
checkStream(ois);
ois.defaultReadObject();
}

public void checkStream(ObjectInputStream stream){
...
}


Since the function checkStream() and its enclosing class are not final and public, it means that the function can be overridden, which may mean that an attacker may override the checkStream() function in order to get access to the object during deserialization.
References
[1] SER09-J. Do not invoke overridable methods from the readObject() method CERT
[2] EXTEND-5: Limit the extensibility of classes and methods Oracle
[3] SERIAL-3: View deserialization the same as object construction Oracle
[4] Standards Mapping - CIS Azure Kubernetes Service Benchmark 1
[5] Standards Mapping - CIS Amazon Elastic Kubernetes Service Benchmark 4.1
[6] Standards Mapping - CIS Amazon Web Services Foundations Benchmark 1
[7] Standards Mapping - CIS Google Kubernetes Engine Benchmark normal
desc.structural.java.code_correctness_readobject_invokes_overridable_function
Abstract
The readonly keyword enforces the rule that the variable must be initialized as it's declared or in the constructor and cannot be modified anywhere else. This works as expected for value types, however the content of objects and lists are still modifiable even if it is declared as private readonly.
Explanation
Returning a private readonly list variable from a getter-only property allows the calling code to modify the contents of the list, effectively giving the list write access and contradicting the intentions of the programmer who made it private readonly.

Example 1: The following code contains a list _item which is declared as private readonly.

class Order
{
private readonly List<string> _item = new List<string>();
public IEnumerable<string> Item { get { return _item; } }

public Order()
{
/*class initialize */
}

/*some important function......*/
}
References
[1] Standards Mapping - CIS Azure Kubernetes Service Benchmark 1
[2] Standards Mapping - CIS Amazon Elastic Kubernetes Service Benchmark 5
[3] Standards Mapping - CIS Amazon Web Services Foundations Benchmark 1
[4] Standards Mapping - CIS Google Kubernetes Engine Benchmark normal
[5] Standards Mapping - Common Weakness Enumeration CWE ID 398
desc.structural.dotnet.code_correctness_readonly_collection_reference
Abstract
A function breaks the Checks-Effects-Interaction pattern or fails to safeguard against reentrancy attacks.
Explanation
A reentrancy attack occurs when a malicious contract calls back into the calling contract before the first invocation is finished. This happens when the vulnerable contract exposes a function that interacts with external contracts before locally carrying out the effect of the current call. This can lead to the external contract taking over the control flow of the interaction.

If a malicious contract calls the exposed function of a victim that unsafely interacts back with the calling contract, then the attacking contract will receive and process the interaction (via a fallback function for example) and immediately call back the victim's exposed function again in order to enter a call-interact-call loop. This recursive state prevents any further code in the victim to be executed and can lead to partial or total draining of assets or values depending on the nature of the interaction.

The Checks-Effects-Interaction pattern is commonly used in smart contracts to prevent incorrect logic. The pattern designates that the code first check any required conditions, then carry out the related state change (the effect) and finally interact with the external contract in relation to that effect.

Example 1: The following example allows a reentrancy attack by performing a check, interaction, and then effect, not adhering to the Checks-Effects-Interaction pattern.

The code:

1. Checks the balance of the sender (Checks).
2. Sends out Ether to the caller via msg.sender.call.value (Interaction).
3. Performs a state change by decreasing the balance of the sender (Effects).


function withdraw(uint amount) public{
if (credit[msg.sender] >= amount) {
require(msg.sender.call.value(amount)());
credit[msg.sender]-=amount;
}
}


The code in Example 1 breaks the Checks-Effects-Interaction pattern by doing Checks-Interaction-Effects instead. This can lead to reentrancy if an attacking smart contract receives the Ether in a fallback function and immediately calls back withdraw, creating a recursive situation where Ether is drained because the line of code that decreases the balance (aka the Effect) never gets executed.
References
[1] Enterprise Ethereum Alliance External Calls and Re-entrancy
[2] Standards Mapping - CIS Azure Kubernetes Service Benchmark 4
[3] Standards Mapping - CIS Amazon Elastic Kubernetes Service Benchmark 4
[4] Standards Mapping - CIS Amazon Web Services Foundations Benchmark 3
[5] Standards Mapping - CIS Google Kubernetes Engine Benchmark normal
[6] Standards Mapping - Common Weakness Enumeration CWE ID 841
[7] Standards Mapping - Smart Contract Weakness Classification SWC-107
desc.structural.solidity.swc107
Abstract
A program capable of creating a circular link in a data structure can cause stack exhaustion when the data structure is processed recursively.
Explanation
Using recursion is a staple for creating and managing linked data structures. Recursion also runs the risk of processing indefinitely if the data incorporates a circular link, which in turn will exhaust the stack and crash the program.

Example 1: The following code snippet demonstrates this vulnerability using Apache Log4j2.

Marker child = MarkerManager.getMarker("child");
Marker parent = MarkerManager.getMarker("parent");

child.addParents(parent);
parent.addParents(child);

String toInfinity = child.toString();


When child calls toString(), which includes a recursive processing method, it triggers a stack overflow exception (stack exhaustion). This exception occurs due to the circular link between child and parent.
References
[1] DOS-1: Beware of activities that may use disproportionate resources Oracle
[2] Standards Mapping - CIS Azure Kubernetes Service Benchmark 1
[3] Standards Mapping - CIS Amazon Elastic Kubernetes Service Benchmark 2
[4] Standards Mapping - CIS Amazon Web Services Foundations Benchmark 2
[5] Standards Mapping - CIS Google Kubernetes Engine Benchmark normal
[6] Standards Mapping - Common Weakness Enumeration CWE ID 674
[7] Standards Mapping - Payment Card Industry Software Security Framework 1.2 Control Objective C.3.3 - Web Software Attack Mitigation
desc.controlflow.java.code_correctness_stack_exhaustion
Abstract
Comparing a floating-point value with a String object is unreliable and should not be done.
Explanation
In order to compare a floating-point value to a String object, it must first be changed into a String object, typically via a function such as Double.toString(). Depending on the type and value of the floating-point variable, when converted to a String object, it could be "NaN", "Infinity", "-Infinity", have a certain amount of trailing decimal places containing zeroes, or may contain an exponent field. If converted to a hexadecimal String, the representation may differ greatly as well.

Example 1: The following compares a floating-point variable with a String.


...
int initialNum = 1;
...
String resultString = Double.valueOf(initialNum/10000.0).toString();
if (s.equals("0.0001")){
//do something
...
}
...
References
[1] NUM11-J. Do not compare or inspect the string representation of floating-point values CERT
[2] Standards Mapping - CIS Azure Kubernetes Service Benchmark 1
[3] Standards Mapping - CIS Amazon Elastic Kubernetes Service Benchmark 4
[4] Standards Mapping - CIS Amazon Web Services Foundations Benchmark 1
[5] Standards Mapping - CIS Google Kubernetes Engine Benchmark normal
[6] Standards Mapping - Common Weakness Enumeration CWE ID 398
desc.dataflow.java.code_correctness_string_comparison_of_float
Abstract
An attempt was made to use one of the following accounts to connect to the database: admin, administrator, guest, root, or sa.
Explanation
Windows Azure SQL Database supports only SQL Server Authentication. Windows Authentication (integrated security) is not supported. Users must provide credentials (login and password) every time they connect to Windows Azure SQL Database. Per Microsoft Windows Azure SQL Database General Guidelines and Limitations, the following account names are not available: admin, administrator, guest, root, sa.
References
[1] Security Guidelines and Limitations (Windows Azure SQL Database)
[2] Windows Azure SQL Database Concepts
[3] Transact-SQL Support (Windows Azure SQL Database)
[4] Development Considerations in Windows Azure SQL Database
[5] Managing Databases and Logins in Windows Azure SQL Database
[6] Configure and manage Azure AD authentication with Azure SQL
[7] How to: Connect to Windows Azure SQL Database Using sqlcmd
[8] Copying Databases in Windows Azure SQL Database
[9] Data Types (Windows Azure SQL Database)
[10] Deprecated Database Engine Features in SQL Server 2012
[11] EXECUTE AS (Transact-SQL)
[12] Security Statements
[13] System Stored Procedures (Windows Azure SQL Database)
[14] Guidelines and Limitations (Windows Azure SQL Database)
[15] General Guidelines and Limitations (Windows Azure SQL Database)
[16] Standards Mapping - CIS Azure Kubernetes Service Benchmark 1
[17] Standards Mapping - CIS Amazon Elastic Kubernetes Service Benchmark 1
[18] Standards Mapping - CIS Amazon Web Services Foundations Benchmark 3
[19] Standards Mapping - CIS Google Kubernetes Engine Benchmark confidentiality
[20] Standards Mapping - CIS Kubernetes Benchmark partial
[21] Standards Mapping - Common Weakness Enumeration CWE ID 272
[22] Standards Mapping - Common Weakness Enumeration Top 25 2023 [22] CWE ID 269
[23] Standards Mapping - DISA Control Correlation Identifier Version 2 CCI-000381, CCI-002233, CCI-002235
[24] Standards Mapping - General Data Protection Regulation (GDPR) Access Violation
[25] Standards Mapping - NIST Special Publication 800-53 Revision 4 AC-6 Least Privilege (P1)
[26] Standards Mapping - NIST Special Publication 800-53 Revision 5 AC-6 Least Privilege
[27] Standards Mapping - OWASP Top 10 2021 A01 Broken Access Control
[28] Standards Mapping - OWASP Application Security Verification Standard 4.0 1.4.3 Access Control Architectural Requirements (L2 L3), 10.2.2 Malicious Code Search (L2 L3)
[29] Standards Mapping - OWASP Mobile 2024 M1 Improper Credential Usage
[30] Standards Mapping - Payment Card Industry Data Security Standard Version 1.2 Requirement 7.1.1
[31] Standards Mapping - Payment Card Industry Data Security Standard Version 2.0 Requirement 7.1.1
[32] Standards Mapping - Payment Card Industry Data Security Standard Version 3.0 Requirement 7.1.2
[33] Standards Mapping - Payment Card Industry Data Security Standard Version 3.2 Requirement 7.1.2
[34] Standards Mapping - Payment Card Industry Data Security Standard Version 3.2.1 Requirement 7.1.2
[35] Standards Mapping - Payment Card Industry Data Security Standard Version 3.1 Requirement 7.1.2
[36] Standards Mapping - Payment Card Industry Data Security Standard Version 4.0 Requirement 7.2.2
[37] Standards Mapping - Payment Card Industry Software Security Framework 1.0 Control Objective 5.3 - Authentication and Access Control
[38] Standards Mapping - Payment Card Industry Software Security Framework 1.1 Control Objective 5.3 - Authentication and Access Control
[39] Standards Mapping - Payment Card Industry Software Security Framework 1.2 Control Objective 5.3 - Authentication and Access Control, Control Objective C.2.1.2 - Web Software Access Controls
[40] Standards Mapping - Security Technical Implementation Guide Version 3.1 APP3500 CAT II
[41] Standards Mapping - Security Technical Implementation Guide Version 3.4 APP3500 CAT II
[42] Standards Mapping - Security Technical Implementation Guide Version 3.5 APP3500 CAT II
[43] Standards Mapping - Security Technical Implementation Guide Version 3.6 APP3500 CAT II
[44] Standards Mapping - Security Technical Implementation Guide Version 3.7 APP3500 CAT II
[45] Standards Mapping - Security Technical Implementation Guide Version 3.9 APP3500 CAT II
[46] Standards Mapping - Security Technical Implementation Guide Version 3.10 APP3500 CAT II
[47] Standards Mapping - Security Technical Implementation Guide Version 4.1 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[48] Standards Mapping - Security Technical Implementation Guide Version 4.2 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[49] Standards Mapping - Security Technical Implementation Guide Version 4.3 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[50] Standards Mapping - Security Technical Implementation Guide Version 4.4 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[51] Standards Mapping - Security Technical Implementation Guide Version 4.5 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[52] Standards Mapping - Security Technical Implementation Guide Version 4.6 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[53] Standards Mapping - Security Technical Implementation Guide Version 4.7 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[54] Standards Mapping - Security Technical Implementation Guide Version 4.8 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[55] Standards Mapping - Security Technical Implementation Guide Version 4.9 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[56] Standards Mapping - Security Technical Implementation Guide Version 4.10 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[57] Standards Mapping - Security Technical Implementation Guide Version 4.11 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[58] Standards Mapping - Security Technical Implementation Guide Version 5.1 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[59] Standards Mapping - Security Technical Implementation Guide Version 5.2 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[60] Standards Mapping - Security Technical Implementation Guide Version 5.3 APSC-DV-000500 CAT II, APSC-DV-000510 CAT I, APSC-DV-001500 CAT II
[61] Standards Mapping - Web Application Security Consortium Version 2.00 Insufficient Authorization (WASC-02)
[62] Standards Mapping - Web Application Security Consortium 24 + 2 Insufficient Authorization
desc.structural.sql.code_quality_database_authentication_use_of_restricted_accounts
Abstract
This statement will never be executed.
Explanation
The surrounding code makes it impossible for this statement to ever be executed.

Example: The condition for the second if statement is impossible to satisfy. It requires that the variable s be non-null, while on the only path where s can be assigned a non-null value there is a return statement.


String s = null;

if (b) {
s = "Yes";
return;
}

if (s != null) {
Dead();
}
desc.internal.cpp.dead_code
Abstract
A function defines code with no effect.
Explanation
In Solidity, developers can write code that has no effect, which can lead to unexpected behavior or code that does not perform the intended action.

Example 1: The following code tries to update the balance of msg.sender but uses == instead of = to do so, which has no effect.


function deposit(uint amount) public payable {
require(msg.value == amount, 'incorrect amount');
balance[msg.sender] == amount;
}
desc.structural.solidity.swc135