Fix various overflows and UB in src/ #7494

indutny · 2016-06-30T07:46:53Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src

Description of change

Fix various overflows and UB in src/

R= @bnoordhuis and/or @nodejs/collaborators

NOTE: This doesn't fix:

../src/string_search.h:231:68: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
SUMMARY: AddressSanitizer: undefined-behavior ../src/string_search.h:231:68 in
../src/string_search.h:186:34: runtime error: index 18446744073709451866 out of bounds for type 'int [251]'
SUMMARY: AddressSanitizer: undefined-behavior ../src/string_search.h:186:34 in
../src/string_search.h:194:25: runtime error: index 18446744073709451866 out of bounds for type 'int [251]'
SUMMARY: AddressSanitizer: undefined-behavior ../src/string_search.h:194:25 in

I'm open to suggestions how it could be approached, though.

`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.

Many extensions are unknown to the `ClientHelloParser::ParseExtension`, do not cast user-supplied `uint16_t` to `enum`.

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.

indutny · 2016-06-30T07:47:11Z

Discovered with:

-fsanitize=undefined,address,signed-integer-overflow,unsigned-integer-overflow,integer

indutny · 2016-06-30T07:47:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3138/

addaleax · 2016-06-30T08:01:49Z

For the ../src/string_search.h:231:68: one, I think using i < haystack_len is okay?

bnoordhuis · 2016-06-30T09:12:31Z

First two commits LGTM but as to the last one, it would be better to rewrite it to use unsigned types with proper bound checks, rather than haphazardly peppering it with static_casts.

indutny · 2016-06-30T16:15:00Z

@bnoordhuis it is using lots of negative integers, and I'm not too confident in the code at the moment to make serious refactoring. Adding casts, on other hand, makes things closer to what developer has intended.

indutny · 2016-06-30T16:15:29Z

@addaleax will it be? I think there is some algorithmic trickery there.

jasnell · 2016-06-30T17:16:49Z

I agree with @bnoordhuis ... the first two are fine, the third is questionable. Perhaps pull that one out into a separate PR where it can be iterated on?

indutny · 2016-06-30T17:18:49Z

Will surely do after I'll land the first two. Thanks!

addaleax · 2016-06-30T18:38:03Z

@indutny I’d actually say it’s a common pattern for descending for loops with unsigned types, even though I know it’s not pretty.

indutny · 2016-06-30T18:41:37Z

@addaleax oh yeah, I included that warning just for completeness. Sorry, forgot to comment!

trevnorris · 2016-06-30T19:24:09Z

src/node_buffer.cc

  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);

-  if (offset >= ts_obj_length)
-    return env->ThrowRangeError("Offset is out of bounds");
-


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

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?

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

@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.

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

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

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

@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?

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

indutny · 2016-07-11T06:24:10Z

Landing first two commits, since there were no objection on them.

indutny · 2016-07-11T06:44:47Z

Landed in 46f40cf and c9f6776.

indutny · 2016-07-11T06:44:51Z

Thank you everyone!

`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. PR-URL: #7494 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Many extensions are unknown to the `ClientHelloParser::ParseExtension`, do not cast user-supplied `uint16_t` to `enum`. PR-URL: #7494 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`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. PR-URL: #7494 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Many extensions are unknown to the `ClientHelloParser::ParseExtension`, do not cast user-supplied `uint16_t` to `enum`. PR-URL: #7494 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax · 2016-09-30T20:31:31Z

I can’t quite see how either commit that landed qualifies as semver-minor… these seem like they could (should?) be backported to v4.x together with #8154?

indutny · 2016-09-30T20:43:46Z

@addaleax the throw was moved and happens earlier with this PR, not sure if it is acceptable as a patch version change.

addaleax · 2016-09-30T20:51:50Z

I don’t think that difference can actually be triggered without touching the binding directly… but we don’t need to make a big fuzz about this, I’m perfectly fine with this not landing in v4.

