Skip to content

Commit

Permalink
ggwave : fix out-of-bounds access in ggwave_decode
Browse files Browse the repository at this point in the history
Also, provide a memory-safe overload called ggwave_ndecode()
The overload takes an extra parameter that specifies the size of
the output buffer and thus limits the size of the Rx payload that can be
decoded and stored.
  • Loading branch information
ggerganov committed Sep 19, 2021
1 parent 1a0af88 commit f7c38d8
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 8 deletions.
26 changes: 24 additions & 2 deletions include/ggwave/ggwave.h
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,12 @@ extern "C" {
// If the return value is -1 then there was an error during the decoding process.
// Usually can occur if there is a lot of background noise in the audio.
//
// If the return value is greater than 0, then there will be that number of bytes
// decoded in the outputBuffer
// If the return value is greater than 0, then there are that number of bytes decoded.
//
// IMPORTANT:
// The actual bytes written to outputBuffer are 1 more than the return value since this
// function writes one extra null terminating byte at the end of the payload. This is for
// convenience, since the most common type of payload is text messages.
//
// Example:
//
Expand Down Expand Up @@ -218,6 +222,22 @@ extern "C" {
int dataSize,
char * outputBuffer);

// Memory-safe overload of ggwave_decode
//
// outputSize - optionally specify the size of the output buffer
//
// If the return value is -2 then the provided outputBuffer was not big enough to
// store the decoded data.
//
// See ggwave_decode for more information
//
GGWAVE_API int ggwave_ndecode(
ggwave_Instance instance,
const char * dataBuffer,
int dataSize,
char * outputBuffer,
int outputSize);

#ifdef __cplusplus
}

Expand Down Expand Up @@ -388,6 +408,8 @@ class GGWave {
void setRxProtocols(const RxProtocols & rxProtocols) { m_rxProtocols = rxProtocols; }
const RxProtocols & getRxProtocols() const { return m_rxProtocols; }

int lastRxDataLength() const { return m_lastRxDataLength; }

const TxRxData & getRxData() const { return m_rxData; }
const RxProtocol & getRxProtocol() const { return m_rxProtocol; }
const RxProtocolId & getRxProtocolId() const { return m_rxProtocolId; }
Expand Down
43 changes: 41 additions & 2 deletions src/ggwave.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,54 @@ int ggwave_decode(

ggWave->decode(cbWaveformInp);

// todo : avoid allocation
// TODO : avoid allocation
GGWave::TxRxData rxData;

auto rxDataLength = ggWave->takeRxData(rxData);
if (rxDataLength == -1) {
// failed to decode message
return -1;
} else if (rxDataLength > 0) {
std::copy(rxData.begin(), rxData.end(), outputBuffer);
memcpy(outputBuffer, rxData.data(), rxDataLength + 1);
}

return rxDataLength;
}

extern "C"
int ggwave_ndecode(
ggwave_Instance instance,
const char * dataBuffer,
int dataSize,
char * outputBuffer,
int outputSize) {
// TODO : avoid duplicated code
GGWave * ggWave = (GGWave *) g_instances[instance];

GGWave::CBWaveformInp cbWaveformInp = [&](void * data, uint32_t nMaxBytes) -> uint32_t {
uint32_t nCopied = std::min((uint32_t) dataSize, nMaxBytes);
std::copy(dataBuffer, dataBuffer + nCopied, (char *) data);

dataSize -= nCopied;
dataBuffer += nCopied;

return nCopied;
};

ggWave->decode(cbWaveformInp);

// TODO : avoid allocation
GGWave::TxRxData rxData;

auto rxDataLength = ggWave->takeRxData(rxData);
if (rxDataLength == -1) {
// failed to decode message
return -1;
} else if (rxDataLength + 1 > outputSize) {
// the outputBuffer is not big enough to store the data
return -2;
} else if (rxDataLength > 0) {
memcpy(outputBuffer, rxData.data(), rxDataLength + 1);
}

return rxDataLength;
Expand Down
16 changes: 12 additions & 4 deletions tests/test-ggwave.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,23 @@ int main() {

int ret;
const char * payload = "test";
char decoded[256];
char decoded[16];

int n = ggwave_encode(instance, payload, 4, GGWAVE_TX_PROTOCOL_AUDIBLE_FAST, 50, NULL, 1);
char waveform[n];

ret = ggwave_encode(instance, payload, 4, GGWAVE_TX_PROTOCOL_AUDIBLE_FAST, 50, waveform, 0);
CHECK(ret > 0);
int ne = ggwave_encode(instance, payload, 4, GGWAVE_TX_PROTOCOL_AUDIBLE_FAST, 50, waveform, 0);
CHECK(ne > 0);

ret = ggwave_decode(instance, waveform, sizeof(signed short)*ret, decoded);
// note that the output buffer has to be 1 extra byte longer than the expected payload
ret = ggwave_ndecode(instance, waveform, sizeof(signed short)*ne, decoded, 4);
CHECK(ret == -2); // fail

ret = ggwave_ndecode(instance, waveform, sizeof(signed short)*ne, decoded, 5);
CHECK(ret == 4); // success

// unsafe method - will write the decoded output to the output buffer regardless of the size
ret = ggwave_decode(instance, waveform, sizeof(signed short)*ne, decoded);
CHECK(ret == 4);

CHECK(strcmp(decoded, payload) == 0);
Expand Down

0 comments on commit f7c38d8

Please sign in to comment.