From 775a82c305aec32597bc24baabed6ac24df739e9 Mon Sep 17 00:00:00 2001 From: Andre Date: Wed, 16 Oct 2019 21:14:49 +0200 Subject: [PATCH] Invert test to avoid undefined behavior (#58) * Invert test to avoid undefined behavior Otherwise, when i == MAX_PLAYERS, we end up accessing an element beyond the end of the array. Found with Frama-C. * Add patch for bug * Add detail about bug --- challenges/WhackJack/CMakeLists.txt | 2 +- challenges/WhackJack/README.md | 5 +++++ challenges/WhackJack/src/player.c | 8 ++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/challenges/WhackJack/CMakeLists.txt b/challenges/WhackJack/CMakeLists.txt index 334fa5a49..be1f61c8d 100644 --- a/challenges/WhackJack/CMakeLists.txt +++ b/challenges/WhackJack/CMakeLists.txt @@ -1,5 +1,5 @@ set( SERVICE_ID "00065" ) set( AUTHOR_ID "CROMU" ) add_compile_options( -O0 -g -DENABLE_BUFFERED_PRINTF ) -set( VULN_COUNT "1" ) +set( VULN_COUNT "2" ) buildCB() diff --git a/challenges/WhackJack/README.md b/challenges/WhackJack/README.md index b33c6455f..e38eae09f 100755 --- a/challenges/WhackJack/README.md +++ b/challenges/WhackJack/README.md @@ -36,3 +36,8 @@ This vulnerability is easy to find and very easy to fix. Exploitation is hard g Proving Vulnerability 1: hard Fixing Vulnerability 1: easy +### Unmarked Bugs/Vulnerabilities + +Vulnerability 2 was unmarked in the original challenge, and it is more of a bug/undefined behavior than something that can be exploited. The fix is to reorder the tests to avoid undefined behavior. Otherwise, when `i == MAX_PLAYERS`, we end up accessing an element beyond the end of the array. Found with Frama-C. + +No PoV is provided. diff --git a/challenges/WhackJack/src/player.c b/challenges/WhackJack/src/player.c index 3945f1573..9e8824181 100644 --- a/challenges/WhackJack/src/player.c +++ b/challenges/WhackJack/src/player.c @@ -39,7 +39,15 @@ int method; i = 0; +#ifdef PATCHED_2 + + while (i < MAX_PLAYERS && playerList[i].player_name[0] != 0) + +#else + while (playerList[i].player_name[0] != 0 && i < MAX_PLAYERS) + +#endif ++i; if (i == MAX_PLAYERS) {