Hi,
I am not sure this is the proper place to make suggestions on CWE examples, but
I have noticed a problem with CWE 129 - Example 3 (This is also a problem with
CWE 125 - Example 1, and CWE 839 - Example 3). In each of the examples, the bad
code has an insufficient check for out-of-bounds read, and the fix is to
correct the check on out-of-bounds read. That part is fine, the issue, is when
the insufficient check for the out-of-bounds read fails, the example code uses
the invalid index to print out a value from the array, creating a new
out-of-bounds read not covered by any check. The suggested fix does not cover
this and given the description of the example, so I do not think this second
out-of-bounds read was intentional.
One consequence of this issue is that if you are using the example code and the
code after the fix to test code review techniques or static analysis, the code
after the fix will still be flagged with an out-of-bounds read and that may
confuse reviewers or developers of static analysis tools. With a very small
change, I think this example can become more useful for these purposes.
The current example is:
int getValueFromArray(int *array, int len, int index) {
int value;
// check that the array index is less than the maximum
// length of the array
if (index < len) {
// get the value at the specified index of the array
value = array[index];
}
// if array index is invalid then output error message
// and return value indicating error
else {
printf("Value is: %d\n", array[index]);
value = -1;
}
return value;
}
With a fix that changes only the line "if (index < len) {" to
if (index >= 0 && index < len) {
My suggestion is to have the example print out the invalid index instead of a
value using the invalid index, so the changed code would be
int getValueFromArray(int *array, int len, int index) {
int value;
// check that the array index is less than the maximum
// length of the array
if (index < len) {
// get the value at the specified index of the array
value = array[index];
}
// if array index is invalid then output error message
// and return value indicating error
else {
printf("Invalid index used: %d\n", index);
value = -1;
}
return value;
}
The fix would remain:
if (index >= 0 && index < len) {
This change should apply to all CWE's using this example (I've noticed CWE-125,
CWE-129 and CWE 839 using this example, but I would not be surprised if other
instances are as well)
Does my suggested change make sense? Again, please correct me if this is the
wrong forum to suggest this change.
With regards,
John Thomas
Field Application Engineer Technical Lead
LDRA Technology
