Extra checking to prevent loop counter from wrapping around #1766

kevinbackhouse · 2021-07-07T15:10:29Z

The bug is this loop condition in CrwMap::decodeArray. The loop counter, c, is a uint16_t, so if the component size is greater than 2 * 0xffff then c will wrap around and the loop won't terminate.

There are 3 commits:

Regression test
Fix for GHSA-hqjh-hpv8-8r9p
Defensive fixes for similar looking loop conditions

So the 3rd commit is optional. I'll add comments to the code review explaining those changes.

src/actions.cpp

kevinbackhouse · 2021-07-07T15:18:26Z

src/basicio.cpp

-            blockIndex--;
-            blockSize = (long)p_->blocksMap_[blockIndex].getSize();


Unfortunately, this code doesn't seem to be covered by any of our tests. But I am pretty sure that if this loop doesn't terminate early then, on the final iteration of the loop, this array access will attempt to access blocksMap_[-1]. Since all this code is doing is preloading blockSize for the next iteration, it is better to just load it at the beginning of the loop.

kevinbackhouse · 2021-07-07T15:24:05Z

src/convert.cpp

-        for (int i = 0; i < pos->count(); ++i) {
+        for (long i = 0; i < pos->count(); ++i) {


The return type count is long and the parameter type of toString is long, so long is a better choice for the type of i.

kevinbackhouse · 2021-07-07T15:24:58Z

src/convert.cpp

-        for (unsigned i = 0; i < value.length(); ++i) {
+        for (size_t i = 0; i < value.length(); ++i) {


value is a std::string, so size_t is a better choice for the type of i.

kevinbackhouse · 2021-07-07T15:28:36Z

src/crwimage_int.cpp

+        const uint32_t component_size = ciffComponent.size();
+        enforce(component_size % 2 == 0, kerCorruptedMetadata);
+        enforce(component_size/2 <= static_cast<uint32_t>(std::numeric_limits<uint16_t>::max()), kerCorruptedMetadata);
+        const uint16_t num_components = static_cast<uint16_t>(component_size/2);
        uint16_t c = 1;
-        while (uint32_t(c)*2 < ciffComponent.size()) {
+        while (c < num_components) {


This is the fix for GHSA-hqjh-hpv8-8r9p
The value of ciffComponent.size() is bounds-checked before the start of the loop and the correct number of iterations is precomputed.

kevinbackhouse · 2021-07-07T15:30:37Z

src/crwimage_int.cpp

-            if (ifdId == canonCsId && c == 23 && ciffComponent.size() > 50) n = 3;
+            if (ifdId == canonCsId && c == 23 && component_size >= 52) n = 3;


The number 50 looks wrong to me. If c == 23 and n == 3 then the read below will read the 6 bytes starting at offset 46. Therefore the size needs to be at least 52.

kevinbackhouse · 2021-07-07T15:31:51Z

src/exiv2.cpp

-        for (unsigned int i = 0; i < input.length(); ++i) {
+        for (size_t i = 0; i < input.length(); ++i) {


input is a std::string, so size_t is a better type for i.

kevinbackhouse · 2021-07-07T15:32:43Z

src/exiv2.cpp

-                if (input.length() - 4 > i) {
+                if (input.length() >= 4 && input.length() - 4 > i) {


Make sure that input.length() - 4 cannot overflow.

kevinbackhouse · 2021-07-07T15:35:24Z

src/iptc.cpp

-        uint32_t i = 0;
-        while (i < bytes.size() - 3 && bytes.at(i) != 0x1c)
+        size_t i = 0;
+        while (i + 3 < bytes.size() && bytes.at(i) != 0x1c)
            i++;


The subtraction bytes.size() - 3 could overflow. i + 3 cannot, because i is initially 0 and only incremented by 1 on each loop iteration.

kevinbackhouse · 2021-07-07T15:39:18Z

src/iptc.cpp

-        while (i < bytes.size() - 3) {
+        while (i + 3 < bytes.size()) {


Hmm. I just realized that my logic that i + 3 cannot overflow doesn't work here because i is incremented with a += in this loop. I'll add another commit to fix it.

kevinbackhouse · 2021-07-07T15:40:22Z

src/tags_int.cpp

-            for (uint16_t i = 0; i < value.count(); i++ ) { // for each element in value array
+            long count = value.count();
+            enforce(0 <= count && count <= std::numeric_limits<uint16_t>::max(), kerCorruptedMetadata);
+            for (uint16_t i = 0; i < count; i++ ) { // for each element in value array


Make sure that value.count() doesn't exceed the range of a uint16_t.

kevinbackhouse · 2021-07-07T15:41:12Z

src/tags_int.cpp

-        for (unsigned int i = 0; i < stringValue.length(); ++i) {
+        for (size_t i = 0; i < stringValue.length(); ++i) {


stringValue is a std::string, so size_t is a better choice for i.

kevinbackhouse · 2021-07-07T15:43:22Z

src/tiffvisitor_int.cpp

-                    for ( int16_t k = 0 ; k < records[i].size ; k++ ) s << " " << ints.at(nStart++);
+                    for ( uint16_t k = 0 ; k < records[i].size ; k++ ) s << " " << ints.at(nStart++);


The type of records[i].size is uint16_t, so uint16_t is a better choice for k.

kevinbackhouse · 2021-07-07T15:52:27Z

src/iptc.cpp

+        size_t i = 0;
        while (i < bytes.size() - 3 && bytes.at(i) != 0x1c)


The type of bytes.size() is size_t, so size_t is a better choice for i.

kevinbackhouse · 2021-07-07T15:53:10Z

src/iptc.cpp

+        if (bytes.size() < 3) {
+            return;
+        }


Prevent integer overflow in bytes.size() - 3 below.

kevinbackhouse · 2021-07-07T16:05:15Z

I used two CodeQL queries to find loop conditions similar to the original bug. Both queries have quite a few false positives, so I ignored some of the results.

The first query looks for loop conditions of the form a < b where the value of b could be larger than the type of a is capable of representing. For example: a is a uint16_t and b is a size_t.

import cpp
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils

from Loop loop, RelationalOperation cmp, Expr a, Expr b, LocalVariable v
where loop.getCondition().getAChild*() = cmp
and a = cmp.getLesserOperand()
and b = cmp.getGreaterOperand()
and exprMaxVal(a) < upperBound(b.getFullyConverted())
and a.getAChild*() = v.getAnAccess()
select cmp, a, b

The second query looks for loop conditions that do arithmetic in the comparison. For example: a < size-10.

import cpp

// Find loop conditions like this: `while (a < size-10)`.
// Conditions like that are often an overflow risk.
from Loop loop, RelationalOperation cmp, BinaryArithmeticOperation binop
where
  loop.getCondition().getAChild*() = cmp and
  binop = cmp.getAnOperand() and
  not binop.isConstant() and
  // Ignore results in the standard libraries and in the xmpsdk subdirectory.
  exists (string path |
    path = cmp.getLocation().getFile().getRelativePath() and
    not path.matches("xmpsdk/%"))
select binop, "Arithmetic in loop condition."

Extra checking to prevent loop counter from wrapping around (backport #1766)

kevinbackhouse added 3 commits July 6, 2021 16:31

Regression test for GHSA-hqjh-hpv8-8r9p

a35a2fd

Extra checking to prevent the loop counter from wrapping around.

97c4880

Defensive coding changes to avoid integer overflow in loop conditions.

1b204d9

kevinbackhouse added the forward-to-main Forward changes in a 0.28.x PR to main with Mergify label Jul 7, 2021

kevinbackhouse added this to the v0.27.5 milestone Jul 7, 2021

kevinbackhouse commented Jul 7, 2021

View reviewed changes

src/actions.cpp Show resolved Hide resolved

kevinbackhouse commented Jul 7, 2021

View reviewed changes

Better fix for potential integer overflow in bytes.size() - 3.

ee8af71

kevinbackhouse commented Jul 7, 2021

View reviewed changes

kevinbackhouse added the bug label Jul 7, 2021

kevinbackhouse changed the title ~~Fix ghsa hqjh hpv8 8r9p~~ Extra checking to prevent loop counter from wrapping around Jul 7, 2021

Type of escapeStart should be size_t.

e74d8ac

hassec approved these changes Jul 16, 2021

View reviewed changes

kevinbackhouse merged commit c4861fe into Exiv2:0.27-maintenance Jul 17, 2021

mergify bot mentioned this pull request Jul 17, 2021

Extra checking to prevent loop counter from wrapping around (backport #1766) #1787

Merged

kevinbackhouse added a commit that referenced this pull request Jul 25, 2021

Merge pull request #1787 from Exiv2/mergify/bp/main/pr-1766

944e68f

Extra checking to prevent loop counter from wrapping around (backport #1766)

kevinbackhouse deleted the Fix-GHSA-hqjh-hpv8-8r9p branch July 26, 2021 11:57

kevinbackhouse mentioned this pull request Jul 26, 2021

Fix build error when EXIV2_DEBUG_MESSAGES is enabled #1801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra checking to prevent loop counter from wrapping around #1766

Extra checking to prevent loop counter from wrapping around #1766

kevinbackhouse commented Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021 •

edited

Loading

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse Jul 7, 2021

kevinbackhouse commented Jul 7, 2021

		blockIndex--;
		blockSize = (long)p_->blocksMap_[blockIndex].getSize();

		for (int i = 0; i < pos->count(); ++i) {
		for (long i = 0; i < pos->count(); ++i) {

		for (unsigned i = 0; i < value.length(); ++i) {
		for (size_t i = 0; i < value.length(); ++i) {

		if (ifdId == canonCsId && c == 23 && ciffComponent.size() > 50) n = 3;
		if (ifdId == canonCsId && c == 23 && component_size >= 52) n = 3;

		for (unsigned int i = 0; i < input.length(); ++i) {
		for (size_t i = 0; i < input.length(); ++i) {

		if (input.length() - 4 > i) {
		if (input.length() >= 4 && input.length() - 4 > i) {

		while (i < bytes.size() - 3) {
		while (i + 3 < bytes.size()) {

		for (unsigned int i = 0; i < stringValue.length(); ++i) {
		for (size_t i = 0; i < stringValue.length(); ++i) {

		for ( int16_t k = 0 ; k < records[i].size ; k++ ) s << " " << ints.at(nStart++);
		for ( uint16_t k = 0 ; k < records[i].size ; k++ ) s << " " << ints.at(nStart++);

		size_t i = 0;
		while (i < bytes.size() - 3 && bytes.at(i) != 0x1c)

Extra checking to prevent loop counter from wrapping around #1766

Extra checking to prevent loop counter from wrapping around #1766

Conversation

kevinbackhouse commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbackhouse Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbackhouse commented Jul 7, 2021

kevinbackhouse Jul 7, 2021 •

edited

Loading