From f5ca5a07a687b56d83705405f934b186d98aec00 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 30 Jun 2016 03:41:28 -0400 Subject: [PATCH 1/3] buffer: fix unintended unsigned overflow `offset` is user supplied variable and may be bigger than `ts_obj_length`. There is no need to subtract them and pass along, so just throw when the subtraction result would overflow. --- src/node_buffer.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 9ae92d861edb9a..16d7c95ebe6b81 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -693,6 +693,9 @@ void StringWrite(const FunctionCallbackInfo& args) { size_t max_length; CHECK_NOT_OOB(ParseArrayIndex(args[1], 0, &offset)); + if (offset >= ts_obj_length) + return env->ThrowRangeError("Offset is out of bounds"); + CHECK_NOT_OOB(ParseArrayIndex(args[2], ts_obj_length - offset, &max_length)); max_length = MIN(ts_obj_length - offset, max_length); @@ -700,9 +703,6 @@ void StringWrite(const FunctionCallbackInfo& args) { if (max_length == 0) return args.GetReturnValue().Set(0); - if (offset >= ts_obj_length) - return env->ThrowRangeError("Offset is out of bounds"); - uint32_t written = StringBytes::Write(env->isolate(), ts_obj_data + offset, max_length, From 05d0321f32310f2f44d0f838a5d7b1c93ef4bf70 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 30 Jun 2016 03:42:54 -0400 Subject: [PATCH 2/3] crypto: fix undefined behavior in ParseExtension Many extensions are unknown to the `ClientHelloParser::ParseExtension`, do not cast user-supplied `uint16_t` to `enum`. --- src/node_crypto_clienthello.cc | 4 ++-- src/node_crypto_clienthello.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_crypto_clienthello.cc b/src/node_crypto_clienthello.cc index 8fbc3161f89969..8c862c1b6a5198 100644 --- a/src/node_crypto_clienthello.cc +++ b/src/node_crypto_clienthello.cc @@ -103,7 +103,7 @@ void ClientHelloParser::ParseHeader(const uint8_t* data, size_t avail) { } -void ClientHelloParser::ParseExtension(ClientHelloParser::ExtensionType type, +void ClientHelloParser::ParseExtension(const uint16_t type, const uint8_t* data, size_t len) { // NOTE: In case of anything we're just returning back, ignoring the problem. @@ -210,7 +210,7 @@ bool ClientHelloParser::ParseTLSClientHello(const uint8_t* data, size_t avail) { if (ext_off + ext_len > avail) return false; - ParseExtension(static_cast(ext_type), + ParseExtension(ext_type, data + ext_off, ext_len); diff --git a/src/node_crypto_clienthello.h b/src/node_crypto_clienthello.h index b0b63fb2e38138..3550807c20d05f 100644 --- a/src/node_crypto_clienthello.h +++ b/src/node_crypto_clienthello.h @@ -91,7 +91,7 @@ class ClientHelloParser { bool ParseRecordHeader(const uint8_t* data, size_t avail); void ParseHeader(const uint8_t* data, size_t avail); - void ParseExtension(ExtensionType type, + void ParseExtension(const uint16_t type, const uint8_t* data, size_t len); bool ParseTLSClientHello(const uint8_t* data, size_t avail); From 27bfdc5effe8cced8b151d8f05b5dd79b5a17759 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 30 Jun 2016 03:43:38 -0400 Subject: [PATCH 3/3] string_search: fix various overflows Before values are subtracted in C/C++ they are cast to a common type which depends on the types of lhs and rhs. Usually this means casting to a bigger type, and if the sizes are the same - casting to unsigned. --- src/string_search.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/string_search.h b/src/string_search.h index abc69edb87621d..b9c092742b370a 100644 --- a/src/string_search.h +++ b/src/string_search.h @@ -404,7 +404,7 @@ size_t StringSearch::BoyerMooreSearch( } else { int gs_shift = good_suffix_shift[j + 1]; int bc_occ = CharOccurrence(bad_char_occurence, c); - int shift = j - bc_occ; + int shift = static_cast(j) - bc_occ; if (gs_shift > shift) { shift = gs_shift; } @@ -494,12 +494,12 @@ size_t StringSearch::BoyerMooreHorspoolSearch( const size_t subject_length = subject.length(); const size_t pattern_length = pattern.length(); int* char_occurrences = search->bad_char_table(); - int64_t badness = -pattern_length; + int64_t badness = -static_cast(pattern_length); // How bad we are doing without a good-suffix table. Char last_char = pattern[pattern_length - 1]; int last_char_shift = - pattern_length - 1 - + static_cast(pattern_length) - 1 - CharOccurrence(char_occurrences, static_cast(last_char)); // Perform search @@ -509,7 +509,7 @@ size_t StringSearch::BoyerMooreHorspoolSearch( int subject_char; while (last_char != (subject_char = subject[index + j])) { int bc_occ = CharOccurrence(char_occurrences, subject_char); - int shift = j - bc_occ; + int shift = static_cast(j) - bc_occ; index += shift; badness += 1 - shift; // at most zero, so badness cannot increase. if (index > subject_length - pattern_length) { @@ -528,7 +528,7 @@ size_t StringSearch::BoyerMooreHorspoolSearch( // checked, and decreases by the number of characters we // can skip by shifting. It's a measure of how we are doing // compared to reading each character exactly once. - badness += (pattern_length - j) - last_char_shift; + badness += static_cast(pattern_length - j) - last_char_shift; if (badness > 0) { search->PopulateBoyerMooreTable(); search->strategy_ = &BoyerMooreSearch; @@ -602,7 +602,7 @@ size_t StringSearch::InitialSearch( if (j == pattern_length) { return i; } - badness += j; + badness += static_cast(j); } else { search->PopulateBoyerMooreHorspoolTable(); search->strategy_ = &BoyerMooreHorspoolSearch;