Defect Detection Code Type Expression Issues (CWE-569)

This article continues the series of reviews devoted to detecting vulnerabilities in open-source projects using the AppChecker static code analyzer .

As part of this series, the most common defects in software code that can lead to serious vulnerabilities are considered. In this article we will focus on a wide class of defects like "Expression Issues" and consider them with examples in PHP and Java.

In the international classification of CWE, this type of defect is known as CWE-569: Expression Issues. It includes various errors in logical expressions in the program code. A special case of a defect of this class is the “Assignment instead of comparison” defect.

Assignment instead of comparison (CWE-481: Assigning instead of Comparing) is a defect, as the name implies, namely, that instead of a comparison operator, an assignment operator is used in the program code. Such an error in the code leads to incorrect operation of the program and can lead to a serious security breach.

The fear of committing such errors brought about the emergence of the so-called Yoda notation, requiring the writing of a constant or a function call to the left of the comparison operator: “7 == j” instead of the usual “j == 7”. Let's see the code. For example, such:

pswd=GetText();   if (consthash=hash(pswd)); //  

Suppose that pswd is a variable in which the password entered by the user is stored. In a conditional statement, a hash of a password with a predetermined value should be compared. If these values ​​match, it is assumed that the password matched and the user gets access. However, in this example, instead of the comparison operator “==” there is an assignment operator “=”. In this case, the variable consthash will be assigned the value hash (pswd), and since this operation was successful, the program will consider that the condition is fulfilled, which, in turn, will lead to the authorization of the user, regardless of the password entered.

It would seem that this example is rather artificial and can hardly be found in serious software products. However, analysis of opensource projects has shown that this defect occurs quite often. Usually such defects are caused by the negligence of the developer. Such defects can be detected using signature-heuristic analysis.

It is logical to assume that there is also a reverse defect: comparison instead of assignment. Indeed, the CWE classification defines this type of defect as CWE-482: Comparing instead of Assigning.

Below we give an example of a real code in which, instead of the assignment operation, the comparison operation is used instead of the assignment operation.

 if ($mValueCount == floor($mValueCount)) { ... } else { $mValueCount == floor($mValueCount); ... } 

As can be seen, by the programmer's mistake, the expressions in the condition and the body of the conditional operator are identical, although in the second case there must be an assignment operator. This code fragment is taken from the library for working with documents using PHP - PHPExcel version 1.8.1. We notified the developers of the defect detected using AppChecker, and a patch has already been released, in which this defect has been fixed ( https://github.com/PHPOffice/PHPExcel/pull/710/files ).

Another example of this type of defects was found in the library for working with MP3, which is part of some CMS, in particular wordpress and written in PHP - getID3-1.9.10 :

 if (ord($frame_ownerid) === 0) { $frame_ownerid == ''; } 

As can be seen from this code fragment, a comparison is made in the body of the conditional operator, although according to logic there should be an assignment. We notified the developers about this defect, and they promptly fixed this defect ( https://github.com/JamesHeinrich/getID3/pull/57/files ).

However, the class "Defects in expressions" is not limited to these two defects. Another example is the “Expression is always true” (CWE-571: Expression is always true) and “Expression is always false” defect (CWE-570: Expression is always false). As the name implies, these defects imply the spelling of a condition in a conditional statement, which will always be true / always false, which in turn means that there is no need for the conditional statement in this part of the code.

As an example, consider the Chamilo LMS 1.10.4 open source CMS code snippet written in PHP:

 if ($row['item_type'] != 'dokeos_chapter' || $row['item_type'] != 'dokeos_module') { 

The expression in the conditional operator is always true, since it requires that the same value of 'item_type' be either not equal to 'dokeos_chapter' or not equal to 'dokeos_module'. Thus, the only option for which this expression will be false is when 'item_type' is simultaneously equal to both, in general, different values. The developers were notified of the defect and promptly fixed it ( https://github.com/chamilo/chamilo-lms/pull/1109/files ). As it turned out, the expression should have the operator “&&” instead of “||”.

As a reverse example, consider the open-source e-commerce platform with open source OpenCart 2.2.0.0 :

 if ($chr == 252 && $chr == 253) { 

Obviously, $ chr cannot be equal to 252 and 253 at the same time. The developers have also fixed this defect ( https://github.com/opencart/opencart/pull/4231/files ). As it turned out, the expression should have the operator “||” instead of “&&”.

Also, defects such as “Expression Issues” can be attributed to cases in which both parts of a binary expression are identical (in the particular case this leads to self-attribution). Consider the following Java code snippet:

 if (s2.getClass() != s2.getClass()) return false; 

As can be seen from this code fragment, s2.getClass () is compared to itself, as a result of which the condition will always be false. Most likely this is just a typo programmer, but the case is far from artificial. This example is taken from google sageTV , a cross-platform media management system. The developers were notified of this defect and promptly fixed it ( https://github.com/google/sagetv/commit/93144762681a5f441ad011564ff8309095d9ca31 ).

It is obvious that such defects also do not depend on the programming language. With AppChecker, such defects are detected for all supported languages.

The following PHP example is taken from OpenEMR-4.2.1 :

 if ( !empty($obx24) || !empty($obx24) || !empty($obx25) ) 

As can be seen from this code fragment, the value of $ obx24 is checked twice, although according to the logic in the first case it should be $ obx23. Such carelessness of the developer can lead to serious problems in the program. The developers of OpenEMR agree with this - they promptly fixed this defect when we told them about it ( https://github.com/openemr/openemr/pull/179 ).

Such defects may go unnoticed for a very long time, but the program will not work properly at the same time, and in some cases, as we have already found out, it can lead to security vulnerabilities. It is worth noting that the defect is logical, and therefore independent of the programming language.

This article has reviewed defects such as Expression Issues. Despite the simplicity and seeming banality of such defects, they are quite common in both open-source and commercial projects.

PS>
The free version of AppChecker can be downloaded from our website: https://file.cnpo.ru/index.php/s/o1cLkNrUX4plHMV