Skip to content

Commit

Permalink
Fix upAp7 signed integer overflow (#735)
Browse files Browse the repository at this point in the history
* Fix possible signed integer overflow in ijToIjk

* Update changelog

* reorder

* add _upAp7Checked and _upAp7rChecked for processing user input

* Add PR number to changelog

* update with macro tests

* force k to 0 in fuzzer

* Change per review: add an overall bounds check around the overflow checks
  • Loading branch information
isaacbrodsky committed Dec 22, 2022
1 parent 975de7a commit fc7f3dd
Show file tree
Hide file tree
Showing 12 changed files with 405 additions and 28 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ The public API of this library consists of the functions declared in file

### Fixed
- Fixed possible signed integer overflow in `h3NeighborRotations` (#707)
- Fixed possible signed integer overflow in `localIjToCell` (#706)
- Fixed possible signed integer overflow in `localIjToCell` (#706, #735)

### Changed
- `assert` on defensive code blocks that are not already covered. (#720)
Expand Down
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ set(OTHER_SOURCE_FILES
src/apps/testapps/testCoordIjk.c
src/apps/testapps/testH3Memory.c
src/apps/testapps/testH3Iterators.c
src/apps/testapps/testMathExtensions.c
src/apps/miscapps/cellToBoundaryHier.c
src/apps/miscapps/cellToLatLngHier.c
src/apps/miscapps/generateBaseCellNeighbors.c
Expand All @@ -258,6 +259,7 @@ set(OTHER_SOURCE_FILES
src/apps/fuzzers/fuzzerPolygonToCellsNoHoles.c
src/apps/fuzzers/fuzzerCellToChildPos.c
src/apps/fuzzers/fuzzerInternalAlgos.c
src/apps/fuzzers/fuzzerInternalCoordIjk.c
src/apps/benchmarks/benchmarkPolygonToCells.c
src/apps/benchmarks/benchmarkPolygon.c
src/apps/benchmarks/benchmarkCellsToLinkedMultiPolygon.c
Expand Down Expand Up @@ -512,6 +514,7 @@ if(BUILD_FUZZERS)
add_h3_fuzzer(fuzzerCellToChildPos src/apps/fuzzers/fuzzerCellToChildPos.c)
if(ENABLE_REQUIRES_ALL_SYMBOLS)
add_h3_fuzzer(fuzzerInternalAlgos src/apps/fuzzers/fuzzerInternalAlgos.c)
add_h3_fuzzer(fuzzerInternalCoordIjk src/apps/fuzzers/fuzzerInternalCoordIjk.c)
endif()
endif()

Expand Down
1 change: 1 addition & 0 deletions CMakeTests.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ add_h3_test(testCoordIjk src/apps/testapps/testCoordIjk.c)
add_h3_test(testBaseCells src/apps/testapps/testBaseCells.c)
add_h3_test(testPentagonIndexes src/apps/testapps/testPentagonIndexes.c)
add_h3_test(testH3Iterators src/apps/testapps/testH3Iterators.c)
add_h3_test(testMathExtensions src/apps/testapps/testMathExtensions.c)

add_h3_test_with_arg(testH3NeighborRotations src/apps/testapps/testH3NeighborRotations.c 0)
add_h3_test_with_arg(testH3NeighborRotations src/apps/testapps/testH3NeighborRotations.c 1)
Expand Down
4 changes: 4 additions & 0 deletions src/apps/fuzzers/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ In addition to the public API, the following internal functions of H3 are covere
| directionForNeighbor | [fuzzerInternalAlgos](./fuzzerInternalAlgos.c)
| h3SetToVertexGraph | [fuzzerInternalAlgos](./fuzzerInternalAlgos.c)
| _vertexGraphToLinkedGeo | [fuzzerInternalAlgos](./fuzzerInternalAlgos.c)
| _upAp7Checked | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
| _upAp7rChecked | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
| _ijkNormalizeCouldOverflow | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
| _ijkNormalize | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)

# libFuzzer Usage

Expand Down
53 changes: 53 additions & 0 deletions src/apps/fuzzers/fuzzerInternalCoordIjk.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright 2022 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/** @file
* @brief Fuzzer program for internal functions in coordijk.c
*/

#include "aflHarness.h"
#include "algos.h"
#include "h3api.h"
#include "utility.h"

typedef struct {
CoordIJK ijk;
} inputArgs;

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
if (size < sizeof(inputArgs)) {
return 0;
}
const inputArgs *args = (const inputArgs *)data;

// These functions require that input be non-negative
if (args->ijk.i >= 0 && args->ijk.j >= 0 && args->ijk.k >= 0) {
CoordIJK ijkCopy1 = args->ijk;
_upAp7Checked(&ijkCopy1);
CoordIJK ijkCopy2 = args->ijk;
_upAp7rChecked(&ijkCopy2);
}
// This function needs a guard check to be safe and that guard
// check assumes k = 0.
CoordIJK ijkCopy3 = args->ijk;
ijkCopy3.k = 0;
if (!_ijkNormalizeCouldOverflow(&ijkCopy3)) {
_ijkNormalize(&ijkCopy3);
}

return 0;
}

AFL_HARNESS_MAIN(sizeof(inputArgs));
32 changes: 32 additions & 0 deletions src/apps/testapps/testCellToLocalIj.c
Original file line number Diff line number Diff line change
Expand Up @@ -307,4 +307,36 @@ SUITE(h3ToLocalIj) {
t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
"High magnitude J and I components fail");
}

TEST(localIjToCell_overflow_particularCases) {
H3Index origin;
setH3Index(&origin, 2, 2, CENTER_DIGIT);
H3Index originRes3;
setH3Index(&originRes3, 2, 2, CENTER_DIGIT);

CoordIJ ij = {.i = 553648127, .j = -2145378272};
H3Index out;
t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
"Particular high magnitude J and I components fail (1)");

ij.i = INT32_MAX - 10;
ij.j = -11;
t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
"Particular high magnitude J and I components fail (2)");

ij.i = 553648127;
ij.j = -2145378272;
t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
"Particular high magnitude J and I components fail (3)");

ij.i = INT32_MAX - 10;
ij.j = -10;
t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
"Particular high magnitude J and I components fail (4)");

ij.i = INT32_MAX - 10;
ij.j = -9;
t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
"Particular high magnitude J and I components fail (5)");
}
}
42 changes: 42 additions & 0 deletions src/apps/testapps/testCoordIjk.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,46 @@ SUITE(coordIjk) {
_neighbor(&ijk, INVALID_DIGIT);
t_assert(_ijkMatches(&ijk, &i), "Invalid neighbor is self");
}

