C++: Update documentation for cpp/uncontrolled-allocation-size to clarify its scope

[paldepind]

Change the documentation of the cpp/uncontrolled-allocation-size query to clarify its intended behavior/scope.

#11215

[github-actions]

QHelp previews:

cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp

Uncontrolled allocation size

This code allocates memory using a size value based on user input, with no apparent bound on its magnitude being established. This allows for arbitrary amounts of memory to be allocated.

If the allocation size is calculated by multiplying user input by a sizeof expression, the multiplication can overflow. When an integer multiplication overflows in C, the result wraps around and can be much smaller than intended. A later attempt to write data into the allocated memory can then be out of bounds.

Recommendation

Guard all integer parameters that come from an external user. Implement a guard with the expected range for the parameter and make sure that the input value meets both the minimum and maximum requirements for this range. If the input value fails this guard then reject the request before proceeding further. If the input value passes the guard then subsequent calculations should not overflow.

Example

int factor = atoi(getenv("BRANCHING_FACTOR"));

// BAD: This can allocate too little memory if factor is very large due to overflow.
char **root_node = (char **) malloc(factor * sizeof(char *));

// GOOD: Prevent overflow and unbounded allocation size by checking the input.
if (factor > 0 && factor <= 1000) {
    char **root_node = (char **) malloc(factor * sizeof(char *));
}

This code shows one way to guard that an input value is within the expected range. If factor fails the guard, then an error is returned, and the value is not used as an argument to the subsequent call to malloc. Without this guard, the allocated buffer might be too small to hold the data intended for it.

References

• The CERT Oracle Secure Coding Standard for C: INT04-C. Enforce limits on integer values originating from tainted sources.
• Common Weakness Enumeration: CWE-190.
• Common Weakness Enumeration: CWE-789.

[paldepind]

Changed this comment as this unbounded allocation size is bad per the updated description of the query.

[geoffw0]

Changes LGTM.

Can we change the query message as well ("This allocation size is derived from $@ and might overflow.") to put more focus on the tainted / unbounded allocation size than the possibility (which is likely but not certain) of overflow?

[paldepind]

Good idea, I missed that. I've updated the query message now as well 😄

[geoffw0]

Looks good to me.

If you add the ready-for-doc-review label, the docs team will check over your .qhelp and query metadata changes.

You should also add the no-change-note-required tag, which will inform CI that you've considered the changes and don't think they require a change note (unless you do, in which case see this doc and it goes here; roughly speaking, any change that affects query results or the way external developers would write queries of their own should have a change note).

[subatoi]

Looks great, thank you—only some nit comments. Assuming you're happy with the suggestions, feel free to go ahead and merge once you've got an approval from your team (there's no need for me to approve again)

Apologies also if the suggestions mess up some formatting

[subatoi]

Suggested change

	<p>This code allocates memory using a size value based on user input
	with no apparent bound on its magnitude being established. This allows
	for arbitrary amounts of memory being allocated.</p>
	<p>This code allocates memory using a size value based on user input, with no apparent bound on its magnitude being established. This allows for arbitrary amounts of memory to be allocated.</p>

[subatoi]

Suggested change

	<p>If the allocation size is calculated by multiplying user input by a
	<code>sizeof</code> expression the multiplication can overflow. When
	an integer multiplication overflows in C, the result wraps around and
	can be much smaller than intended. A later attempt to write data into
	the allocated memory can then be out-of-bounds.</p>
	<p>If the allocation size is calculated by multiplying user input by a
	<code>sizeof</code> expression, the multiplication can overflow. When
	an integer multiplication overflows in C, the result wraps around and
	can be much smaller than intended. A later attempt to write data into
	the allocated memory can then be out of bounds.</p>

[paldepind]

Thanks @subatoi. I've applied the suggested changes.

[subatoi]

Thank you!

[geoffw0]

👍