From 466fb0ff6b4ff1811168a393d079467eeb6c5d73 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 4 May 2014 22:37:21 +0200 Subject: [PATCH 1/4] Add more buffer overflow checks. There are a few places left, where the contents of the buffer is accessed without first inspecting the available length. --- src/hw_ostc_parser.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/hw_ostc_parser.c b/src/hw_ostc_parser.c index b007377..5606631 100644 --- a/src/hw_ostc_parser.c +++ b/src/hw_ostc_parser.c @@ -563,6 +563,9 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // Extended sample info. for (unsigned int i = 0; i < nconfig; ++i) { if (info[i].divisor && (nsamples % info[i].divisor) == 0) { + if (offset + info[i].size > size) + return DC_STATUS_DATAFORMAT; + unsigned int value = 0; switch (info[i].type) { case 0: // Temperature (0.1 °C). @@ -620,7 +623,7 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call } } - if (data[offset] != 0xFD || data[offset + 1] != 0xFD) + if (offset + 2 > size || data[offset] != 0xFD || data[offset + 1] != 0xFD) return DC_STATUS_DATAFORMAT; return DC_STATUS_SUCCESS; From 5abad6e56838535db24dc54211202a825f8b16b7 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 4 May 2014 22:40:14 +0200 Subject: [PATCH 2/4] Add more error messages. This makes debugging easier, because the error messages immediately reveal where the problem is located, without needing a debugger. --- src/hw_ostc_parser.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/hw_ostc_parser.c b/src/hw_ostc_parser.c index 5606631..c090faa 100644 --- a/src/hw_ostc_parser.c +++ b/src/hw_ostc_parser.c @@ -475,8 +475,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call offset += 1; // Check for buffer overflows. - if (offset + length > size) + if (offset + length > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } // Get the event byte(s). unsigned int nbits = 0; @@ -484,8 +486,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call while (data[offset - 1] & 0x80) { if (nbits && version != 0x23) break; - if (offset + 1 > size) + if (offset + 1 > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } events |= data[offset] << nbits; nbits += 8; offset++; @@ -525,8 +529,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // Manual Gas Set & Change if (events & 0x10) { - if (offset + 2 > size) + if (offset + 2 > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } sample.event.type = SAMPLE_EVENT_GASCHANGE2; sample.event.time = 0; sample.event.flags = 0; @@ -537,8 +543,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // Gas Change if (events & 0x20) { - if (offset + 1 > size) + if (offset + 1 > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } unsigned int idx = data[offset]; if (idx < 1 || idx > ngasmix) return DC_STATUS_DATAFORMAT; @@ -553,8 +561,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // SetPoint Change if ((events & 0x40) && (version == 0x23)) { - if (offset + 1 > size) + if (offset + 1 > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } sample.setpoint = data[offset] / 100.0; if (callback) callback (DC_SAMPLE_SETPOINT, sample, userdata); offset++; @@ -563,8 +573,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // Extended sample info. for (unsigned int i = 0; i < nconfig; ++i) { if (info[i].divisor && (nsamples % info[i].divisor) == 0) { - if (offset + info[i].size > size) + if (offset + info[i].size > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } unsigned int value = 0; switch (info[i].type) { @@ -602,8 +614,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call if (version != 0x23) { // SetPoint Change if (events & 0x40) { - if (offset + 1 > size) + if (offset + 1 > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } sample.setpoint = data[offset] / 100.0; if (callback) callback (DC_SAMPLE_SETPOINT, sample, userdata); offset++; @@ -611,8 +625,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // Bailout Event if (events & 0x80) { - if (offset + 2 > size) + if (offset + 2 > size) { + ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; + } sample.event.type = SAMPLE_EVENT_GASCHANGE2; sample.event.time = 0; sample.event.flags = 0; @@ -623,8 +639,10 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call } } - if (offset + 2 > size || data[offset] != 0xFD || data[offset + 1] != 0xFD) + if (offset + 2 > size || data[offset] != 0xFD || data[offset + 1] != 0xFD) { + ERROR (abstract->context, "Invalid end marker found!"); return DC_STATUS_DATAFORMAT; + } return DC_STATUS_SUCCESS; } From 4db8535e3950ad5d762876ac1f4a02b9e96a026d Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 4 May 2014 22:43:54 +0200 Subject: [PATCH 3/4] Use the extended sample length stored in the data. Currently, the buffer overflow checks take into account the size of the entire dive. But since the length of the extended sample is stored in the data, we can actually check for overflows in each sample. The main benefit is that errors will be caught much earlier now. An additional advantage is that we can now easily skip any remaining sample bytes. Normally such bytes are not present, unless a firmware update introduces a new feature which our parser doesn't support yet. --- src/hw_ostc_parser.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/hw_ostc_parser.c b/src/hw_ostc_parser.c index c090faa..26fea22 100644 --- a/src/hw_ostc_parser.c +++ b/src/hw_ostc_parser.c @@ -486,13 +486,14 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call while (data[offset - 1] & 0x80) { if (nbits && version != 0x23) break; - if (offset + 1 > size) { + if (length < 1) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } events |= data[offset] << nbits; nbits += 8; offset++; + length--; } // Alarms @@ -529,7 +530,7 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call // Manual Gas Set & Change if (events & 0x10) { - if (offset + 2 > size) { + if (length < 2) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } @@ -539,11 +540,12 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call sample.event.value = data[offset] | (data[offset + 1] << 16); if (callback) callback (DC_SAMPLE_EVENT, sample, userdata); offset += 2; + length -= 2; } // Gas Change if (events & 0x20) { - if (offset + 1 > size) { + if (length < 1) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } @@ -557,23 +559,25 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call sample.event.value = gasmix[idx].oxygen | (gasmix[idx].helium << 16); if (callback) callback (DC_SAMPLE_EVENT, sample, userdata); offset++; + length--; } // SetPoint Change if ((events & 0x40) && (version == 0x23)) { - if (offset + 1 > size) { + if (length < 1) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } sample.setpoint = data[offset] / 100.0; if (callback) callback (DC_SAMPLE_SETPOINT, sample, userdata); offset++; + length--; } // Extended sample info. for (unsigned int i = 0; i < nconfig; ++i) { if (info[i].divisor && (nsamples % info[i].divisor) == 0) { - if (offset + info[i].size > size) { + if (length < info[i].size) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } @@ -608,24 +612,26 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call } offset += info[i].size; + length -= info[i].size; } } if (version != 0x23) { // SetPoint Change if (events & 0x40) { - if (offset + 1 > size) { + if (length < 1) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } sample.setpoint = data[offset] / 100.0; if (callback) callback (DC_SAMPLE_SETPOINT, sample, userdata); offset++; + length--; } // Bailout Event if (events & 0x80) { - if (offset + 2 > size) { + if (length < 2) { ERROR (abstract->context, "Buffer overflow detected!"); return DC_STATUS_DATAFORMAT; } @@ -635,8 +641,15 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call sample.event.value = data[offset] | (data[offset + 1] << 16); if (callback) callback (DC_SAMPLE_EVENT, sample, userdata); offset += 2; + length -= 2; } } + + // Skip remaining sample bytes (if any). + if (length) { + WARNING (abstract->context, "Remaining %u bytes skipped.", length); + } + offset += length; } if (offset + 2 > size || data[offset] != 0xFD || data[offset + 1] != 0xFD) { From b7a5394e73804019f4d89415302c76f0fc6231be Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 4 May 2014 22:50:31 +0200 Subject: [PATCH 4/4] Add support for the new OSTC3 bailout event. With the exception of the different event mask (single byte 0x80 vs two byte 0x0100), the OSTC3 bailout event is identical to the OSTC2 variant. Just as before, the new bailout event is reported to the application as a normal gas change event. --- src/hw_ostc_parser.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/hw_ostc_parser.c b/src/hw_ostc_parser.c index 26fea22..8fb8875 100644 --- a/src/hw_ostc_parser.c +++ b/src/hw_ostc_parser.c @@ -562,16 +562,33 @@ hw_ostc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t call length--; } - // SetPoint Change - if ((events & 0x40) && (version == 0x23)) { - if (length < 1) { - ERROR (abstract->context, "Buffer overflow detected!"); - return DC_STATUS_DATAFORMAT; + if (version == 0x23) { + // SetPoint Change + if (events & 0x40) { + if (length < 1) { + ERROR (abstract->context, "Buffer overflow detected!"); + return DC_STATUS_DATAFORMAT; + } + sample.setpoint = data[offset] / 100.0; + if (callback) callback (DC_SAMPLE_SETPOINT, sample, userdata); + offset++; + length--; + } + + // Bailout Event + if (events & 0x0100) { + if (length < 2) { + ERROR (abstract->context, "Buffer overflow detected!"); + return DC_STATUS_DATAFORMAT; + } + sample.event.type = SAMPLE_EVENT_GASCHANGE2; + sample.event.time = 0; + sample.event.flags = 0; + sample.event.value = data[offset] | (data[offset + 1] << 16); + if (callback) callback (DC_SAMPLE_EVENT, sample, userdata); + offset += 2; + length -= 2; } - sample.setpoint = data[offset] / 100.0; - if (callback) callback (DC_SAMPLE_SETPOINT, sample, userdata); - offset++; - length--; } // Extended sample info.