TEST(_upAp7Checked) {
CoordIJK ijk;

_setIJK(&ijk, 0, 0, 0);
t_assertSuccess(_upAp7Checked(&ijk));
_setIJK(&ijk, INT32_MAX, 0, 0);
t_assert(_upAp7Checked(&ijk) == E_FAILED, "i + i overflows");
_setIJK(&ijk, INT32_MAX / 2, 0, 0);
t_assert(_upAp7Checked(&ijk) == E_FAILED, "i * 3 overflows");
_setIJK(&ijk, 0, INT32_MAX, 0);
t_assert(_upAp7Checked(&ijk) == E_FAILED, "j + j overflows");
// This input should be invalid because j < 0
_setIJK(&ijk, INT32_MAX / 3, -2, 0);
t_assert(_upAp7Checked(&ijk) == E_FAILED, "(i * 3) - j overflows");
_setIJK(&ijk, INT32_MAX / 3, INT32_MAX / 2, 0);
t_assert(_upAp7Checked(&ijk) == E_FAILED, "i + (j * 2) overflows");
// This input should be invalid because j < 0
_setIJK(&ijk, -1, 0, 0);
t_assert(_upAp7Checked(&ijk) == E_SUCCESS, "i < 0 succeeds");
}

TEST(_upAp7rChecked) {
CoordIJK ijk;

_setIJK(&ijk, 0, 0, 0);
t_assertSuccess(_upAp7rChecked(&ijk));
_setIJK(&ijk, INT32_MAX, 0, 0);
t_assert(_upAp7rChecked(&ijk) == E_FAILED, "i + i overflows");
_setIJK(&ijk, 0, INT32_MAX, 0);
t_assert(_upAp7rChecked(&ijk) == E_FAILED, "j + j overflows");
_setIJK(&ijk, 0, INT32_MAX / 2, 0);
t_assert(_upAp7rChecked(&ijk) == E_FAILED, "3 * j overflows");
_setIJK(&ijk, INT32_MAX / 2, INT32_MAX / 3, 0);
t_assert(_upAp7rChecked(&ijk) == E_FAILED, "(i * 2) + j overflows");
// This input should be invalid because i < 0
_setIJK(&ijk, -2, INT32_MAX / 3, 0);
t_assert(_upAp7rChecked(&ijk) == E_FAILED, "(j * 3) - 1 overflows");
// This input should be invalid because j < 0
_setIJK(&ijk, -1, 0, 0);
t_assert(_upAp7rChecked(&ijk) == E_SUCCESS, "i < 0 succeeds");
}
}
109 changes: 109 additions & 0 deletions src/apps/testapps/testMathExtensions.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright 2022 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/** @file
* @brief tests functions and macros in mathExtensions.h
*
* usage: `testMathExtensions`
*/

