Fix potential memory corruption bug

UDPC_atostr(...) uses a uint32_t as an offset into a buffer inside the
UDPC Context such that there can be at most 32 different addr-strings.
The call is thread-safe, up to a point (at most 32 concurrent calls will
return correct strings). The problem was that the offset was not being
reset to 0 and was always being incremented by 40. Should the offset
overflow, the offset into the buffer will be "mis-aligned" such that the
32nd offset into the buffer can possibly overwrite memory past the
buffer's end if the addr string is long enough.

The fix is to use a mutex instead of an atomic uint32_t to lock a
code-block that will always prevent overflow (using modulus operator).

I think the speed-loss is negligable (using a lock instead of an atomic
uint32_t) since it isn't expected for programs using UDPC to use
UDPC_atostr(...) very frequently.
This commit is contained in:
Stephen Seo 2023-06-21 13:23:26 +09:00
parent 211715fc56
commit 2495396d76
2 changed files with 12 additions and 5 deletions

View file

@ -230,7 +230,6 @@ public:
std::atomic_uint_fast8_t loggingType;
// See UDPC_AuthPolicy enum in UDPC.h for possible values
std::atomic_uint_fast8_t authPolicy;
std::atomic_uint32_t atostrBufIndex;
char atostrBuf[UDPC_ATOSTR_SIZE];
UDPC_SOCKETTYPE socketHandle;
@ -267,6 +266,9 @@ public:
unsigned char pk[crypto_sign_PUBLICKEYBYTES];
std::atomic_bool keysSet;
std::mutex atostrBufIndexMutex;
std::uint32_t atostrBufIndex;
}; // struct Context
Context *verifyContext(UDPC_HContext ctx);

View file

@ -223,12 +223,12 @@ loggingType(UDPC_DEBUG),
#else
loggingType(UDPC_WARNING),
#endif
atostrBufIndex(0),
receivedPkts(),
cSendPkts(),
rng_engine(),
conMapMutex(),
peerPKWhitelistMutex()
peerPKWhitelistMutex(),
atostrBufIndex(0)
{
std::memset(atostrBuf, 0, UDPC_ATOSTR_SIZE);
@ -2633,8 +2633,13 @@ const char *UDPC_atostr(UDPC_HContext ctx, UDPC_IPV6_ADDR_TYPE addr) {
if(!c) {
return nullptr;
}
const uint32_t headIndex =
c->atostrBufIndex.fetch_add(UDPC_ATOSTR_BUFSIZE) % UDPC_ATOSTR_SIZE;
uint32_t headIndex;
{
// Use a lock to ensure that "atostrBufIndex" is always a valid value.
std::lock_guard<std::mutex> indexLock(c->atostrBufIndexMutex);
c->atostrBufIndex = (c->atostrBufIndex + UDPC_ATOSTR_BUFSIZE) % UDPC_ATOSTR_SIZE;
headIndex = c->atostrBufIndex;
}
uint32_t index = headIndex;
bool usedDouble = false;