Use HTTPS for Wikimedia query-help links

[miachillgood]

Update 98 Wikipedia and Wikibooks references in non-Python query-help files from HTTP to HTTPS.

This is a scheme-only change: paths, fragments, and link text remain unchanged. The Python query-help subset remains outside this PR except for the Python copy of FLinesOfDuplicatedCodeCommon.inc.qhelp, which must match the synchronized Java and JavaScript copies.

Validation:

• git diff --check
• Confirmed representative Wikipedia and Wikibooks HTTPS targets return HTTP 200
• Confirmed no matching HTTP Wikimedia links remain in the changed language query-help scope
• Confirmed the Java, JavaScript, and Python FLinesOfDuplicatedCodeCommon.inc.qhelp files are synchronized

Part of #5163.

[geoffw0]

LGTM.

(I checked a handful of the new URLs manually, and had Copilot look for unusual cases)

[geoffw0]

This change is in a comment, but it's not doing any harm to update it along with the others.

[github-actions]

QHelp previews:

cpp/ql/src/Best Practices/Exceptions/CatchingByValue.qhelp

Catching by value

Catching an exception by value will create a new local variable which is a copy of the originally thrown object. Creating the copy is slightly wasteful, but not catastrophic. More worrisome is the fact that if the type being caught is a strict supertype of the originally thrown type, then the copy might not contain as much information as the original exception.

Recommendation

The parameter to the catch block should have its type changed from T to T& or const T&.

Example

voidbad() {
try {
/* ... */
}
catch(std::exception a_copy_of_the_thrown_exception) {
// Do something with a_copy_of_the_thrown_exception
}
}
voidgood() {
try {
/* ... */
}
catch(const std::exception& the_thrown_exception) {
// Do something with the_thrown_exception
}
}

References

• C++ FAQ: What should I throw?, What should I catch?.
• Wikibooks: Throwing objects.

cpp/ql/src/Best Practices/Exceptions/ThrowingPointers.qhelp

Throwing pointers

As C++ is not a garbage collected language, exceptions should not be dynamically allocated. Dynamically allocating an exception puts an onus on every catch site to ensure that the memory is freed.

As a special case, it is permissible to throw anything derived from Microsoft MFC's CException class as a pointer. This is for historical reasons; modern code and modern frameworks should not throw pointer values.

Recommendation

The new keyword immediately following the throw keyword should be removed. Any catch sites which previously caught the pointer should be changed to catch by reference or const reference.

Example

voidbad() {
thrownewstd::exception("This is how not to throw an exception");
}
voidgood() {
throwstd::exception("This is how to throw an exception");
}

References

• C++ FAQ: What should I throw?, What should I catch?.
• Wikibooks: Throwing objects.

cpp/ql/src/Best Practices/Likely Errors/OffsetUseBeforeRangeCheck.qhelp

Array offset used before range check

The program contains an and-expression where the array access is defined before the range check. Consequently the array is accessed without any bounds checking. The range check does not protect the program from segmentation faults caused by attempts to read beyond the end of a buffer.

Recommendation

Update the and-expression so that the range check precedes the array offset. This will ensure that the bounds are checked before the array is accessed.

Example

The find function can read past the end of the buffer pointed to by str if start is longer than or equal to the length of the buffer (or longer than len, depending on the contents of the buffer).

intfind(intstart, char*str, chargoal)
{
intlen=strlen(str);
//Potential buffer overflowfor (inti=start; str[i] !=0&&i<len; i++) { if (str[i] ==goal)
returni; }
return-1;
}
intfindRangeCheck(intstart, char*str, chargoal)
{
intlen=strlen(str);
//Range check protects against buffer overflowfor (inti=start; i<len&&str[i] !=0 ; i++) {
if (str[i] ==goal)
returni; }
return-1;
}

Update the and-expression so that the range check precedes the array offset (for example, the findRangeCheck function).

References

• cplusplus.com: C++: array.
• Wikipedia: Bounds checking.
• Common Weakness Enumeration: CWE-120.
• Common Weakness Enumeration: CWE-125.

cpp/ql/src/Best Practices/Likely Errors/Slicing.qhelp

Slicing

This query finds assignments of a non-reference instance of a derived type to a variable of the base type where the derived type has more fields than the base. These assignments slice off all the fields added by the derived type, and can cause unexpected state when accessed as the derived type.

Recommendation

Change the type of the variable at the left-hand side of the assignment to the subclass.

Example

staticint idctr = 0;
//Basic connection with idclassConnection {
public:int connId;
virtualvoidprint_info() {
cout << "id: " << connId << "\n";
}
Connection() {
connId = idctr++;
}
};
//Adds counters, and an overriding print_infoclassMeteredConnection : publicConnection {
public:int txCtr;
int rxCtr;
MeteredConnection() {
txCtr = 0;
rxCtr = 0;
}
virtualvoidprint_info() {
cout << "id: " << connId << "\n" << "tx/rx: " << txCtr << "/" << rxCtr << "\n";
}
};
intmain(int argc, char* argv[]) {
Connection conn;
MeteredConnection m_conn;
Connection curr_conn = conn;
curr_conn.print_info();
curr_conn = m_conn; //Wrong: Derived MetricConnection assigned to Connection //variable, will slice off the counters and the overriding print_info
curr_conn.print_info(); //Will not print the counters.
Connection* curr_pconn = &conn;
curr_pconn->print_info();
curr_pconn = &m_conn; //Correct: Pointer assigned to address of the MetricConnection. //Counters and virtual functions remain intact.
curr_pconn->print_info(); //Will call the correct method MeteredConnection::print_info
}

References

• Wikipedia: Object slicing.
• DevX.com: Slicing in C++.

cpp/ql/src/Best Practices/Magic Constants/MagicConstantsNumbers.qhelp

Magic numbers

A magic number is a numeric literal (for example, 8080, 2048) that is used in the middle of a block of code without explanation. It is considered good practice to avoid magic numbers by assigning the numbers to named constants and using the named constants instead. The reasons for this are twofold:

1. A number in isolation can be inexplicable to later programmers, whereas a named constant (such as MAX_GUESTS) is more readily understood.
2. Using the same named constant in many places, makes the code much easier to update if the requirements change (for example, one more guest is permitted).
   This rule finds magic numbers for which there is no pre-existing named constant (for example, the line marked (4) below).

Recommendation

Consider creating a const or a macro to encapsulate the literal, then replace all the relevant occurrences in the code.

Example

voidsanitize(Fields[] record) {
//The number of fields here can be put in a constfor (fieldCtr = 0; field < 7; field++) {
sanitize(fields[fieldCtr]);
}
}
#defineNUM_FIELDS7voidprocess(Fields[] record) {
//This avoids using a magic constant by using the macro insteadfor (fieldCtr = 0; field < NUM_FIELDS; field++) {
process(fields[fieldCtr]);
}
}

References

• Magic number (Wikipedia)
• Mats Henricson and Erik Nyquist, Industrial Strength C++, published by Prentice Hall PTR (1997). Chapter 5: Object Life Cycle, Rec 5.4 (PDF).
• DCL06-C. Use meaningful symbolic constants to represent literal values

cpp/ql/src/Best Practices/Magic Constants/MagicConstantsString.qhelp

Magic strings

A magic string is a string literal (for example, "SELECT", "127.0.0.1") that is used in the middle of a block of code without explanation. It is considered good practice to avoid magic strings by assigning the strings to named constants and using the named constants instead. The reasons for this are twofold:

1. A string in isolation can be inexplicable to later programmers, whereas a named constant (such as SMTP_HELO) is more readily understood.
2. Using the same named constant in many places, makes the code much easier to update if the requirements change (for example, a protocol is updated).
   This rule finds magic strings for which there is no pre-existing named constant.

Recommendation

Consider replacing the magic string with a new named constant.

References

• Magic string (Wikipedia)
• Mats Henricson and Erik Nyquist, Industrial Strength C++, published by Prentice Hall PTR (1997). Chapter 5: Object Life Cycle, Rec 5.4 (PDF).
• DCL06-C. Use meaningful symbolic constants to represent literal values

cpp/ql/src/Best Practices/RuleOfThree.qhelp

Rule of three

This query finds classes that define a destructor, a copy constructor, or a copy assignment operator, but not all three of them. The compiler generates default implementations for these functions, and since they deal with similar concerns it is likely that if the default implementation of one of them is not satisfactory, then neither are those of the others.

The query flags any such class with a warning, and also display the list of generated warnings in the result view.

Recommendation

Explicitly define the missing functions.

References

• Wikipedia: Rule of three (C++ programming)

cpp/ql/src/Best Practices/RuleOfTwo.qhelp

Inconsistent definition of copy constructor and assignment ('Rule of Two')

This rule finds classes that define a copy constructor or a copy assignment operator, but not both of them. The compiler generates default implementations for these functions, and since they deal with similar concerns it is likely that if the default implementation of one of them is not satisfactory, then neither is that of the other.

When a class defines a copy constructor or a copy assignment operator, but not both, this can cause unexpected behavior. The object initialization (that is, Class c1 = c2) may behave differently from object assignment (that is, c1 = c2).

Recommendation

First, consider whether the user-defined member needs to be explicitly defined at all. If no user-defined copy constructor is provided for a class, the compiler will always attempt to generate a public copy constructor that recursively invokes the copy constructor of each field. If the existing user-defined copy constructor does exactly the same, it is most likely beneficial to delete it. The compiler-generated version may be more efficient, and it does not need to be manually maintained as fields are added and deleted.

If the user-defined member does need to exist, the other corresponding member should be defined too. It can be defined as defaulted (using = default) if the compiler-generated implementation is acceptable, or it can be defined as deleted (using = delete) if it should never be called.

Example

classC {
private:
Other* other = NULL;
public:C(const C& copyFrom) {
Other* newOther = newOther();
*newOther = copyFrom.other;
this->other = newOther;
}
//No operator=, by default will just copy the pointer other, will not create a new object
};
classD {
Other* other = NULL;
public:
D& operator=(D& rhs) {
Other* newOther = newOther();
*newOther = rhs.other;
this->other = newOther;
return *this;
}
//No copy constructor, will just copy the pointer other and not create a new object
};

References

• Rule of Three [Wikipedia]
• The Law of The Big Two
• cppreference.com: copy constructor and copy assignment

cpp/ql/src/Documentation/DocumentApi.qhelp

Undocumented API function

Functions that are called from lots of different places are usually important, and justify having documentation written for them. In particular, if a function is defined in a file, and is called from at least two other files, then the function should probably be documented.

As an exception, because their purpose is usually obvious, it is not necessary to document constructors, destructors, implementations of operator=, or functions with fewer than five lines of code.

Recommendation

Add comments to document the purpose of the function. In particular, ensure that the public API of the function is carefully documented. This reduces the chance that a future change to the function will introduce a defect by changing the API and breaking the expectations of the calling functions.

References

• C++ Programming Wikibook: Comments
• Wikipedia: Need for comments

cpp/ql/src/Documentation/FixmeComments.qhelp

FIXME comment

The indicated comment is a FIXME comment. FIXME comments are often used to indicate code that does not work correctly or that may not work in all supported environments. This may be necessary during the implementation of new functionality but FIXME comments should not be present in stable code. Any FIXME comments should be reviewed and the code improved as soon as possible to avoid the accumulation of partially implemented features.

Recommendation

Fix the functionality indicated by the comment. If the comment no longer applies, delete it to avoid confusion.

Example

intisEven(int n) {
//FIXME: Is only correct for small values of nreturn n == 0 || n == 2;
}

References

• Wikipedia: Comment tags
• The case against TODO (and FIXME)
• Common Weakness Enumeration: CWE-546.

cpp/ql/src/Documentation/TodoComments.qhelp

TODO comment

The indicated comment is a TODO comment. TODO comments are often used to indicate code that is incomplete. This may be necessary during the implementation of new functionality but TODO comments should not be present in stable code. Any TODO comments should be reviewed and the code completed as soon as possible to avoid the accumulation of partially implemented features.

Recommendation

Implement the functionality indicated by the comment. If the comment no longer applies, delete it to avoid confusion.

Example

intisOdd(int n) {
//TODO: Works only for positive n. Need to check if negative n is valid inputreturn (n % 2) == 1;
}

References

• Wikipedia: Comment tags
• TODO or not TODO
• The case against TODO
• Common Weakness Enumeration: CWE-546.

cpp/ql/src/Documentation/UncommentedFunction.qhelp

Poorly documented large function

This rule finds large functions that have too few comment lines. Documentation becomes more important as a function becomes more complex, and a lack of documentation makes it harder to maintain.

Recommendation

Add comments to document the purpose of the function. Large, complex functions in particular require detailed documentation, not only because they are harder to understand, but the process of documentation may reveal that the function could be split into smaller, more cohesive functions.

References

• C++ Programming, Coding style conventions
• Wikipedia: Need for comments

cpp/ql/src/Header Cleanup/Cleanup-DuplicateIncludeGuard.qhelp

Duplicate include guard

A common pattern in header files is to use pre-processor directives to guard a header file against being processed more than once per translation unit. This practice is intended to prevent compilation errors. However, pre-processor include guards are prone to human error themselves because each include guard must be assigned a unique macro name to function correctly. If two header files share the same guard macro, the compiler may unexpectedly skip the second file it encounters, leading to compilation errors or configuration bugs.

The query will flag the pre-processor #ifndef directive at the beginning of any include guard that matches another include guard in the project. Browsing the list of results you will be able to find the other directive(s) which use the same macro.

Recommendation

First decide whether the duplicate include guard is dangerous. A duplicate include guard may cause the header file to be skipped over when it shouldn't be, but occasionally this design is used on purpose to 'override' an existing header file.