#include "mathExtensions.h"
#include "test.h"
#include "utility.h"

#define ASSERT_SUB_OVERFLOW(a, b) \
t_assert(SUB_INT32S_OVERFLOWS((a), (b)), #a " - " #b " Overflows")
#define ASSERT_SUB_DOES_NOT_OVERFLOW(a, b) \
t_assert(!SUB_INT32S_OVERFLOWS((a), (b)), #a " - " #b \
" Does Not " \
"Overflow")
#define ASSERT_ADD_OVERFLOW(a, b) \
t_assert(ADD_INT32S_OVERFLOWS((a), (b)), #a " + " #b " Overflows")
#define ASSERT_ADD_DOES_NOT_OVERFLOW(a, b) \
t_assert(!ADD_INT32S_OVERFLOWS((a), (b)), #a " + " #b \
" Does Not " \
"Overflow")

SUITE(mathExtensions) {
TEST(_ipow) {
t_assert(_ipow(7, 0) == 1, "7 ** 0 == 1");
t_assert(_ipow(7, 1) == 7, "7 ** 1 == 7");
t_assert(_ipow(7, 2) == 49, "7 ** 2 == 49");
t_assert(_ipow(1, 20) == 1, "1 ** 20 == 1");
t_assert(_ipow(2, 5) == 32, "2 ** 5 == 32");
}

TEST(subOverflows) {
ASSERT_SUB_DOES_NOT_OVERFLOW(0, 0);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN, 0);
ASSERT_SUB_OVERFLOW(INT32_MIN, 1);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN, -1);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, 0);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, 1);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, -1);
ASSERT_SUB_OVERFLOW(INT32_MIN + 1, 2);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, -2);
ASSERT_SUB_DOES_NOT_OVERFLOW(100, 10);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX, 0);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX, 1);
ASSERT_SUB_OVERFLOW(INT32_MAX, -1);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX - 1, 1);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX - 1, -1);
ASSERT_SUB_OVERFLOW(INT32_MAX - 1, -2);
ASSERT_SUB_OVERFLOW(INT32_MAX - 1, -2);
ASSERT_SUB_OVERFLOW(INT32_MIN, INT32_MAX);
ASSERT_SUB_OVERFLOW(INT32_MAX, INT32_MIN);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN, INT32_MIN);
ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX, INT32_MAX);
ASSERT_SUB_DOES_NOT_OVERFLOW(-1, 0);
ASSERT_SUB_DOES_NOT_OVERFLOW(-1, 10);
ASSERT_SUB_DOES_NOT_OVERFLOW(-1, -10);
ASSERT_SUB_DOES_NOT_OVERFLOW(-1, INT32_MAX);
ASSERT_SUB_OVERFLOW(-2, INT32_MAX);
ASSERT_SUB_DOES_NOT_OVERFLOW(-1, INT32_MIN);
ASSERT_SUB_OVERFLOW(0, INT32_MIN);
}