MylesBorins · 2016-09-30T21:09:27Z

@addaleax it can still land as a minor

addaleax · 2016-09-30T21:24:14Z

Idk, maybe some people use things like utf8Write directly. I doubt it’s common, and it’s undocumented, but @indutny has a point here. There’s probably no compiler which doesn’t handle the code here well, so let’s just leave it off v4.x unless I happened to over-convince somebody else that it should land there.

indutny added 3 commits June 30, 2016 03:41

buffer: fix unintended unsigned overflow

f5ca5a0

`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.

crypto: fix undefined behavior in ParseExtension

05d0321

Many extensions are unknown to the `ClientHelloParser::ParseExtension`, do not cast user-supplied `uint16_t` to `enum`.

string_search: fix various overflows

27bfdc5

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.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 30, 2016

mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 30, 2016

trevnorris reviewed Jun 30, 2016
View reviewed changes

mscdex added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 1, 2016

indutny closed this Jul 11, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

addaleax mentioned this pull request Sep 30, 2016

buffer: allow .write() offset to be at buffer end #8154

Closed

3 tasks

addaleax added the lts-watch-v4.x label Sep 30, 2016

MylesBorins added the lts-agenda label Sep 30, 2016

MylesBorins mentioned this pull request Sep 30, 2016

LTS WG Meeting - 2016-10-03 8:00pm UTC nodejs/Release#139

Closed

MylesBorins added dont-land-on-v4.x and removed lts-agenda labels Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various overflows and UB in src/ #7494

Fix various overflows and UB in src/ #7494

indutny commented Jun 30, 2016 •

edited

Loading

indutny commented Jun 30, 2016

indutny commented Jun 30, 2016

addaleax commented Jun 30, 2016 •

edited

Loading

bnoordhuis commented Jun 30, 2016

indutny commented Jun 30, 2016

indutny commented Jun 30, 2016

jasnell commented Jun 30, 2016

indutny commented Jun 30, 2016

addaleax commented Jun 30, 2016 •

edited

Loading

indutny commented Jun 30, 2016

trevnorris Jun 30, 2016 •

edited

Loading

indutny Jun 30, 2016

trevnorris Jun 30, 2016

RReverser Aug 23, 2016

indutny Aug 23, 2016

addaleax Aug 24, 2016

RReverser Aug 24, 2016

addaleax Aug 24, 2016

RReverser Aug 24, 2016

indutny commented Jul 11, 2016

indutny commented Jul 11, 2016

indutny commented Jul 11, 2016

addaleax commented Sep 30, 2016

indutny commented Sep 30, 2016

addaleax commented Sep 30, 2016

MylesBorins commented Sep 30, 2016

addaleax commented Sep 30, 2016

Fix various overflows and UB in src/ #7494

Fix various overflows and UB in src/ #7494

Conversation

indutny commented Jun 30, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

indutny commented Jun 30, 2016

indutny commented Jun 30, 2016

addaleax commented Jun 30, 2016 • edited Loading

bnoordhuis commented Jun 30, 2016

indutny commented Jun 30, 2016

indutny commented Jun 30, 2016

jasnell commented Jun 30, 2016

indutny commented Jun 30, 2016

addaleax commented Jun 30, 2016 • edited Loading

indutny commented Jun 30, 2016

trevnorris Jun 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Jul 11, 2016

indutny commented Jul 11, 2016

indutny commented Jul 11, 2016

addaleax commented Sep 30, 2016

indutny commented Sep 30, 2016

addaleax commented Sep 30, 2016

MylesBorins commented Sep 30, 2016

addaleax commented Sep 30, 2016

indutny commented Jun 30, 2016 •

edited

Loading

addaleax commented Jun 30, 2016 •

edited

Loading

addaleax commented Jun 30, 2016 •

edited

Loading

trevnorris Jun 30, 2016 •

edited

Loading