To address the issue, rename the macros used by all but one instance of the duplicate include guard. Remember to change both the #ifndef and the #define directive to use the new macro name. Alternatively, consider using the #pragma once directive to prevent multiple inclusion without the need to define unique macros.

Example

Here's an example of two header files that have accidentally been given the same include guard macro. To fix the issue, rename both occurrences of the macro in the second file, for example to ANOTHER_HEADER_FILE_H.

// header_file.h
#ifndef HEADER_FILE_H
#defineHEADER_FILE_H// ...
#endif// HEADER_FILE_H

// another_header_file.h
#ifndef HEADER_FILE_H // should be ANOTHER_HEADER_FILE_H
#defineHEADER_FILE_H// should be ANOTHER_HEADER_FILE_H// ...
#endif// HEADER_FILE_H

References

• Wikipedia: Include guard

cpp/ql/src/Likely Bugs/Arithmetic/BadCheckOdd.qhelp

Bad check for oddness

This rule finds code that uses x % 2 == 1 to check whether a number x is odd, which does not work for negative numbers. Applying % to negative numbers produces negative results. For example, (-5) % 2 equals -1, not 1. As a result, this check incorrectly considers all negative numbers as even.

Recommendation

Consider using x % 2 != 0 or (x & 1) == 1 instead.

References

• MSDN Library: Multiplicative Operators and the Modulus Operator.
• Wikipedia: Modulo Operation - Common pitfalls.

cpp/ql/src/Likely Bugs/Likely Typos/AssignWhereCompareMeant.qhelp

Assignment where comparison was intended

This rule finds uses of the assignment operator = in places where the equality operator == would make more sense. This is a very common mistake in C and C++, because of the similarity of the = and the == operator, and the fact that the if statement accepts a condition with an integral type, instead of limiting it to just the bool type.

The rule flags every occurrence of an assignment in a position where its result is interpreted as a truth value. An assignment is only flagged if its right hand side is a compile-time constant.

Recommendation

Check to ensure that the flagged expressions are not typos. If an assignment is really intended to be treated as a truth value, it may be better to surround it with parentheses.

Example

if(p = NULL) { //most likely == was intended. Otherwise it evaluates to the value//of the rhs of the assignment (which is NULL)
...
}

References

• Tutorialspoint - The C++ Programming Language: Operators in C++
• Wikipedia: Operators in C and C++
• Common Weakness Enumeration: CWE-481.

cpp/ql/src/Likely Bugs/Likely Typos/CompareWhereAssignMeant.qhelp

Comparison where assignment was intended

This rule finds uses of the equality operator == in places where the assignment operator = would make more sense. This is a common mistake in C and C++, because of the similarity of the = and the == operator, and the fact that expressions are valid as top-level statements.

The rule flags every occurrence of an equality operator in a position where its result is discarded.

Recommendation

Check to ensure that the flagged expressions are not typos. If the result of an equality test is really intended to be discarded, it should be explicitly cast to void.

Example

int x;
x == 4; // most likely = was intended. Otherwise this statement has no effect.
...

References

• Tutorialspoint - The C++ Programming Language: Operators in C++
• Wikipedia: Operators in C and C++
• Common Weakness Enumeration: CWE-482.

cpp/ql/src/Likely Bugs/OO/IncorrectConstructorDelegation.qhelp

Incorrect constructor delegation

Prior to C++11, there is no mechanism for a constructor to delegate part of the object initialization to another, although other languages provide this feature. Consequently, any instance where a constructor call appears in the body of a constructor without being used is suspect.

Recommendation

The rule flags constructor calls in constructors which are not used in some way. This is usually a misguided attempt to share some initialization code between multiple constructors, or to provide sensible defaults for some constructor parameters. The effect of a flagged expression would be to initialize an instance of the current class on the stack, and then let it go out of scope at the end of the constructor call.

There are several ways to address the underlying issue of sharing initialization code, and the most appropriate needs to picked in each case. Roughly speaking, the options are:

• Introduce actual default values for the constructor parameters.
• Duplicate the initialization code in each constructor.
• Factor out the initialization code into a member function that is called from each constructor.
• If your compiler supports it, use C++11's constructor delegation feature.

Example

classCircle {
private:double m_x;
double m_y;
double m_radius;
double m_area;
public:// Real constructor:Circle(double x, double y, double radius) :
m_x(x), m_y(y), m_radius(radius)
{
m_area = 3.14159 * m_radius * m_radius;
}
Circle() {
// WRONG: Attempt to define the unit circle by default fails.Circle(0, 0, 1);
}
};

References

• Dr Dobb's Journal: Delegating constructors?
• Wikipedia: Object construction improvement in C++11.

cpp/ql/src/Metrics/Files/FCommentRatio.qhelp

Percentage of comments

This metric measures the percentage of lines in a file that contain a comment or are part of a multi-line comment.

Having a low percentage of comments is an indication that a file does not have sufficient documentation. Undocumented code is hard to understand, modify, and reuse.

Recommendation

Add documentation to files with a low percentage of comments. It is most useful to start documenting the public functions first.

References

• C++ Programming, Coding style conventions
• Wikipedia: Need for comments

cpp/ql/src/Metrics/Files/FCyclomaticComplexity.qhelp

Average cyclomatic complexity of files

This metric measures the average cyclomatic complexity of the functions in a file.

The cyclomatic complexity of a function is the number of linearly independent execution paths through that function. A path is linearly independent path if it differs from all other paths by at least one node. Straight-line code therefore has a cyclomatic complexity of one, while branches, switches and loops increase cyclomatic complexity.

Functions with a high cyclomatic complexity are typically hard to understand and test. By extension, files whose functions have a high average cyclomatic complexity are problematic, and usually would benefit from refactoring.

As a concrete example, consider the following function:

intf(int i, int j) {
// startint result;
if(i % 2 == 0) {
// iEven
result = i + j;
}
else {
// iOddif(j % 2 == 0) {
// jEven
result = i * j;
}
else {
// jOdd
result = i - j;
}
}
return result;
// end
}

The control flow graph for this function is as follows:

The graph shows that the number of linearly independent execution paths through the function, and hence its cyclomatic complexity, is 3. The three paths are:

• start -> iEven -> end
• start -> iOdd -> jEven -> end
• start -> iOdd -> jOdd -> end

Recommendation

Functions with a high cyclomatic complexity should be simplified, for instance by tidying up any complex logic within them or by splitting them into multiple methods using the Extract Method refactoring.

References

• M. Fowler, Refactoring. Addison-Wesley, 1999.
• T. J. McCabe, A Complexity Measure. IEEE Transactions on Software Engineering, SE-2(4), December 1976.
• Dave Thomas, Refactoring as Meta Programming?, in Journal of Object Technology, vol. 4, no. 1, January-February 2005, pp. 7-11.
• Wikipedia: Cyclomatic complexity
• Wikipedia: Code refactoring

cpp/ql/src/Metrics/Files/FLinesOfCode.qhelp

Lines of code in files

This metric measures the number of lines of code in a file. This excludes comments and blank lines.

Having too many lines of code in a file is an indication that it can be split into several files of more manageable size. Generated code is a notable exception to this.

Recommendation

Long files should be examined to see if they can be split into smaller, more cohesive files.

References

• M. Fowler. Refactoring. Addison-Wesley, 1999.
• Wikipedia: Code refactoring
• Refactoring as Meta Programming?

cpp/ql/src/Metrics/Files/FLinesOfComments.qhelp

Lines of comments in files

This metric measures the number of lines of comments in a file.

Files that have too few comments are more likely to be insufficiently documented. Long files that have no comments at all require particular scrutiny, as these are more likely to need explicit documentation in comments.

Recommendation

Files with few to no comments should be examined to see if they require documentation. Particular attention should be given to long files.

References

• C++ Programming, Coding style conventions
• Wikipedia: Need for comments

cpp/ql/src/Metrics/Files/FTodoComments.qhelp

Number of todo/fixme comments per file

This metric measures the number of TODO or FIXME comments in the code.

These comments tend to document points of ambiguity in the software's requirements. Often they refer to corner-cases that are not handled or potential defects. It is therefore very important to monitor such comments and, where appropriate, escalate comments to an defect-tracking system or other means of ensuring that action is taken.

Recommendation

Remove unnecessary TODO/FIXME comments, and those that are no longer relevant. File tickets for the remaining comments in a defect-tracker, or otherwise ensure that someone is responsible for fixing them.

References

• Wikipedia: Comment tags
• TODO or not TODO
• The case against TODO

cpp/ql/src/Metrics/Functions/FunLinesOfComments.qhelp

Lines of comments per function

This metric measures the number of lines of comments in a function.

Functions that have too few comments are more likely to be insufficiently documented. Long, complex functions that have no comments at all require particular scrutiny, as these are more likely to need explicit documentation in comments.

Recommendation

Functions with few to no comments should be examined to see if they require documentation. Particular attention should be given to long, complex functions.

References

• C++ Programming, Coding style conventions
• Wikipedia: Need for comments

cpp/ql/src/Metrics/Functions/FunPercentageOfComments.qhelp

Comment ratio per function

This metric measures the percentage of lines in an function that contain a comment or are part of a multi-line comment.

Having a low comment ratio is an indication that a function does not have sufficient documentation.

Recommendation

Start by adding documentation to functions that are used from other parts of the code. Then document large complex functions, as they benefit most from documentation.

References

• C++ Programming, Coding style conventions
• Wikipedia: Need for comments

cpp/ql/src/Security/CWE/CWE-079/CgiXss.qhelp

CGI script vulnerable to cross-site scripting

Directly writing an HTTP request parameter back to a web page allows for a cross-site scripting vulnerability. The data is displayed in a user's web browser as belonging to one site, but it is provided by some other site that the user browses to. In effect, such an attack allows one web site to insert content in the other one.

For web servers implemented with the Common Gateway Interface (CGI), HTTP parameters are supplied via the QUERY_STRING environment variable.

Recommendation

To guard against cross-site scripting, consider escaping special characters before writing the HTTP parameter back to the page.

Example

In the following example, the bad_server writes a parameter directly back to the HTML page that the user will see. The good_server first escapes any HTML special characters before writing to the HTML page.

voidbad_server() {
char*query=getenv("QUERY_STRING");
puts("<p>Query results for ");
// BAD: Printing out an HTTP parameter with no escapingputs(query);
puts("\n<p>\n");
puts(do_search(query));
}
voidgood_server() {
char*query=getenv("QUERY_STRING");
puts("<p>Query results for ");
// GOOD: Escape HTML characters before adding to a pagechar*query_escaped=escape_html(query);
puts(query_escaped);
free(query_escaped);
puts("\n<p>\n");
puts(do_search(query));
}

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• Wikipedia: Cross-site scripting.
• IETF Tools: The Common Gateway Specification (CGI).
• Common Weakness Enumeration: CWE-79.

cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.qhelp

Use of dangerous function

This rule finds calls to the gets function, which is dangerous and should not be used. See Related rules below for rules that identify other dangerous functions.

The gets function is one of the vulnerabilities exploited by the Internet Worm of 1988, one of the first computer worms to spread through the Internet. The gets function provides no way to limit the amount of data that is read and stored, so without prior knowledge of the input it is impossible to use it safely with any size of buffer.

Recommendation

Replace calls to gets with fgets, specifying the maximum length to copy. This will prevent the buffer overflow.

Example

The following example gets a string from standard input in two ways:

#defineBUFFERSIZE (1024)
// BAD: using getsvoidecho_bad() {
charbuffer[BUFFERSIZE];
gets(buffer);
printf("Input was: '%s'\n", buffer);
}
// GOOD: using fgetsvoidecho_good() {
charbuffer[BUFFERSIZE];
fgets(buffer, BUFFERSIZE, stdin);
printf("Input was: '%s'\n", buffer);
}

The first version uses gets and will overflow if the input is longer than the buffer. The second version of the code uses fgets and will not overflow, because the amount of data written is limited by the length parameter.

Related rules

Other dangerous functions identified by CWE-676 ("Use of Potentially Dangerous Function") include strcpy and strcat. Use of these functions is highlighted by rules for the following CWEs:

• CWE-120 Classic Buffer Overflow.
• CWE-131 Incorrect Calculation of Buffer Size.

References

• Wikipedia: Morris worm.
• E. Spafford. The Internet Worm Program: An Analysis. Purdue Technical Report CSD-TR-823, (online), 1988.
• Common Weakness Enumeration: CWE-242.
• Common Weakness Enumeration: CWE-676.

cpp/ql/src/jsf/4.10 Classes/AV Rule 79.qhelp

Resource not released in destructor

This rule finds resources that are allocated by a class, but not released in the destructor of that class. Allocating a resource includes:

• Allocating memory with malloc

• Creating objects using new

• Opening files

• Opening network sockets
  Resource management can be a complex task, so a standard best practice is the pattern of Resource Acquisition is Initialization (RAII). In an RAII class, the constructor allocates all required resources, and the destructor frees all resources. This guarantees that simply deleting an instance of the class is enough to free resources, and benefits from C++'s automatic object lifetime management. A well-designed RAII class cannot be a source of resource leaks as long as its lifetime is properly managed:

• If it is allocated with new it should be released with delete by the client that created it

• If its lifetime is lexical (it is only used in one function), then it is enough to declare it as a local variable of that function (not a pointer) and the C++ runtime will ensure it is released on exit.
  There are two possible messages:

• "Resource x is acquired by class C but not released in the destructor. It is released from f, so this function may need to be called in the destructor".
  This indicates that the resource (x) is being released, just not in the destructor of the class. Typically, it is released in a function called close, free or something similar. This does not always indicate a resource leak, but it shows that the class is unnecessarily difficult to use because it does not conform to the RAII pattern.