TEST(addOverflows) {
ASSERT_ADD_DOES_NOT_OVERFLOW(0, 0);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN, 0);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN, 1);
ASSERT_ADD_OVERFLOW(INT32_MIN, -1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, 0);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, 1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, -1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, 2);
ASSERT_ADD_OVERFLOW(INT32_MIN + 1, -2);
ASSERT_ADD_DOES_NOT_OVERFLOW(100, 10);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX, 0);
ASSERT_ADD_OVERFLOW(INT32_MAX, 1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX, -1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX - 1, 1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX - 1, -1);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX - 1, -2);
ASSERT_ADD_OVERFLOW(INT32_MAX - 1, 2);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN, INT32_MAX);
ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX, INT32_MIN);
ASSERT_ADD_OVERFLOW(INT32_MAX, INT32_MAX);
ASSERT_ADD_OVERFLOW(INT32_MIN, INT32_MIN);
ASSERT_ADD_DOES_NOT_OVERFLOW(-1, 0);
ASSERT_ADD_DOES_NOT_OVERFLOW(-1, 10);
ASSERT_ADD_DOES_NOT_OVERFLOW(-1, -10);
ASSERT_ADD_DOES_NOT_OVERFLOW(-1, INT32_MAX);
ASSERT_ADD_DOES_NOT_OVERFLOW(-2, INT32_MAX);
ASSERT_ADD_OVERFLOW(-1, INT32_MIN);
ASSERT_ADD_DOES_NOT_OVERFLOW(0, INT32_MIN);
}
}
5 changes: 4 additions & 1 deletion src/h3lib/include/coordijk.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2018, 2020-2021 Uber Technologies, Inc.
* Copyright 2016-2018, 2020-2022 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -93,8 +93,11 @@ int _ijkMatches(const CoordIJK *c1, const CoordIJK *c2);
void _ijkAdd(const CoordIJK *h1, const CoordIJK *h2, CoordIJK *sum);
void _ijkSub(const CoordIJK *h1, const CoordIJK *h2, CoordIJK *diff);
void _ijkScale(CoordIJK *c, int factor);
bool _ijkNormalizeCouldOverflow(const CoordIJK *ijk);
void _ijkNormalize(CoordIJK *c);
Direction _unitIjkToDigit(const CoordIJK *ijk);
H3Error _upAp7Checked(CoordIJK *ijk);
H3Error _upAp7rChecked(CoordIJK *ijk);
void _upAp7(CoordIJK *ijk);
void _upAp7r(CoordIJK *ijk);
void _downAp7(CoordIJK *ijk);
Expand Down
10 changes: 9 additions & 1 deletion src/h3lib/include/mathExtensions.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017-2018 Uber Technologies, Inc.
* Copyright 2017-2018, 2022 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -27,6 +27,14 @@
*/
#define MAX(a, b) (((a) > (b)) ? (a) : (b))

/** Evaluates to true if a + b would overflow for int32 */
#define ADD_INT32S_OVERFLOWS(a, b) \
((a) > 0 ? (INT32_MAX - (a) < (b)) : (INT32_MIN - (a) > (b)))

/** Evaluates to true if a - b would overflow for int32 */
#define SUB_INT32S_OVERFLOWS(a, b) \
((a) >= 0 ? (INT32_MIN + (a) >= (b)) : (INT32_MAX + (a) + 1 < (b)))

// Internal functions
int64_t _ipow(int64_t base, int64_t exp);

Expand Down
Loading

0 comments on commit fc7f3dd

Please sign in to comment.