search

trailofbits / cb-multios

DARPA Challenges Sets for Linux, Windows, and macOS
https://blog.trailofbits.com/2016/08/01/your-tool-works-better-than-mine-prove-it/
MIT License
520 stars 103 forks source link

Invert test to avoid undefined behavior #58

Closed maroneze closed 5 years ago

maroneze commented 5 years ago

Otherwise, when i == MAX_PLAYERS, we end up accessing an element beyond the end of the array. Found with Frama-C.

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

ekilmer commented 5 years ago

Thank you for your contribution @maroneze! Apologies that it took so long for a response. We will try to be better about addressing any future contributions related to your issue here #57 . Keep it up!

I've gone ahead and added an #ifdef PATCHED_2 around your fix, in a similar manner as that found in https://github.com/trailofbits/cb-multios/blob/dbcd16decde12b8e7d0a21d4c525cfaf4f6a36e8/challenges/WhackJack/src/service.c#L62-L72

We prefer that there be some #ifdef PATCHED_$num around any future unmarked patches and ideally a POV that exercises the vulnerability. In the CGC, a team could still score points for finding a vulnerability that was unmarked, which is why adding the #ifdef is preferred.

For this vulnerability, a POV feels unnecessary/impossible, so I think this should be ready to merge after CI passes.

I've also added your description of the bug to the README.