• "Resource x is acquired by class C but not released anywhere in this class".
  This indicates that the class is allocating resources but not responsible for releasing them. This is very error-prone: even if a class requires an explicit close operator, it should manage any resources it allocates rather than forcing clients to manage them. In the worst case this can be a resource leak, if client code does not free the resource.

Recommendation

If the resource is not being released at all, ensure that the class does release the resource, normally by adding the release to the destructor of the class. This change needs to be carefully validated: client code may be relying on the resource outliving the class that allocated it, and must be reviewed and updated if necessary.

In the other case, for instance a class that has an explicit close function, the aim is to migrate the class to a straightforward RAII pattern. This can be achieved in several steps:

1. First, ensure that the close function (or its equivalent) is safe to call twice, by releasing resources only if they have not been released before.
2. Next, call the close function from the destructor. This does not require changing the client code, since it is safe to call it twice.
3. Migrate client code to remove direct uses of the close function, taking the opportunity to check that the object itself is being deleted appropriately.
4. Finally, when possible also migrate initialization code to the constructor of the class to make it follow the RAII pattern precisely.

Example

// This class opens a file but never closes it. Even its clients// cannot close the fileclassResourceLeak {
private:int sockfd;
FILE* file;
public:C() {
sockfd = socket(AF_INET, SOCK_STREAM, 0);
}
voidf() {
file = fopen("foo.txt", "r");
...
}
};
// This class relies on its client to release any stream it// allocates. Note that this means the client must have// intimate knowledge of the implementation of the class to// decide whether it is safe to release the stream. classStreamPool {
private:
Stream *instance;
public:
Stream *createStream(char *name) {
if (!instance) instance = newStream(name);
return instance;
}
}// This class handles its resources, but does not do that in// the constructor/destructor. It can be rewritten easily to// be safer to use.classStreamHandler {
private:char *_name;
Stream *stream;
public:C(char *name) {
_name = strdup(name):
}
voidopen() {
stream = newStream();
}
voidclose() {
delete stream;
}
~StreamHandler() {
free(_name);
// stream should be deleted here, not in close()
}
}

References

• AV Rule 79, Joint Strike Fighter Air Vehicle C++ Coding Standards. Lockheed Martin Corporation, 2005.
• S. Meyers. Effective C++ 3d ed. pp 61-66. Addison-Wesley Professional, 2005.
• Resource Acquisition Is Initialization
• Common Weakness Enumeration: CWE-404.

csharp/ql/src/Bad Practices/CallsUnmanagedCode.qhelp

Calls to unmanaged code

Microsoft defines two broad categories for source code. Managed code compiles into bytecode and is then executed by a virtual machine. Unmanaged code is compiled directly into machine code. All C# code is managed but it is possible to call external unmanaged code. This rule finds calls to extern methods that are implemented by unmanaged code. Managed code has many advantages over unmanaged code such as built in memory management performed by the virtual machine and the ability to run compiled programs on a wider variety of architectures.

Recommendation

Consider whether the calls could be replaced by calls to managed code instead.

Example

This example shows a function that displays a message box when clicked. It is implemented with unmanaged code from the User32.dll library.

