Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various overflows and UB in src/ #7494

Closed
wants to merge 3 commits into from
Closed

Fix various overflows and UB in src/ #7494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5ca5a0
buffer: fix unintended unsigned overflow
indutny Jun 30, 2016
05d0321
crypto: fix undefined behavior in ParseExtension
indutny Jun 30, 2016
27bfdc5
string_search: fix various overflows
indutny Jun 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,16 +693,16 @@ void StringWrite(const FunctionCallbackInfo<Value>& 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);

if (max_length == 0)
return args.GetReturnValue().Set(0);

Copy link
Contributor

@trevnorris trevnorris Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, are we missing test coverage that would have caught this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one edge case with max_length equal to 0, but otherwise nothing bad happens here. Btw, should we consider this as a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO you're simply making this defined behavior, which falls under a bug fix and a semver-minor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris @indutny Tracked issue I got after upgrade to this point. Apparently this broke some of my code e.g. using protocol-buffers (and I'm sure some other binary encoders too) which, for certain data, are writing empty strings in the end of the buffer, which is safe operation (no-op) on its own and worked up until this PR without any extra checks.

I was going to prepare PR to fix this but sounds like it falls under

There is one edge case with max_length equal to 0

and I'm wondering whether we consider this an expected behavior?

Minimal sample: Buffer.alloc(0).write('') - works in pre-6.4.0, throws now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, sorry to break your code!

I don't have any strong opinion on how it should work, but to me it will be fine to not throw in this case.

cc @trevnorris

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RReverser @indutny See #8127/#8154, it should already been fixed on master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax By master you mean v6.x? (because that's where issue is present, at least it was when I checked yesterday)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RReverser I’m a bit confused – the commit from #8154 is only in master, not yet on the v6.x branch, yes. I’d anticipate that it lands cleanly, so there should be no need for manual backporting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax Ah I thought you meant it should already work on v6.x branch (as in already landed). Sorry for misunderstanding.

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,
Expand Down
4 changes: 2 additions & 2 deletions src/node_crypto_clienthello.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<ExtensionType>(ext_type),
ParseExtension(ext_type,
data + ext_off,
ext_len);

Expand Down
2 changes: 1 addition & 1 deletion src/node_crypto_clienthello.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions src/string_search.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ size_t StringSearch<Char>::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<int>(j) - bc_occ;
if (gs_shift > shift) {
shift = gs_shift;
}
Expand Down Expand Up @@ -494,12 +494,12 @@ size_t StringSearch<Char>::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<int64_t>(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<int>(pattern_length) - 1 -
CharOccurrence(char_occurrences, static_cast<Char>(last_char));

// Perform search
Expand All @@ -509,7 +509,7 @@ size_t StringSearch<Char>::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<int>(j) - bc_occ;
index += shift;
badness += 1 - shift; // at most zero, so badness cannot increase.
if (index > subject_length - pattern_length) {
Expand All @@ -528,7 +528,7 @@ size_t StringSearch<Char>::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<int64_t>(pattern_length - j) - last_char_shift;
if (badness > 0) {
search->PopulateBoyerMooreTable();
search->strategy_ = &BoyerMooreSearch;
Expand Down Expand Up @@ -602,7 +602,7 @@ size_t StringSearch<Char>::InitialSearch(
if (j == pattern_length) {
return i;
}
badness += j;
badness += static_cast<int64_t>(j);
} else {
search->PopulateBoyerMooreHorspoolTable();
search->strategy_ = &BoyerMooreHorspoolSearch;
Expand Down