usingSystem;usingSystem.Windows.Forms;usingSystem.Runtime.InteropServices;publicpartialclassUnmanagedCodeExample:Form{[DllImport("User32.dll")]publicstaticexternintMessageBox(inth,stringm,stringc,inttype);privatevoidbtnSayHello_Click(objectsender,EventArgse){MessageBox(0,"Hello World","Title",0);// BAD}}

Fixing by Using Managed Code

This code example does the exact same thing except it uses managed code to do so.

usingSystem;usingSystem.Windows.Forms;publicpartialclassManagedCodeExample:Form{privatevoidbtnSayHello_Click(objectsender,EventArgse){MessageBox.Show("Hello World","Title");}}

References

• MSDN, C# Reference extern.
• Wikipedia, Managed code.

csharp/ql/src/Bad Practices/Comments/TodoComments.qhelp

TODO comment

A comment that includes the words TODO, FIXME, or similar words often indicates code that is incomplete or broken, or highlights ambiguities in the software's specification.

Recommendation

Address the problem indicated by the comment.

Example

In the following example, the programmer has not yet implemented the correct behavior for the case where parameter a is zero: the method will throw a DivideByZeroException exception in this case.

usingSystem;classBad{publicstaticdoubleSolveQuadratic(doublea,doubleb,doublec){// TODO: handle case where a == 0return(-b+Math.Sqrt(b*b-4*a*c))/(2*a);}}

As a first step to fixing this problem, a check could be introduced that compares a to zero and throws another exception if this is the case. A better solution would be to use a different formula that does not rely on a being non-zero. Regardless of the solution adopted, the TODO comment should then be removed.

References

• Approxion: TODO or not TODO.
• Wikipedia: Comment tags.
• Common Weakness Enumeration: CWE-546.

csharp/ql/src/Bad Practices/Declarations/EmptyInterface.qhelp

Empty interface

Empty interfaces are often used as a way of marking particular classes.

Recommendation

In some languages, using a marker interface is a useful design pattern, but in C# it is better to use custom attributes. Marker interfaces are always inherited and so they cannot be applied to a single class without applying it to all subclasses. Custom attributes do not have this limitation.

Example

In this example, the IsPrintable interface has no defined methods and is simply being used as a marker.

usingSystem;classBad{interfaceIsPrintable{}classForm1:IsPrintable{}}

The following example is better because it uses attributes instead.

usingSystem;classGood{[AttributeUsage(AttributeTargets.Class)]classPrintableAttribute:Attribute{}[Printable]classForm1{}}

References

• Wikipedia: Marker interface pattern
• Microsoft: Using Attributes in C#

csharp/ql/src/Bad Practices/UnmanagedCodeCheck.qhelp

Unmanaged code

Microsoft defines two broad categories for source code. Managed code compiles into bytecode and is then executed by a virtual machine. Unmanaged code is compiled directly into machine code. All C# code is managed but it is possible to call external unmanaged code. This rule finds extern methods that are implemented by unmanaged code. Managed code has many advantages over unmanaged code such as built in memory management performed by the virtual machine and the ability to run compiled programs on a wider variety of architectures.

Recommendation

Consider whether the unmanaged extern methods could be implemented in C# instead.

Example

This example shows a function that displays a message box when clicked. The unmanaged code is shown first and then the same function being performed by managed code is shown after.

// example of using unmanaged codeusingSystem;usingSystem.Windows.Forms;usingSystem.Runtime.InteropServices;publicpartialclassUnmanagedCodeExample:Form{[DllImport("User32.dll")]publicstaticexternintMessageBox(inth,stringm,stringc,inttype);// BADprivatevoidbtnSayHello_Click(objectsender,EventArgse){MessageBox(0,"Hello World","Title",0);}}// the same thing in managed codeusingSystem;usingSystem.Windows.Forms;publicpartialclassManagedCodeExample:Form{privatevoidbtnSayHello_Click(objectsender,EventArgse){MessageBox.Show("Hello World","Title");}}

References

• MSDN, C# Reference extern.
• Wikipedia, Managed code.

csharp/ql/src/Dead Code/DeadStoreOfLocal.qhelp

Useless assignment to local variable

A value is assigned to a local variable, but either that variable is never read later on, or its value is always overwritten before being read. This means that the original assignment has no effect, and could indicate a logic error or incomplete code.

Recommendation

Ensure that you check the program logic carefully. If a value is really not needed, consider omitting the assignment. Be careful, though: if the right-hand side has a side effect (like performing a method call), it is important to keep this to preserve the overall behavior.

Example

The following example shows six different types of assignments to local variables whose value is not read:

• In ParseInt, the result of the call to int.TryParse is assigned directly to the unread local variable success.
• In IsDouble, the out argument of the call to int.TryParse is assigned to the unread local variable i.
• In ParseDouble, the exception thrown by the call to double.Parse in case of parse failure is assigned to the unread local variable e.
• In Count, the elements of ss are assigned to the unread local foreach variable s.
• In IsInt, o is assigned (in case o is an integer) to the unread local type test variable i.
• In IsString, o is assigned (in case o is a string) to the unread local type case variable s.

usingSystem;classBad{doubleParseInt(strings){varsuccess=int.TryParse(s,outinti);returni;}boolIsDouble(strings){varsuccess=double.TryParse(s,outdoublei);returnsuccess;}doubleParseDouble(strings){try{returndouble.Parse(s);}catch(FormatExceptione){returndouble.NaN;}}intCount(string[]ss){intcount=0;foreach(varsinss)count++;returncount;}stringIsInt(objecto){if(oisinti)return"yes";elsereturn"no";}stringIsString(objecto){switch(o){casestrings:return"yes";default:return"no";}}}

The revised example eliminates the unread assignments.

usingSystem;classGood{doubleParseInt(strings){int.TryParse(s,outinti);returni;}boolIsDouble(strings){varsuccess=double.TryParse(s,out_);returnsuccess;}doubleParseDouble(strings){try{returndouble.Parse(s);}catch(FormatException){returndouble.NaN;}}intCount(string[]ss){returnss.Length;}stringIsInt(objecto){if(oisint)return"yes";elsereturn"no";}stringIsString(objecto){switch(o){casestring _:return"yes";default:return"no";}}}

References

• Wikipedia: Dead store.
• MSDN, Code Analysis for Managed Code, CA1804: Remove unused locals.
• Microsoft: What's new in C# 7 - Discards.
• Common Weakness Enumeration: CWE-563.

csharp/ql/src/Likely Bugs/BadCheckOdd.qhelp

Bad parity check

Avoid using x % 2 == 1 or x % 2 > 0 to check whether a number x is odd, or x % 2 != 1 to check whether it is even. Such code does not work for negative numbers. For example, -5 % 2 equals -1, not 1.

Recommendation

Consider using x % 2 != 0 to check for odd and x % 2 == 0 to check for even.

Example

-9 is an odd number but this example does not detect it as one. This is because -9 % 2 is -1, not 1.

classCheckOdd{privatestaticboolIsOdd(intx){returnx%2==1;}publicstaticvoidMain(String[]args){Console.Out.WriteLine(IsOdd(-9));// prints False}}

It would be better to check if the number is even and then invert that check.

classCheckOdd{privatestaticboolIsOdd(intx){returnx%2!=0;}publicstaticvoidMain(String[]args){Console.Out.WriteLine(IsOdd(-9));// prints True}}

References

• MSDN Library: % Operator (C# Reference).
• Wikipedia: Modulo Operation - Common pitfalls.

csharp/ql/src/Metrics/Callables/CCyclomaticComplexity.qhelp

Cyclomatic complexity of functions

This metric measures the number of linearly independent execution paths through methods. Linearly independent paths are calculated using the control flow diagram for a piece of code. A linearly independent path includes at least one new edge that has not been included in any linearly independent path counted so far. Methods with a high cyclomatic complexity can be difficult to understand and difficult to test because there are so many different ways they could execute.

Consider this method:

publicstaticvoidfoo(intcount){if(count>10){Console.WriteLine("The count is large");}vartimesLeft=count;while(timesLeft>0){switch(Console.ReadLine()){case"BYE":Console.WriteLine("Good bye");break;case"HELLO":Console.WriteLine("Hi");break;case"HELP":Console.WriteLine("Try HELLO or BYE.");break;default:Console.WriteLine("Input not understood.");break;}timesLeft--;}}

The control flow diagram for through this method looks like this:

The first linearly independent path through this method is one where the condition for every branch point returns false. The path through the diagram would go Start → count>10 → timesLeft>0 → End. There is also another path where the first condition returns true and hence "The count is large" is printed. This counts as a second independent path because it adds the edges from count>10 → "The count is large" → timesLeft>0 which have not been included in the first path we counted. Likewise the second condition being true would add another independent path through the switch statement and one of its cases. The switch statement has 4 cases that count as paths however one has already been counted in the previous path. As such the switch statement adds another 3 independent paths through the method bringing the total cyclomatic complexity to 6.

Recommendation

Complex methods should have parts of their functionality extracted to helper methods. This makes testing easier because each helper method can be tested individually.

References

• Wikipedia. Cyclomatic complexity.
• Wikipedia. Control flow diagram.
• Wolfram MathWorld. Linearly Independent.

csharp/ql/src/Metrics/Files/FCyclomaticComplexity.qhelp

Average cyclomatic complexity of files

This metric measures the average cyclomatic complexity of the functions in a file.

The cyclomatic complexity of a function is the number of linearly independent execution paths through that function. A path is linearly independent path if it differs from all other paths by at least one node. Straight-line code therefore has a cyclomatic complexity of one, while branches, switches and loops increase cyclomatic complexity.

Functions with a high cyclomatic complexity are typically hard to understand and test. By extension, files whose functions have a high average cyclomatic complexity are problematic, and usually would benefit from refactoring.

As a concrete example, consider the following function:

intf(inti,intj){// startintresult;if(i%2==0){// iEvenresult=i+j;}else{// iOddif(j%2==0){// jEvenresult=i*j;}else{// jOddresult=i-j;}}returnresult;// end}

The control flow graph for this function is as follows:

The graph shows that the number of linearly independent execution paths through the function, and hence its cyclomatic complexity, is 3. The three paths are:

• start -> iEven -> end
• start -> iOdd -> jEven -> end
• start -> iOdd -> jOdd -> end

Recommendation

Functions with a high cyclomatic complexity should be simplified, for instance by tidying up any complex logic within them or by splitting them into multiple methods using the Extract Method refactoring.

References

• M. Fowler, Refactoring. Addison-Wesley, 1999.
• T. J. McCabe, A Complexity Measure. IEEE Transactions on Software Engineering, SE-2(4), December 1976.
• Dave Thomas, Refactoring as Meta Programming?, in Journal of Object Technology, vol. 4, no. 1, January-February 2005, pp. 7-11.
• Wikipedia: Cyclomatic complexity
• Wikipedia: Code refactoring

csharp/ql/src/Metrics/Files/FSelfContainedness.qhelp

Self containedness of files

This metric measures the percentage of types on which the file depends for which the build process built from source. The availability of source code is one of the key factors that affects how easy a project will be to build for different versions of the .NET framework. Files with low self-containedness are also more affected by changes to their dependencies.

Recommendation

Depending on your project, self-containedness may or may not be an issue for you. If you decide that it should be addressed then there are a few things you can do to easily increase self-containedness. One way of increasing self-containedness is by creating wrappers for any external classes. If the external class is changed then only your wrapper needs to be updated. You should also try to use libraries with source code available.

References

• Wikipedia. Software Portability.

csharp/ql/src/Metrics/RefTypes/TUnmanagedCode.qhelp

Types containing unmanaged code

Microsoft defines two broad categories for source code. Managed code compiles into bytecode and is then executed by a virtual machine. Unmanaged code is compiled directly into machine code. All C# code is managed but it is possible to call external unmanaged code. This metric counts the number of extern methods in each class that are implemented by unmanaged code. Managed code has many advantages over unmanaged code such as built in memory management performed by the virtual machine and the ability to run compiled programs on a wider variety of architectures.

Recommendation

Consider whether the unmanaged methods could be replaced by managed code instead.

Example

This example shows a function that displays a message box when clicked. It is implemented with unmanaged code from the User32.dll library.

usingSystem;usingSystem.Windows.Forms;usingSystem.Runtime.InteropServices;publicpartialclassUnmanagedCodeExample:Form{[DllImport("User32.dll")]publicstaticexternintMessageBox(inth,stringm,stringc,inttype);// violationprivatevoidbtnSayHello_Click(objectsender,EventArgse){MessageBox(0,"Hello World","Title",0);}}

Fixing by Using Managed Code

This example uses managed code to perform the same function.

usingSystem;usingSystem.Windows.Forms;publicpartialclassManagedCodeExample:Form{privatevoidbtnSayHello_Click(objectsender,EventArgse){MessageBox.Show("Hello World","Title");}}

References

• MSDN, C# Reference. extern.
• Wikipedia. Managed code.

csharp/ql/src/Security Features/CWE-079/XSS.qhelp

Cross-site scripting

Directly writing user input (for example, an HTTP request parameter) to a webpage, without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

Recommendation

To guard against cross-site scripting, consider using a library that provides suitable encoding functionality, such as the System.Net.WebUtility class, to sanitize the untrusted input before writing it to the page. For other possible solutions, see the references.

Example

The following example shows the page parameter being written directly to the server error page, leaving the website vulnerable to cross-site scripting.

usingSystem;usingSystem.Web;publicclassXSSHandler:IHttpHandler{publicvoidProcessRequest(HttpContextctx){ctx.Response.Write("The page \""+ctx.Request.QueryString["page"]+"\" was not found.");}}

Sanitizing the user-controlled data using the WebUtility.HtmlEncode method prevents the vulnerability:

usingSystem;usingSystem.Web;usingSystem.Net;publicclassXSSHandler:IHttpHandler{publicvoidProcessRequest(HttpContextctx){stringpage=WebUtility.HtmlEncode(ctx.Request.QueryString["page"]);ctx.Response.Write("The page \""+page+"\" was not found.");}}

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

csharp/ql/src/Security Features/InadequateRSAPadding.qhelp

Weak encryption: inadequate RSA padding

This query finds uses of RSA encryption without secure padding. Using PKCS#1 v1.5 padding can open up your application to several different attacks resulting in the exposure of the encryption key or the ability to determine plaintext from encrypted messages.

Recommendation

Use the more secure PKCS#1 v2 (OAEP) padding.

References

• Wikipedia. RSA. Padding Schemes.
• Common Weakness Enumeration: CWE-327.
• Common Weakness Enumeration: CWE-780.

csharp/ql/src/Security Features/InsecureRandomness.qhelp

Insecure randomness

Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, such as a password, makes it easier for an attacker to predict the value.

Pseudo-random number generators generate a sequence of numbers that only approximates the properties of random numbers. The sequence is not truly random because it is completely determined by a relatively small set of initial values, the seed. If the random number generator is cryptographically weak, then this sequence may be easily predictable through outside observations.

Recommendation

Use a cryptographically secure pseudo-random number generator if the output is to be used in a security sensitive context. As a rule of thumb, a value should be considered "security sensitive" if predicting it would allow the attacker to perform an action that they would otherwise be unable to perform. For example, if an attacker could predict the random password generated for a new user, they would be able to log in as that new user.

For C#, RNGCryptoServiceProvider provides a cryptographically secure pseudo-random number generator. Random is not cryptographically secure, and should be avoided in security contexts. For contexts which are not security sensitive, Random may be preferable as it has a more convenient interface, and is likely to be faster.

Example

The following examples show different ways of generating a password.

In the first case, we generate a fresh password by appending a random integer to the end of a static string. The random number generator used (Random) is not cryptographically secure, so it may be possible for an attacker to predict the generated password.

In the second example, a cryptographically secure random number generator is used for the same purpose. In this case, it is much harder to predict the generated integers.

In the final example, the password is generated using the Membership.GeneratePassword library method, which generates a password with a bias, therefore should be avoided.

usingSystem.Security.Cryptography;usingSystem.Web.Security;stringGeneratePassword(){// BAD: Password is generated using a cryptographically insecure RNGRandomgen=newRandom();stringpassword="mypassword"+gen.Next();// GOOD: Password is generated using a cryptographically secure RNGusing(RNGCryptoServiceProvidercrypto=newRNGCryptoServiceProvider()){byte[]randomBytes=newbyte[sizeof(int)];crypto.GetBytes(randomBytes);password="mypassword"+BitConverter.ToInt32(randomBytes);}// BAD: Membership.GeneratePassword generates a password with a biaspassword=Membership.GeneratePassword(12,3);returnpassword;}

References

• Wikipedia. Pseudo-random number generator.
• MSDN. RandomNumberGenerator.
• MSDN. Membership.GeneratePassword.
• Common Weakness Enumeration: CWE-338.

csharp/ql/src/Security Features/InsufficientKeySize.qhelp

Weak encryption: Insufficient key size

This rule finds uses of encryption algorithms with too small a key size. Encryption algorithms are vulnerable to brute force attack when too small a key size is used.

Recommendation

The key should be at least 2048-bit long when using RSA encryption, and 128-bit long when using symmetric encryption.

References

• Wikipedia. Key size.
• Common Weakness Enumeration: CWE-326.

csharp/ql/src/Security Features/WeakEncryption.qhelp

Weak encryption

Weak encryption algorithms provide very little security. For example DES encryption uses keys of 56 bits only, and no longer provides sufficient protection for sensitive data. TripleDES should also be deprecated for very sensitive data: Although it improves on DES by using 168-bit long keys, it provides in fact at most 112 bits of security.

Recommendation

You should switch to a more secure encryption algorithm, such as AES (Advanced Encryption Standard) and use a key length which is reasonable for the application for which it is being used. Do not use the ECB encryption mode since it is vulnerable to replay and other attacks.

Example

This example uses DES, which is limited to a 56-bit key. The key provided is actually 64 bits but the last bit of each byte is turned into a parity bit. For example the bytes 01010101 and 01010100 can be used in place of each other when encrypting and decrypting.

classWeakEncryption{publicstaticbyte[]encryptString(){SymmetricAlgorithmserviceProvider=newDESCryptoServiceProvider();byte[]key={16,22,240,11,18,150,192,21};serviceProvider.Key=key;ICryptoTransformencryptor=serviceProvider.CreateEncryptor();Stringmessage="Hello World";byte[]messageB=System.Text.Encoding.ASCII.GetBytes(message);returnencryptor.TransformFinalBlock(messageB,0,messageB.Length);}}

References

• Wikipedia: Key Size
• Wikipedia: DES
• Common Weakness Enumeration: CWE-327.

go/ql/src/RedundantCode/DeadStoreOfLocal.qhelp

Useless assignment to local variable

A value is assigned to a variable, but either it is never read, or its value is always overwritten before being read. This means that the original assignment has no effect, and could indicate a logic error or incomplete code.

Recommendation

Remove assignments to variables that are immediately overwritten, or use the blank identifier _ as a placeholder for return values that are never used.

Example

In the following example, a value is assigned to a, but then immediately overwritten, a value is assigned to b and never used, and finally, the results of a call to fmt.Println are assigned to two temporary variables, which are then immediately overwritten by a call to function.

package main
import"fmt"funcmain() {
a:=calculateValue()
a=2b:=calculateValue()
ignore, ignore1:=fmt.Println(a)
ignore, ignore1, err:=function()
iferr!=nil {
panic(err)
}
fmt.Println(a)
}

The result of calculateValue is never used, and if calculateValue is a side-effect free function, those assignments can be removed. To ignore all the return values of fmt.Println, you can simply not assign it to any variables. To ignore only certain return values, use _.

package main
import"fmt"funcmain() {
a:=2fmt.Println(a)
_, _, err:=function()
iferr!=nil {
panic(err)
}
fmt.Println(a)
}

References

• Wikipedia: Dead store.
• The Go Programming Language Specification: Blank identifier.
• Common Weakness Enumeration: CWE-563.

go/ql/src/RedundantCode/UnreachableStatement.qhelp

Unreachable statement

An unreachable statement often indicates missing code or a latent bug and should be examined carefully.

Recommendation

Examine the surrounding code to determine why the statement has become unreachable. If it is no longer needed, remove the statement.

Example

In the following example, the body of the for statement cannot terminate normally, so the update statement i++ becomes unreachable:

package main
funcmul(xs []int) int {
res:=1fori:=0; i<len(xs); i++ {
x:=xs[i]
res*=xifres==0 {
}
return0
}
returnres
}

Most likely, the return statement should be moved inside the if statement:

package main
funcmulGood(xs []int) int {
res:=1fori:=0; i<len(xs); i++ {
x:=xs[i]
res*=xifres==0 {
return0
}
}
returnres
}

References

• Wikipedia: Unreachable code.
• Common Weakness Enumeration: CWE-561.

go/ql/src/Security/CWE-079/ReflectedXss.qhelp

Reflected cross-site scripting

Directly writing user input (for example, an HTTP request parameter) to an HTTP response without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called reflected cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the response, or one of the other solutions that are mentioned in the references.

Example

The following example code writes part of an HTTP request (which is controlled by the user) directly to the response. This leaves the website vulnerable to cross-site scripting.

package main
import (
"fmt""net/http"
)
funcserve() {
http.HandleFunc("/user", func(w http.ResponseWriter, r*http.Request) {
r.ParseForm()
username:=r.Form.Get("username")
if!isValidUsername(username) {
// BAD: a request parameter is incorporated without validation into the responsefmt.Fprintf(w, "%q is an unknown user", username)
} else {
// TODO: Handle successful login
}
})
http.ListenAndServe(":80", nil)
}

Sanitizing the user-controlled data prevents the vulnerability:

package main
import (
"fmt""html""net/http"
)
funcserve1() {
http.HandleFunc("/user", func(w http.ResponseWriter, r*http.Request) {
r.ParseForm()
username:=r.Form.Get("username")
if!isValidUsername(username) {
// GOOD: a request parameter is escaped before being put into the responsefmt.Fprintf(w, "%q is an unknown user", html.EscapeString(username))
} else {
// TODO: do something exciting
}
})
http.ListenAndServe(":80", nil)
}

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

go/ql/src/Security/CWE-079/StoredXss.qhelp

Stored cross-site scripting

Directly using externally-controlled stored values (for example, file names or database contents) to create HTML content without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called stored cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before using uncontrolled stored values to create HTML content, or one of the other solutions that are mentioned in the references.

Example

The following example code writes file names directly to an HTTP response. This leaves the website vulnerable to cross-site scripting, if an attacker can choose the file names on the disk.

package main
import (
"io""net/http""os"
)
funcListFiles(w http.ResponseWriter, r*http.Request) {
files, _:=os.ReadDir(".")
for_, file:=rangefiles {
io.WriteString(w, file.Name()+"\n")
}
}

Sanitizing the file names prevents the vulnerability:

package main
import (
"html""io""net/http""os"
)
funcListFiles1(w http.ResponseWriter, r*http.Request) {
files, _:=os.ReadDir(".")
for_, file:=rangefiles {
io.WriteString(w, html.EscapeString(file.Name())+"\n")
}
}

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP: Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

go/ql/src/Security/CWE-338/InsecureRandomness.qhelp

Use of insufficient randomness as the key of a cryptographic algorithm

Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, such as a password, makes it easier for an attacker to predict the value.

Pseudo-random number generators generate a sequence of numbers that only approximates the properties of random numbers. The sequence is not truly random because it is completely determined by a relatively small set of initial values, the seed. If the random number generator is cryptographically weak, then this sequence may be easily predictable through outside observations.

Recommendation

Use a cryptographically secure pseudo-random number generator if the output is to be used in a security sensitive context. As a rule of thumb, a value should be considered "security sensitive" if predicting it would allow the attacker to perform an action that they would otherwise be unable to perform. For example, if an attacker could predict the random password generated for a new user, they would be able to log in as that new user.

For Go, crypto/rand provides a cryptographically secure pseudo-random number generator. math/rand is not cryptographically secure, and should be avoided in security contexts. For contexts which are not security sensitive, math/rand may be preferable as it has a more convenient interface, and is likely to be faster.

Example

The example below uses the math/rand package instead of crypto/rand to generate a password:

package main
import (
"math/rand"
)
varcharset= []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789")
funcgeneratePassword() string {
s:=make([]rune, 20)
fori:=ranges {
s[i] =charset[rand.Intn(len(charset))]
}
returnstring(s)
}

Instead, use crypto/rand:

package main
import (
"crypto/rand""math/big"
)
funcgeneratePasswordGood() string {
s:=make([]rune, 20)
fori:=ranges {
idx, err:=rand.Int(rand.Reader, big.NewInt(int64(len(charset))))
iferr!=nil {
// handle err
}
s[i] =charset[idx.Int64()]
}
returnstring(s)
}

References

• Wikipedia. Pseudo-random number generator.
• OWASP: Insecure Randomness.
• OWASP: Secure Random Number Generation.
• Common Weakness Enumeration: CWE-338.

java/ql/src/Likely Bugs/Concurrency/LazyInitStaticField.qhelp

Incorrect lazy initialization of a static field

The tactic of initializing a static field the first time it is used, known as "lazy initialization", can be problematic in a multi-threaded context when used without proper synchronization. If a separate thread starts executing before the field is initialized, the thread may see an incompletely initialized object.

Recommendation

If lazy initialization is desirable for performance reasons, the best solution is usually to declare the enclosing method synchronized. Otherwise, avoid lazy initialization and initialize static fields using static initializers. A third possibility is to declare the field volatile and use the double-checked locking idiom as explained in the article referenced below. As the article points out, it is crucial to declare the field volatile: double-checked locking by itself is not correct under the Java memory model.

Example

In the following example, the static field resource is initialized without synchronization.

classSingleton {
privatestaticResourceresource;
publicResourcegetResource() {
if(resource == null)
resource = newResource(); // Lazily initialize "resource"returnresource;
}
}

In the following modification of the above example, Singleton uses the recommended approach of using a static initializer to initialize resource.

classSingleton {
privatestaticResourceresource;
static {
resource = newResource(); // Initialize "resource" only once
}
publicResourcegetResource() {
returnresource;
}
}

References

• University of Maryland Department of Computer Science: The "Double-Checked Locking is Broken" Declaration.
• Common Weakness Enumeration: CWE-543.
• Common Weakness Enumeration: CWE-609.

java/ql/src/Metrics/Callables/CCyclomaticComplexity.qhelp

Cyclomatic complexity of functions

The cyclomatic complexity of a method (or constructor) is the number of possible linearly-independent execution paths through that method (see [Wikipedia]). It was originally introduced as a complexity measure by Thomas McCabe [McCabe].

A method with high cyclomatic complexity is typically difficult to understand and test.

Example

intf(inti, intj) {
intresult;
if(i % 2 == 0) {
result = i + j;
}
else {
if(j % 2 == 0) {
result = i * j;
}
else {
result = i - j;
}
}
returnresult;
}

The control flow graph for this method is as follows:

As you can see from the graph, the number of linearly-independent execution paths through the method is 3. Therefore, the cyclomatic complexity is 3.

Recommendation

Simplify methods that have a high cyclomatic complexity. For example, tidy up complex logic, and/or split methods into multiple smaller methods using the 'Extract Method' refactoring from [Fowler].

References

• M. Fowler, Refactoring. Addison-Wesley, 1999.
• T. J. McCabe, A Complexity Measure. IEEE Transactions on Software Engineering, SE-2(4), December 1976.
• Wikipedia: Cyclomatic complexity.

java/ql/src/Metrics/Files/FCyclomaticComplexity.qhelp

Average cyclomatic complexity of files

This metric measures the average cyclomatic complexity of the functions in a file.

The cyclomatic complexity of a function is the number of linearly independent execution paths through that function. A path is linearly independent path if it differs from all other paths by at least one node. Straight-line code therefore has a cyclomatic complexity of one, while branches, switches and loops increase cyclomatic complexity.

Functions with a high cyclomatic complexity are typically hard to understand and test. By extension, files whose functions have a high average cyclomatic complexity are problematic, and usually would benefit from refactoring.

As a concrete example, consider the following function:

intf(inti, intj) {
// startintresult;
if(i % 2 == 0) {
// iEvenresult = i + j;
}
else {
// iOddif(j % 2 == 0) {
// jEvenresult = i * j;
}
else {
// jOddresult = i - j;
}
}
returnresult;
// end
}

The control flow graph for this function is as follows:

The graph shows that the number of linearly independent execution paths through the function, and hence its cyclomatic complexity, is 3. The three paths are:

• start -> iEven -> end
• start -> iOdd -> jEven -> end
• start -> iOdd -> jOdd -> end

Recommendation

Functions with a high cyclomatic complexity should be simplified, for instance by tidying up any complex logic within them or by splitting them into multiple methods using the Extract Method refactoring.

References

• M. Fowler, Refactoring. Addison-Wesley, 1999.
• T. J. McCabe, A Complexity Measure. IEEE Transactions on Software Engineering, SE-2(4), December 1976.
• Dave Thomas, Refactoring as Meta Programming?, in Journal of Object Technology, vol. 4, no. 1, January-February 2005, pp. 7-11.
• Wikipedia: Cyclomatic complexity
• Wikipedia: Code refactoring

java/ql/src/Metrics/Files/FLinesOfDuplicatedCode.qhelp

Duplicated lines in files

This metric measures the number of lines in a file that are contained within a block that is duplicated elsewhere. These lines may include code, comments and whitespace, and the duplicate block may be in this file or in another file.

A file that contains many lines that are duplicated within the code base is problematic for a number of reasons.

Duplicated code increases overall code size, making the code base harder to maintain and harder to understand. It also becomes harder to fix bugs, since a programmer applying a fix to one copy has to always remember to update other copies accordingly. Finally, code duplication is generally an indication of a poorly designed or hastily written code base, which typically suffers from other problems as well.

Recommendation

Refactor files with lots of duplicated code to extract the common code into a shared library or module.

References

• Wikipedia: Duplicate code.
• M. Fowler, Refactoring. Addison-Wesley, 1999.

java/ql/src/Metrics/Files/FLinesOfSimilarCode.qhelp

Similar lines in files

A file that contains many lines that are similar to other code within the code base is problematic for the same reasons as a file that contains a lot of (exactly) duplicated code.

Duplicated code increases overall code size, making the code base harder to maintain and harder to understand. It also becomes harder to fix bugs, since a programmer applying a fix to one copy has to always remember to update other copies accordingly. Finally, code duplication is generally an indication of a poorly designed or hastily written code base, which typically suffers from other problems as well.

Recommendation

Refactor similar code snippets by extracting common functionality into methods that can be reused across classes.

References

• Wikipedia: Duplicate code.
• M. Fowler, Refactoring. Addison-Wesley, 1999.

java/ql/src/Metrics/Files/FSelfContainedness.qhelp

Self-containedness of files

This metric measures the percentage of the types on which a compilation unit depends for which we have source code available.

The availability of source code is one of the key factors affecting how easy or difficult it will be to build a software project in the future, especially on platforms other than those for which it was originally developed. Projects will a high level of self-containedness are likely to be more portable and easier to build in ten years' time than those that depend on many binary-only, third-party libraries. (This is one reason why many of the dependencies of open-source projects are distributed as source code, aside from the fact that the binaries are generally larger and more unwieldy to distribute.)

In the context of Java's platform independence, the availability of source code is less critical than it is for platform-dependent languages. However, note that there can be minor binary incompatibilities between different versions of Java.

Recommendation

Low self-containedness may or may not be a problem, depending on the context of your project. However, if you determine that it is an issue for you, it is best tackled at a project level, in the following ways:

• Try to use libraries for which the source code is available.
• Try to obtain the source code for binary-only libraries from the authors.
• Where practical, rewrite parts of your code to reduce your dependence on external libraries.

References

• Wikipedia: Software portability
• Oracle Technology Network: Java SE 6 Compatibility
• Java Language Specification: Binary Compatibility

java/ql/src/Metrics/RefTypes/TSelfContainedness.qhelp

Self-containedness of types

This metric measures the percentage of the types on which a type depends for which we have source code available.

The availability of source code is one of the key factors affecting how easy or difficult it will be to build a software project in the future, especially on platforms other than those for which it was originally developed. Projects will a high level of self-containedness are likely to be more portable and easier to build in ten years' time than those that depend on many binary-only, third-party libraries. (This is one reason why many of the dependencies of open-source projects are distributed as source code, aside from the fact that the binaries are generally larger and more unwieldy to distribute.)

In the context of Java's platform independence, the availability of source code is less critical than it is for platform-dependent languages. However, note that there can be minor binary incompatibilities between different versions of Java.

Recommendation

Low self-containedness may or may not be a problem, depending on the context of your project. However, if you determine that it is an issue for you, it is best tackled at a project level, in the following ways:

• Try to use libraries for which the source code is available.
• Try to obtain the source code for binary-only libraries from the authors.
• Where practical, rewrite parts of your code to reduce your dependence on external libraries.

References

• Wikipedia. Software portability. Published online.
• Oracle Technology Network: Java SE 6 Compatibility
• Java Language Specification: Binary Compatibility

java/ql/src/Security/CWE/CWE-079/XSS.qhelp

Cross-site scripting

Directly writing user input (for example, an HTTP request parameter) to a web page, without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the page, or one of the other solutions that are mentioned in the reference.

Example

The following example shows the page parameter being written directly to the page, leaving the website vulnerable to cross-site scripting.

publicclassXSSextendsHttpServlet {
protectedvoiddoGet(HttpServletRequestrequest, HttpServletResponseresponse)
throwsServletException, IOException {
// BAD: a request parameter is written directly to the Servlet response streamresponse.getWriter().print(
"The page \"" + request.getParameter("page") + "\" was not found.");
}
}

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.

java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qhelp

HTTP response splitting

Directly writing user input (for example, an HTTP request parameter) to an HTTP header can lead to an HTTP request-splitting or response-splitting vulnerability.

HTTP response splitting can lead to vulnerabilities such as XSS and cache poisoning.

HTTP request splitting can allow an attacker to inject an additional HTTP request into a client's outgoing socket connection. This can allow an attacker to perform an SSRF-like attack.

In the context of a servlet container, if the user input includes blank lines and the servlet container does not escape the blank lines, then a remote user can cause the response to turn into two separate responses. The remote user can then control one or more responses, which is also HTTP response splitting.

Recommendation

Guard against HTTP header splitting in the same way as guarding against cross-site scripting. Before passing any data into HTTP headers, either check the data for special characters, or escape any special characters that are present.

If the code calls Netty API's directly, ensure that the validateHeaders parameter is set to true.

Example

The following example shows the 'name' parameter being written to a cookie in two different ways. The first way writes it directly to the cookie, and thus is vulnerable to response-splitting attacks. The second way first removes all special characters, thus avoiding the potential problem.

publicclassResponseSplittingextendsHttpServlet {
protectedvoiddoGet(HttpServletRequestrequest, HttpServletResponseresponse)
throwsServletException, IOException {
// BAD: setting a cookie with an unvalidated parameterCookiecookie = newCookie("name", request.getParameter("name"));
response.addCookie(cookie);
// GOOD: remove special characters before putting them in the headerStringname = removeSpecial(request.getParameter("name"));
Cookiecookie2 = newCookie("name", name);
response.addCookie(cookie2);
}
privatestaticStringremoveSpecial(Stringstr) {
returnstr.replaceAll("[^a-zA-Z ]", "");
}
}

Example

The following example shows the use of the library 'netty' with HTTP response-splitting verification configurations. The second way will verify the parameters before using them to build the HTTP response.

importio.netty.handler.codec.http.DefaultHttpHeaders;
publicclassResponseSplitting {
// BAD: Disables the internal response splitting verificationprivatefinalDefaultHttpHeadersbadHeaders = newDefaultHttpHeaders(false);
// GOOD: Verifies headers passed don't contain CRLF charactersprivatefinalDefaultHttpHeadersgoodHeaders = newDefaultHttpHeaders();
// BAD: Disables the internal response splitting verificationprivatefinalDefaultHttpResponsebadResponse = newDefaultHttpResponse(version, httpResponseStatus, false);
// GOOD: Verifies headers passed don't contain CRLF charactersprivatefinalDefaultHttpResponsegoodResponse = newDefaultHttpResponse(version, httpResponseStatus);
}

Example

The following example shows the use of the netty library with configurations for verification of HTTP request splitting. The second recommended approach in the example verifies the parameters before using them to build the HTTP request.

publicclassNettyRequestSplitting {
// BAD: Disables the internal request splitting verificationprivatefinalDefaultHttpHeadersbadHeaders = newDefaultHttpHeaders(false);
// GOOD: Verifies headers passed don't contain CRLF charactersprivatefinalDefaultHttpHeadersgoodHeaders = newDefaultHttpHeaders();
// BAD: Disables the internal request splitting verificationprivatefinalDefaultHttpRequestbadRequest = newDefaultHttpRequest(httpVersion, method, uri, false);
// GOOD: Verifies headers passed don't contain CRLF charactersprivatefinalDefaultHttpRequestgoodResponse = newDefaultHttpRequest(httpVersion, method, uri);
}

References

• SecLists.org: HTTP response splitting.
• OWASP: HTTP Response Splitting.
• Wikipedia: HTTP response splitting.
• CAPEC: CAPEC-105: HTTP Request Splitting
• Common Weakness Enumeration: CWE-113.

java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.qhelp

Use of a cryptographic algorithm with insufficient key size

Modern encryption relies on the computational infeasibility of breaking a cipher and decoding its message without the key. As computational power increases, the ability to break ciphers grows, and key sizes need to become larger as a result. Cryptographic algorithms that use too small of a key size are vulnerable to brute force attacks, which can reveal sensitive data.

Recommendation

Use a key of the recommended size or larger. The key size should be at least 128 bits for AES encryption, 256 bits for elliptic-curve cryptography (ECC), and 2048 bits for RSA, DSA, or DH encryption.

Example

The following code uses cryptographic algorithms with insufficient key sizes.

KeyPairGeneratorkeyPairGen1 = KeyPairGenerator.getInstance("RSA");
keyPairGen1.initialize(1024); // BAD: Key size is less than 2048KeyPairGeneratorkeyPairGen2 = KeyPairGenerator.getInstance("DSA");
keyPairGen2.initialize(1024); // BAD: Key size is less than 2048KeyPairGeneratorkeyPairGen3 = KeyPairGenerator.getInstance("DH");
keyPairGen3.initialize(1024); // BAD: Key size is less than 2048KeyPairGeneratorkeyPairGen4 = KeyPairGenerator.getInstance("EC");
ECGenParameterSpececSpec = newECGenParameterSpec("secp112r1"); // BAD: Key size is less than 256keyPairGen4.initialize(ecSpec);
KeyGeneratorkeyGen = KeyGenerator.getInstance("AES");
keyGen.init(64); // BAD: Key size is less than 128

To fix the code, change the key sizes to be the recommended size or larger for each algorithm.

References

• Wikipedia: Key size.
• Wikipedia: Strong cryptography.
• OWASP: Cryptographic Storage Cheat Sheet.
• OWASP: Testing for Weak Encryption.
• NIST: Transitioning the Use of Cryptographic Algorithms and Key Lengths.
• Common Weakness Enumeration: CWE-326.

java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp

Insecure randomness

If you use a cryptographically weak pseudo-random number generator to generate security-sensitive values, such as passwords, attackers can more easily predict those values.

Pseudo-random number generators generate a sequence of numbers that only approximates the properties of random numbers. The sequence is not truly random because it is completely determined by a relatively small set of initial values (the seed). If the random number generator is cryptographically weak, then this sequence may be easily predictable through outside observations.

Recommendation

The java.util.Random random number generator is not cryptographically secure. Use a secure random number generator such as java.security.SecureRandom instead.

Use a cryptographically secure pseudo-random number generator if the output is to be used in a security-sensitive context. As a general rule, a value should be considered "security-sensitive" if predicting it would allow the attacker to perform an action that they would otherwise be unable to perform. For example, if an attacker could predict the random password generated for a new user, they would be able to log in as that new user.

Example

The following examples show different ways of generating a cookie with a random value.

In the first (BAD) case, we generate a fresh cookie by appending a random integer to the end of a static string. The random number generator used (Random) is not cryptographically secure, so it may be possible for an attacker to predict the generated cookie.

Randomr = newRandom(); // BAD: Random is not cryptographically securebyte[] bytes = newbyte[16];
r.nextBytes(bytes);
StringcookieValue = encode(bytes);
Cookiecookie = newCookie("name", cookieValue);
response.addCookie(cookie);

In the second (GOOD) case, we generate a fresh cookie by appending a random integer to the end of a static string. The random number generator used (SecureRandom) is cryptographically secure, so it is not possible for an attacker to predict the generated cookie.

SecureRandomr = newSecureRandom(); // GOOD: SecureRandom is cryptographically securebyte[] bytes = newbyte[16];
r.nextBytes(bytes);
StringcookieValue = encode(bytes);
Cookiecookie = newCookie("name", cookieValue);
response.addCookie(cookie);

References

• Wikipedia: Pseudo-random number generator.
• Java Docs: Random.
• Java Docs: SecureRandom.
• Common Weakness Enumeration: CWE-330.
• Common Weakness Enumeration: CWE-338.

java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.qhelp

Trust boundary violation

A trust boundary violation occurs when a value is passed from a less trusted context to a more trusted context.

For example, a value that is generated by a less trusted source, such as a user, may be passed to a more trusted source, such as a system process. If the less trusted source is malicious, then the value may be crafted to exploit the more trusted source.

Trust boundary violations are often caused by a failure to validate input. For example, if a web application accepts a cookie from a user, then the application should validate the cookie before using it. If the cookie is not validated, then the user may be able to craft a malicious cookie that exploits the application.

Recommendation

To maintain a trust boundary, validate data from less trusted sources before use.

Example

In the first (bad) example, the server accepts a parameter from the user, then uses it to set the username without validation.

publicvoiddoGet(HttpServletRequestrequest, HttpServletResponseresponse) {
Stringusername = request.getParameter("username");
// BAD: The input is written to the session without being sanitized.request.getSession().setAttribute("username", username);
}

In the second (good) example, the server validates the parameter from the user, then uses it to set the username.

publicvoiddoGet(HttpServletRequestrequest, HttpServletResponseresponse) {
Stringusername = request.getParameter("username");
if (validator.isValidInput("HTTP parameter", username, "username", 20, false)) {
// GOOD: The input is sanitized before being written to the session.request.getSession().setAttribute("username", username);
}
}

References

• Wikipedia: Trust boundary.
• Common Weakness Enumeration: CWE-501.

java/ql/src/Violations of Best Practice/Comments/TodoComments.qhelp

TODO/FIXME comments

A comment that includes the word TODO or FIXME often marks a part of the code that is incomplete or broken, or highlights ambiguities in the software's specification.

For example, this list of comments is typical of those found in real programs:

• TODO: move this code somewhere else
• FIXME: handle this case
• FIXME: find a better solution to this workaround
• TODO: test this

Recommendation

It is very important that TODO or FIXME comments are not just removed from the code. Each of them must be addressed in some way.

Simpler comments can usually be immediately addressed by fixing the code, adding a test, doing some refactoring, or clarifying the intended behavior of a feature.

In contrast, larger issues may require discussion, and a significant amount of work to address. In these cases it is a good idea to move the comment to an issue-tracking system, so that the issue can be tracked and prioritized relative to other defects and feature requests.

References

• Approxion: TODO or not TODO.
• Wikipedia: Comment tags, Issue tracking system.
• Common Weakness Enumeration: CWE-546.

java/ql/src/Violations of Best Practice/Dead Code/DeadRefTypes.qhelp

Unused classes and interfaces

A non-public class or interface that is not used anywhere in the program may cause a programmer to waste time and effort maintaining and documenting it.

Recommendation

Ensure that redundant types are removed from the program.

References

• Wikipedia: Unreachable code.
• Common Weakness Enumeration: CWE-561.

javascript/ql/src/Comments/TodoComments.qhelp

TODO comment

A comment that includes the words TODO, FIXME or similar words often indicates code that is incomplete or broken, or highlights ambiguities in the software's specification.

Recommendation

Address the problem indicated by the comment.

Example

In the following example, the programmer has not yet implemented the correct behavior for the case where parameter a is zero: the function will return Infinity or NaN (depending on the values of b and c) in this case.

functionsolveQuadratic(a,b,c){// TODO: handle case where a === 0return(-b+Math.sqrt(b*b-4*a*c))/(2*a);}

As a first step to fixing this problem, a check could be introduced that compares a to zero and throws an exception if this is the case. A better solution would be to use a different formula that does not rely on a being non-zero. Regardless of the solution adopted, the TODO comment should then be removed.

References

• Approxion: TODO or not TODO.
• Wikipedia: Comment tags.
• Common Weakness Enumeration: CWE-546.

javascript/ql/src/Declarations/DeadStoreOfLocal.qhelp

Useless assignment to local variable

A value is assigned to a variable or property, but either that location is never read later on, or its value is always overwritten before being read. This means that the original assignment has no effect, and could indicate a logic error or incomplete code.

Recommendation

Ensure that you check the control and data flow in the method carefully. If a value is really not needed, consider omitting the assignment. Be careful, though: if the right-hand side has a side-effect (like performing a method call), it is important to keep this to preserve the overall behavior.

Example

In the following example, the return value of the call to send on line 2 is assigned to the local variable result, but then never used.

functionf(x){varresult=send(x);waitForResponse();returngetResponse();}

Assuming that send returns a status code indicating whether the operation succeeded or not, the value of result should be checked, perhaps like this:

functionf(x){varresult=send(x);// check for errorif(result===-1)thrownewError("send failed");waitForResponse();returngetResponse();}

References

• Wikipedia: Dead store.
• Common Weakness Enumeration: CWE-563.

javascript/ql/src/Declarations/DeadStoreOfProperty.qhelp

Useless assignment to property

A value is assigned to a variable or property, but either that location is never read later on, or its value is always overwritten before being read. This means that the original assignment has no effect, and could indicate a logic error or incomplete code.

Recommendation

Ensure that you check the control and data flow in the method carefully. If a value is really not needed, consider omitting the assignment. Be careful, though: if the right-hand side has a side-effect (like performing a method call), it is important to keep this to preserve the overall behavior.

Example

In the following example, the return value of the call to send on line 2 is assigned to the local variable result, but then never used.

functionf(x){varresult=send(x);waitForResponse();returngetResponse();}

Assuming that send returns a status code indicating whether the operation succeeded or not, the value of result should be checked, perhaps like this:

functionf(x){varresult=send(x);// check for errorif(result===-1)thrownewError("send failed");waitForResponse();returngetResponse();}

References

• Wikipedia: Dead store.

javascript/ql/src/LanguageFeatures/Eval.qhelp

Use of eval

The built-in eval function and the Function constructor allow executing arbitrary strings as JavaScript code. This is a dangerous feature, since this code has the same access privileges as any other code, so great care has to be taken to ensure that malicious code is not accidentally executed this way. Using this feature also hampers static checking and program comprehension. In many cases, better alternatives are available and should be used instead.

Recommendation

There are few genuine uses of eval and Function. If you are trying to assign to a property whose name is not known until runtime, use a computed property access. If you are trying to evaluate a string to a JSON object, use JSON.parse. In other cases, you may be able to use the Interpreter pattern.

Example

In the following example, eval is used to define getter and setter methods for properties x and y on Point.prototype:

functionPoint(x,y){this.x=x;this.y=y;}["x","y"].forEach(function(p){eval("Point.prototype.get_"+p+" = function() {"+" return this."+p+";"+"}");eval("Point.prototype.set_"+p+" = function(v) {"+" if (typeof v !== 'number')"+" throw Error('number expected');"+" this."+p+" = v;"+"}");});

In a variant, the programmer has realized that they can use computed property accesses to avoid having to wrap the assignment into an eval, although they still use the Function constructor to create the accessor functions:

functionPoint(x,y){this.x=x;this.y=y;}["x","y"].forEach(function(p){Point.prototype["get_"+p]=newFunction("","return this."+p+";");Point.prototype["set_"+p]=newFunction("v","if (typeof v !== 'number')"+" throw Error('number expected');"+" this."+p+" = v;");});

This is not necessary either as the following example shows, where the use of Function has also been replaced by computed property accesses:

functionPoint(x,y){this.x=x;this.y=y;}["x","y"].forEach(function(p){Point.prototype["get_"+p]=function(){returnthis[p];};Point.prototype["set_"+p]=function(v){if(typeofv!=='number')throwError('number expected');this[p]=v;};});

References

• D. Crockford, JavaScript: The Good Parts, Appendix B.3. O'Reilly, 2008.
• Common Weakness Enumeration: CWE-676.

javascript/ql/src/Metrics/FCyclomaticComplexity.qhelp

Average cyclomatic complexity of files

This metric measures the average cyclomatic complexity of the functions in a file.

The cyclomatic complexity of a function is the number of linearly independent execution paths through that function. A path is linearly independent path if it differs from all other paths by at least one node. Straight-line code therefore has a cyclomatic complexity of one, while branches, switches and loops increase cyclomatic complexity.

Functions with a high cyclomatic complexity are typically hard to understand and test. By extension, files whose functions have a high average cyclomatic complexity are problematic, and usually would benefit from refactoring.

As a concrete example, consider the following function:

functionf(i,j){// startvarresult;if(i%2==0){// iEvenresult=i+j;}else{// iOddif(j%2==0){// jEvenresult=i*j;}else{// jOddresult=i-j;}}returnresult;// end}

The control flow graph for this function is as follows:

The graph shows that the number of linearly independent execution paths through the function, and hence its cyclomatic complexity, is 3. The three paths are:

• start -> iEven -> end
• start -> iOdd -> jEven -> end
• start -> iOdd -> jOdd -> end

Recommendation

Functions with a high cyclomatic complexity should be simplified, for instance by tidying up any complex logic within them or by splitting them into multiple methods using the Extract Method refactoring.

References

• M. Fowler, Refactoring. Addison-Wesley, 1999.
• T. J. McCabe, A Complexity Measure. IEEE Transactions on Software Engineering, SE-2(4), December 1976.
• Dave Thomas, Refactoring as Meta Programming?, in Journal of Object Technology, vol. 4, no. 1, January-February 2005, pp. 7-11.
• Wikipedia: Cyclomatic complexity
• Wikipedia: Code refactoring

javascript/ql/src/Metrics/FLinesOfDuplicatedCode.qhelp

Duplicated lines in files

This metric measures the number of lines in a file that are contained within a block that is duplicated elsewhere. These lines may include code, comments and whitespace, and the duplicate block may be in this file or in another file.

A file that contains many lines that are duplicated within the code base is problematic for a number of reasons.

Duplicated code increases overall code size, making the code base harder to maintain and harder to understand. It also becomes harder to fix bugs, since a programmer applying a fix to one copy has to always remember to update other copies accordingly. Finally, code duplication is generally an indication of a poorly designed or hastily written code base, which typically suffers from other problems as well.

Recommendation

Refactor files with lots of duplicated code to extract the common code into a shared library or module.

References

• Wikipedia: Duplicate code.
• M. Fowler, Refactoring. Addison-Wesley, 1999.

javascript/ql/src/Metrics/FLinesOfSimilarCode.qhelp

Similar lines in files

This metric measures the number of lines in a file that are contained within a block that is duplicated elsewhere. These lines may include code, comments and whitespace, and the duplicate block may be in this file or in another file.

A file that contains many lines that are similar to other code within the code base is problematic for the same reasons as a file that contains a lot of (exactly) duplicated code.

Duplicated code increases overall code size, making the code base harder to maintain and harder to understand. It also becomes harder to fix bugs, since a programmer applying a fix to one copy has to always remember to update other copies accordingly. Finally, code duplication is generally an indication of a poorly designed or hastily written code base, which typically suffers from other problems as well.

Recommendation

Refactor similar code snippets by extracting common functionality into functions that can be reused across modules.

References

• Wikipedia: Duplicate code.
• M. Fowler, Refactoring. Addison-Wesley, 1999.

javascript/ql/src/Metrics/FunCyclomaticComplexity.qhelp

Cyclomatic complexity of functions

This metric measures the cyclomatic complexity of each function in the project.

The cyclomatic complexity of a function is an indication of the number of paths that can be taken during the execution of a function. Code with many branches and loops has high cyclomatic complexity. A cyclomatic complexity above 50 should be considered bad practice and above 75 should definitely be addressed.

Functions with high cyclomatic complexity are

• difficult to test since tests should be provided for each possible execution path;
• difficult to understand since a developer needs to understand how all conditions interact;
• difficult to maintain since many execution paths is an indication of functions that perform too many tasks.

Recommendation

The primary way to reduce the complexity is to extract sub-functionality into separate functions. This improves on all problems described above. If the function naturally breaks up into a sequence of operations it is preferable to extract each operation as a separate function. Even if that's not the case it is often possible to extract the body of an iteration into a separate function to reduce complexity. If the complexity can't be reduced significantly make sure that the function is properly documented and carefully tested.

References

• M. Fowler. Refactoring. Addison-Wesley, 1999.
• T. J. McCabe. A Complexity Measure. IEEE Transactions on Software Engineering, SE-2(4), December 1976.
• Wikipedia: Cyclomatic complexity.

javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp

Exception text reinterpreted as HTML

Directly writing error messages to a webpage without sanitization allows for a cross-site scripting vulnerability if parts of the error message can be influenced by a user.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the page, or one of the other solutions that are mentioned in the references.

Example

The following example shows an exception being written directly to the document, and this exception can potentially be influenced by the page URL, leaving the website vulnerable to cross-site scripting.

functionsetLanguageOptions(){varhref=document.location.href,deflt=href.substring(href.indexOf("default=")+8);try{varparsed=unknownParseFunction(deflt);}catch(e){document.write("Had an error: "+e+".");}}

Example

This second example shows an input being validated using the JSON schema validator ajv, and in case of an error, the error message is sent directly back in the response.

importexpressfrom'express';importAjvfrom'ajv';letapp=express();letajv=newAjv();ajv.addSchema({type: 'object',additionalProperties: {type: 'number'}},'pollData');app.post('/polldata',(req,res)=>{if(!ajv.validate('pollData',req.body)){res.send(ajv.errorsText());}});

This is unsafe, because the error message can contain parts of the input. For example, the input {'<img src=x onerror=alert(1)>': 'foo'} will generate the error data/<img src=x onerror=alert(1)> should be number, causing reflected XSS.

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-079/ReflectedXss.qhelp

Reflected cross-site scripting

Directly writing user input (for example, an HTTP request parameter) to an HTTP response without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called reflected cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the response, or one of the other solutions that are mentioned in the references.

Example

The following example code writes part of an HTTP request (which is controlled by the user) directly to the response. This leaves the website vulnerable to cross-site scripting.

varapp=require('express')();app.get('/user/:id',function(req,res){if(!isValidUserId(req.params.id))// BAD: a request parameter is incorporated without validation into the responseres.send("Unknown user: "+req.params.id);else// TODO: do something exciting;});

Sanitizing the user-controlled data prevents the vulnerability:

varescape=require('escape-html');varapp=require('express')();app.get('/user/:id',function(req,res){if(!isValidUserId(req.params.id))// GOOD: request parameter is sanitized before incorporating it into the responseres.send("Unknown user: "+escape(req.params.id));else// TODO: do something exciting;});

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-079/StoredXss.qhelp

Stored cross-site scripting

Directly using uncontrolled stored value (for example, file names) to create HTML content without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called stored cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before using uncontrolled stored values to create HTML content, or one of the other solutions that are mentioned in the references.

Example

The following example code writes file names directly to a HTTP response. This leaves the website vulnerable to cross-site scripting, if an attacker can choose the file names on the disk.

varexpress=require('express'),fs=require('fs');express().get('/list-directory',function(req,res){fs.readdir('/public',function(error,fileNames){varlist='<ul>';fileNames.forEach(fileName=>{// BAD: `fileName` can contain HTML elementslist+='<li>'+fileName+'</li>';});list+='</ul>'res.send(list);});});

Sanitizing the file names prevents the vulnerability:

varexpress=require('express'),fs=require('fs'),escape=require('escape-html');express().get('/list-directory',function(req,res){fs.readdir('/public',function(error,fileNames){varlist='<ul>';fileNames.forEach(fileName=>{// GOOD: escaped `fileName` can not contain HTML elementslist+='<li>'+escape(fileName)+'</li>';});list+='</ul>'res.send(list);});});

References

• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp

Unsafe HTML constructed from library input

When a library function dynamically constructs HTML in a potentially unsafe way, then it's important to document to clients of the library that the function should only be used with trusted inputs. If the function is not documented as being potentially unsafe, then a client may inadvertently use inputs containing unsafe HTML fragments, and thereby leave the client vulnerable to cross-site scripting attacks.

Recommendation

Document all library functions that can lead to cross-site scripting attacks, and guard against unsafe inputs where dynamic HTML construction is not intended.

Example

The following example has a library function that renders a boldface name by writing to the innerHTML property of an element.

module.exports=functionshowBoldName(name){document.getElementById('name').innerHTML="<b>"+name+"</b>";}

This library function, however, does not escape unsafe HTML, and a client that calls the function with user-supplied input may be vulnerable to cross-site scripting attacks.

The library could either document that this function should not be used with unsafe inputs, or use safe APIs such as innerText.

module.exports=functionshowBoldName(name){constbold=document.createElement('b');bold.innerText=name;document.getElementById('name').appendChild(bold);}

Alternatively, an HTML sanitizer can be used to remove unsafe content.

conststriptags=require('striptags');module.exports=functionshowBoldName(name){document.getElementById('name').innerHTML="<b>"+striptags(name)+"</b>";}

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-079/UnsafeJQueryPlugin.qhelp

Unsafe jQuery plugin

Library plugins, such as those for the jQuery library, are often configurable through options provided by the clients of the plugin. Clients, however, do not know the implementation details of the plugin, so it is important to document the capabilities of each option. The documentation for the plugin options that the client is responsible for sanitizing is of particular importance. Otherwise, the plugin may write user input (for example, a URL query parameter) to a web page without properly sanitizing it first, which allows for a cross-site scripting vulnerability in the client application through dynamic HTML construction.

Recommendation

Document all options that can lead to cross-site scripting attacks, and guard against unsafe inputs where dynamic HTML construction is not intended.

Example

The following example shows a jQuery plugin that selects a DOM element, and copies its text content to another DOM element. The selection is performed by using the plugin option sourceSelector as a CSS selector.

jQuery.fn.copyText=function(options){// BAD may evaluate `options.sourceSelector` as HTMLvarsource=jQuery(options.sourceSelector),text=source.text();jQuery(this).text(text);}

This is, however, not a safe plugin, since the call to jQuery interprets sourceSelector as HTML if it is a string that starts with <.

Instead of documenting that the client is responsible for sanitizing sourceSelector, the plugin can use jQuery.find to always interpret sourceSelector as a CSS selector:

jQuery.fn.copyText=function(options){// GOOD may not evaluate `options.sourceSelector` as HTMLvarsource=jQuery.find(options.sourceSelector),text=source.text();jQuery(this).text(text);}

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• jQuery: Plugin creation.
• Bootstrap: XSS vulnerable bootstrap plugins.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-079/Xss.qhelp

Client-side cross-site scripting

Directly writing user input (for example, a URL query parameter) to a webpage without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called DOM-based cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the page, or one of the other solutions that are mentioned in the references.

Example

The following example shows part of the page URL being written directly to the document, leaving the website vulnerable to cross-site scripting.

functionsetLanguageOptions(){varhref=document.location.href,deflt=href.substring(href.indexOf("default=")+8);document.write("<OPTION value=1>"+deflt+"</OPTION>");document.write("<OPTION value=2>English</OPTION>");}

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp

DOM text reinterpreted as HTML

Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability.

A webpage with this vulnerability reads text from the DOM, and afterwards adds the text as HTML to the DOM. Using text from the DOM as HTML effectively unescapes the text, and thereby invalidates any escaping done on the text. If an attacker is able to control the safe sanitized text, then this vulnerability can be exploited to perform a cross-site scripting attack.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing text to the page, or one of the other solutions that are mentioned in the References section below.

Example

The following example shows a webpage using a data-target attribute to select and manipulate a DOM element using the JQuery library. In the example, the data-target attribute is read into the target variable, and the $ function is then supposed to use the target variable as a CSS selector to determine which element should be manipulated.

$("button").click(function(){vartarget=$(this).attr("data-target");$(target).hide();});

However, if an attacker can control the data-target attribute, then the value of target can be used to cause the $ function to execute arbitrary JavaScript.

The above vulnerability can be fixed by using $.find instead of $. The $.find function will only interpret target as a CSS selector and never as HTML, thereby preventing an XSS attack.

$("button").click(function(){vartarget=$(this).attr("data-target");$.find(target).hide();});

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-116/IncompleteHtmlAttributeSanitization.qhelp

Incomplete HTML attribute sanitization

Sanitizing untrusted input for HTML meta-characters is a common technique for preventing cross-site scripting attacks. Usually, this is done by escaping <, >, & and ". However, the context in which the sanitized value is used decides the characters that need to be sanitized.

As a consequence, some programs only sanitize < and > since those are the most common dangerous characters. The lack of sanitization for " is problematic when an incompletely sanitized value is used as an HTML attribute in a string that later is parsed as HTML.

Recommendation

Sanitize all relevant HTML meta-characters when constructing HTML dynamically, and pay special attention to where the sanitized value is used.

An even safer alternative is to design the application so that sanitization is not needed, for instance by using HTML templates that are explicit about the values they treat as HTML.

Example

The following example code writes part of an HTTP request (which is controlled by the user) to an HTML attribute of the server response. The user-controlled value is, however, not sanitized for ". This leaves the website vulnerable to cross-site scripting since an attacker can use a string like " onclick="alert(42) to inject JavaScript code into the response.

varapp=require('express')();app.get('/user/:id',function(req,res){letid=req.params.id;id=id.replace(/<|>/g,"");// BADletuserHtml=`<div data-id="${id}">${getUserName(id)||"Unknown name"}</div>`;// ...res.send(prefix+userHtml+suffix);});

Sanitizing the user-controlled data for " helps prevent the vulnerability:

varapp=require('express')();app.get('/user/:id',function(req,res){letid=req.params.id;id=id.replace(/<|>|&|"/g,"");// GOODletuserHtml=`<div data-id="${id}">${getUserName(id)||"Unknown name"}</div>`;// ...res.send(prefix+userHtml+suffix);});

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP Types of Cross-Site.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.
• Common Weakness Enumeration: CWE-20.

javascript/ql/src/Security/CWE-116/UnsafeHtmlExpansion.qhelp

Unsafe expansion of self-closing HTML tag

Sanitizing untrusted input for HTML meta-characters is a common technique for preventing cross-site scripting attacks. But even a sanitized input can be dangerous to use if it is modified further before a browser treats it as HTML. A seemingly innocent transformation that expands a self-closing HTML tag from <div attr="{sanitized}"/> to <div attr="{sanitized}"></div> may in fact cause cross-site scripting vulnerabilities.

Recommendation

Use a well-tested sanitization library if at all possible, and avoid modifying sanitized values further before treating them as HTML.

An even safer alternative is to design the application so that sanitization is not needed, for instance by using HTML templates that are explicit about the values they treat as HTML.

Example

The following function transforms a self-closing HTML tag to a pair of open/close tags. It does so for all non-img and non-area tags, by using a regular expression with two capture groups. The first capture group corresponds to the name of the tag, and the second capture group to the content of the tag.

functionexpandSelfClosingTags(html){varrxhtmlTag=/<(?!img|area)(([a-z][^\w\/>]*)[^>]*)\/>/gi;returnhtml.replace(rxhtmlTag,"<$1></$2>");// BAD}

While it is generally known regular expressions are ill-suited for parsing HTML, variants of this particular transformation pattern have long been considered safe.

However, the function is not safe. As an example, consider the following string:

<divalt="<x" title="/><img src=url404 onerror=alert(1)>"/>

When the above function transforms the string, it becomes a string that results in an alert when a browser treats it as HTML.

<divalt="<x" title="></x" ><imgsrc=url404onerror=alert(1)>"/>

References

• jQuery: Security fixes in jQuery 3.5.0
• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP Types of Cross-Site.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

javascript/ql/src/Security/CWE-338/InsecureRandomness.qhelp

Insecure randomness

Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, such as a password, makes it easier for an attacker to predict the value.

Pseudo-random number generators generate a sequence of numbers that only approximates the properties of random numbers. The sequence is not truly random because it is completely determined by a relatively small set of initial values, the seed. If the random number generator is cryptographically weak, then this sequence may be easily predictable through outside observations.

Recommendation

Use a cryptographically secure pseudo-random number generator if the output is to be used in a security-sensitive context. As a rule of thumb, a value should be considered "security-sensitive" if predicting it would allow the attacker to perform an action that they would otherwise be unable to perform. For example, if an attacker could predict the random password generated for a new user, they would be able to log in as that new user.

For JavaScript on the NodeJS platform, crypto.getRandomBytes provides a cryptographically secure pseudo-random byte generator. Note that the conversion from bytes to numbers can introduce bias that breaks the security.

For JavaScript in the browser, crypto.getRandomValues provides a cryptographically secure pseudo-random number generator.

Example

The following examples show different ways of generating a password.

In the first case, we generate a fresh password by appending a random integer to the end of a static string. The random number generator used (Math.random) is not cryptographically secure, so it may be possible for an attacker to predict the generated password.

functioninsecurePassword(){// BAD: the random suffix is not cryptographically securevarsuffix=Math.random();varpassword="myPassword"+suffix;returnpassword;}

In the second example, a cryptographically secure random number generator is used for the same purpose. In this case, it is much harder to predict the generated integers.

functionsecurePassword(){// GOOD: the random suffix is cryptographically securevarsuffix=window.crypto.getRandomValues(newUint32Array(1))[0];varpassword="myPassword"+suffix;// GOOD: if a random value between 0 and 1 is desiredvarsecret=window.crypto.getRandomValues(newUint32Array(1))[0]*Math.pow(2,-32);}

References

• Wikipedia: Pseudo-random number generator.
• Mozilla Developer Network: Crypto: getRandomValues().
• NodeJS: crypto.randomBytes
• Common Weakness Enumeration: CWE-338.

javascript/ql/src/Statements/DanglingElse.qhelp

Misleading indentation of dangling 'else'

In JavaScript, an else clause is always associated with the closest preceding if statement that does not already have an else clause. It is good practice to use indentation to clarify this structure by indenting matching if ... else pairs by the same amount of whitespace.

Indenting the else clause of a nested if statement to suggest that it matches an outer if statement (instead of the one it actually belongs to) is confusing to readers and may even indicate a bug in the program logic.

Recommendation

Ensure that matching if ... else pairs are indented accordingly.

Example

In the following example, the else on line 5 belongs to the if on line 3, while its indentation wrongly suggests that it belongs to the if on line 2.

functionf(){if(cond1())if(cond2())return23;elsereturn42;return56;}

To correct this issue, indent the else on line 5 further:

functionf(){if(cond1())if(cond2())return23;elsereturn42;return56;}

Confusion about which if belongs to which else can also be avoided by always enclosing the branches of an if statement in curly braces:

functionf(){if(cond1()){if(cond2()){return23;}else{return42;}}return56;}

References

• Wikipedia: Dangling else.
• Common Weakness Enumeration: CWE-483.

javascript/ql/src/Statements/UnreachableStatement.qhelp

Unreachable statement

An unreachable statement almost always indicates missing code or a latent bug and should be examined carefully.

Recommendation

Examine the surrounding code to determine why the statement has become unreachable. If it is no longer needed, remove the statement.

Example

In the following example, a spurious semicolon after the if condition at line 2 makes the return statement on line 4 unreachable: the function will always execute the return statement on line 3 first, so it will never reach line 4.

functionf(){if(someCond());return23;return42;}

To correct this issue, remove the spurious semicolon:

functionf(){if(someCond())return23;return42;}

References

• Wikipedia: Unreachable code.
• Common Weakness Enumeration: CWE-561.

javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-079/Xss.qhelp

Client-side cross-site scripting with additional heuristic sources

Directly writing user input (for example, a URL query parameter) to a webpage without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called DOM-based cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the page, or one of the other solutions that are mentioned in the references.

Example

The following example shows part of the page URL being written directly to the document, leaving the website vulnerable to cross-site scripting.

functionsetLanguageOptions(){varhref=document.location.href,deflt=href.substring(href.indexOf("default=")+8);document.write("<OPTION value=1>"+deflt+"</OPTION>");document.write("<OPTION value=2>English</OPTION>");}

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

python/ql/src/Metrics/FLinesOfDuplicatedCode.qhelp

Duplicated lines in files

This metric measures the number of lines in a file that are contained within a block that is duplicated elsewhere. These lines may include code, comments and whitespace, and the duplicate block may be in this file or in another file.

A file that contains many lines that are duplicated within the code base is problematic for a number of reasons.

Duplicated code increases overall code size, making the code base harder to maintain and harder to understand. It also becomes harder to fix bugs, since a programmer applying a fix to one copy has to always remember to update other copies accordingly. Finally, code duplication is generally an indication of a poorly designed or hastily written code base, which typically suffers from other problems as well.

Recommendation

Refactor files with lots of duplicated code to extract the common code into a shared library or module.

References

• Wikipedia: Duplicate code.
• M. Fowler, Refactoring. Addison-Wesley, 1999.

ruby/ql/src/experimental/insecure-randomness/InsecureRandomness.qhelp

Insecure randomness

Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, such as a password, makes it easier for an attacker to predict the value. Pseudo-random number generators generate a sequence of numbers that only approximates the properties of random numbers. The sequence is not truly random because it is completely determined by a relatively small set of initial values, the seed. If the random number generator is cryptographically weak, then this sequence may be easily predictable through outside observations.

Recommendation

When generating values for use in security-sensitive contexts, it's essential to utilize a cryptographically secure pseudo-random number generator. As a general guideline, a value should be deemed "security-sensitive" if its predictability would empower an attacker to perform actions that would otherwise be beyond their reach. For instance, if an attacker could predict a newly generated user's random password, they would gain unauthorized access to that user's account. For Ruby, SecureRandom provides a cryptographically secure pseudo-random number generator. rand is not cryptographically secure, and should be avoided in security contexts. For contexts which are not security sensitive, Random may be preferable as it has a more convenient interface.

Example

The following examples show different ways of generating a password.

The first example uses Random.rand() which is not for security purposes

defgenerate_password()chars=('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a + ['!','@','#','$','%']# BAD: rand is not cryptographically securepassword=(1..10).collect{chars[rand(chars.size)]}.joinendpassword=generate_password

In the second example, the password is generated using SecureRandom.random_bytes() which is a cryptographically secure method.

require'securerandom'defgenerate_password()chars=('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a + ['!','@','#','$','%']# GOOD: SecureRandom is cryptographically securepassword=SecureRandom.random_bytes(10).each_byte.mapdo |byte|
chars[byte % chars.length]end.joinendpassword=generate_password()

References

• Wikipedia: Pseudo-random number generator.
• Common Weakness Enumeration: CWE-338.
• Ruby-doc: Random.
• Common Weakness Enumeration: CWE-338.

ruby/ql/src/queries/security/cwe-079/ReflectedXSS.qhelp

Reflected server-side cross-site scripting

Directly writing user input (for example, an HTTP request parameter) to a webpage, without properly sanitizing the input first, allows for a cross-site scripting vulnerability.

Recommendation

To guard against cross-site scripting, escape user input before writing it to the page. Some frameworks, such as Rails, perform this escaping implicitly and by default.

Take care when using methods such as html_safe or raw. They can be used to emit a string without escaping it, and should only be used when the string has already been manually escaped (for example, with the Rails html_escape method), or when the content is otherwise guaranteed to be safe (such as a hard-coded string).

Example

The following example is safe because the params[:user_name] content within the output tags will be HTML-escaped automatically before being emitted.

<p>Hello <%= params[:user_name] %>!</p>

However, the following example is unsafe because user-controlled input is emitted without escaping, since it is marked as html_safe.

<p>Hello <%= params[:user_name].html_safe %>!</p>

References

• OWASP: XSS Ruby on Rails Cheatsheet.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

ruby/ql/src/queries/security/cwe-079/StoredXSS.qhelp

Stored cross-site scripting

Directly writing an uncontrolled stored value (for example, a database field) to a webpage, without properly sanitizing the value first, allows for a cross-site scripting vulnerability.

This kind of vulnerability is also called stored cross-site scripting, to distinguish it from other types of cross-site scripting.

Recommendation

To guard against stored cross-site scripting, consider escaping before using uncontrolled stored values to create HTML content. Some frameworks, such as Rails, perform this escaping implicitly and by default.

Take care when using methods such as html_safe or raw. They can be used to emit a string without escaping it, and should only be used when the string has already been manually escaped (for example, with the Rails html_escape method), or when the content is otherwise guaranteed to be safe (such as a hard-coded string).

Example

The following example is safe because the user.name content within the output tags will be HTML-escaped automatically before being emitted.

<% user = User.find(1) %>
<p>Hello <%= user.name %>!</p>

However, the following example may be unsafe because user.name is emitted without escaping, since it is marked as html_safe. If the name is not sanitized before being written to the database, then an attacker could use this to insert arbitrary content into the HTML output, including scripts.

<% user = User.find(1) %>
<p>Hello <%= user.name.html_safe %>!</p>

In the next example, content from a file on disk is inserted literally into HTML content. This approach is sometimes used to load script content, such as extensions for a web application, from files on disk. Care should taken in these cases to ensure both that the loaded files are trusted, and that the file cannot be modified by untrusted users.

<script>
<%= File.read(File.join(SCRIPT_DIR, "script.js")).html_safe %>
</script>

References

• OWASP: XSS Ruby on Rails Cheatsheet.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

ruby/ql/src/queries/security/cwe-079/UnsafeHtmlConstruction.qhelp

Unsafe HTML constructed from library input

When a library function dynamically constructs HTML in a potentially unsafe way, then it's important to document to clients of the library that the function should only be used with trusted inputs. If the function is not documented as being potentially unsafe, then a client may inadvertently use inputs containing unsafe HTML fragments, and thereby leave the client vulnerable to cross-site scripting attacks.

Recommendation

Document all library functions that can lead to cross-site scripting attacks, and guard against unsafe inputs where dynamic HTML construction is not intended.

Example

The following example has a library function that renders a boldface name by creating a string containing a <b> with the name embedded in it.

classUsersController < ActionController::Base# BAD - create a user description, where the name is not escapeddefcreate_user_description(name)"<b>#{name}</b>".html_safeendend

This library function, however, does not escape unsafe HTML, and a client that calls the function with user-supplied input may be vulnerable to cross-site scripting attacks.

The library could either document that this function should not be used with unsafe inputs, or escape the input before embedding it in the HTML fragment.

classUsersController < ActionController::Base# Good - create a user description, where the name is escapeddefcreate_user_description(name)"<b>#{ERB::Util.html_escape(name)}</b>".html_safeendend

References

• OWASP: DOM based XSS Prevention Cheat Sheet.
• OWASP: XSS (Cross Site Scripting) Prevention Cheat Sheet.
• OWASP DOM Based XSS.
• OWASP Types of Cross-Site Scripting.
• Wikipedia: Cross-site scripting.
• Common Weakness Enumeration: CWE-79.
• Common Weakness Enumeration: CWE-116.

ruby/ql/src/queries/variables/DeadStoreOfLocal.qhelp

Useless assignment to local variable

A value is assigned to a local variable, but either that variable is never read later on, or its value is always overwritten before being read. This means that the original assignment has no effect, and could indicate a logic error or incomplete code.

Recommendation

Ensure that you check the control and data flow in the method carefully. If a value is really not needed, consider omitting the assignment. Be careful, though: if the right-hand side has a side-effect (like performing a method call), it is important to keep this to preserve the overall behavior.

Example

In the following example, the return value of the call to send on line 2 is assigned to the local variable result, but then never used.

deff(x)result=send(x)waitForResponsereturngetResponseend

Assuming that send returns a status code indicating whether the operation succeeded or not, the value of result should be checked, perhaps like this:

deff(x)result=send(x)# check for errorif(result == -1)raise"Unable to send, check network."endwaitForResponsereturngetResponseend

References

• Wikipedia: Dead store.
• Common Weakness Enumeration: CWE-563.

[jketema]

We have some files that need to be synchronized, but apparently are no longer. This makes CI fail and will need to be fixed before we can merge:

ERROR: /home/runner/work/codeql/codeql/config/sync-files.py:0 - Files from group 'FLinesOfDuplicatedCodeCommon.inc.qhelp' not in sync.
...
ERROR: /home/runner/work/codeql/codeql/config/sync-files.py:0 - ./java/ql/src/Metrics/Files/FLinesOfDuplicatedCodeCommon.inc.qhelp
ERROR: /home/runner/work/codeql/codeql/config/sync-files.py:0 - ./javascript/ql/src/Metrics/FLinesOfDuplicatedCodeCommon.inc.qhelp
ERROR: /home/runner/work/codeql/codeql/config/sync-files.py:0 - ./python/ql/src/Metrics/FLinesOfDuplicatedCodeCommon.inc.qhelp

[geoffw0]

We have some files that need to be synchronized, but apparently are no longer.

Let us know if it isn't clear how to fix this (most likely just overwriting the copies with your modified files).

[miachillgood]

Updated the Python synchronized copy in ef1b821. The Java, JavaScript, and Python files now contain the same HTTPS reference.

[jketema]

LGTM