Skip to content

Commit

Permalink
buffer: avoid overrun on UCS-2 string write
Browse files Browse the repository at this point in the history
CVE-2018-12115
Discovered by ChALkeR - Сковорода Никита Андреевич
Fix by Anna Henningsen

Writing to the second-to-last byte with UCS-2 encoding will cause a -1
length to be send to String::Write(), writing all of the provided Buffer
from that point and beyond.

Fixes: nodejs-private/security#203
PR-URL: nodejs-private/node-private#138
  • Loading branch information
rvagg committed Aug 15, 2018
1 parent 08a150f commit 0052926
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/string_bytes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,11 @@ size_t StringBytes::WriteUCS2(char* buf,
size_t* chars_written) {
uint16_t* const dst = reinterpret_cast<uint16_t*>(buf);

size_t max_chars = (buflen / sizeof(*dst));
size_t max_chars = buflen / sizeof(*dst);
if (max_chars == 0) {
return 0;
}

size_t nchars;
size_t alignment = reinterpret_cast<uintptr_t>(dst) % sizeof(*dst);
if (alignment == 0) {
Expand Down
21 changes: 21 additions & 0 deletions test/parallel/test-buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1506,3 +1506,24 @@ assert.strictEqual(SlowBuffer.prototype.offset, undefined);
// Check pool offset after that by trying to write string into the pool.
assert.doesNotThrow(() => Buffer.from('abc'));
}

// UCS-2 overflow CVE-2018-12115
for (let i = 1; i < 4; i++) {
// Allocate two Buffers sequentially off the pool. Run more than once in case
// we hit the end of the pool and don't get sequential allocations
const x = Buffer.allocUnsafe(4).fill(0);
const y = Buffer.allocUnsafe(4).fill(1);
// Should not write anything, pos 3 doesn't have enough room for a 16-bit char
assert.strictEqual(x.write('ыыыыыы', 3, 'ucs2'), 0);
// CVE-2018-12115 experienced via buffer overrun to next block in the pool
assert.strictEqual(Buffer.compare(y, Buffer.alloc(4, 1)), 0);
}

// Should not write any data when there is no space for 16-bit chars
const z = Buffer.alloc(4, 0);
assert.strictEqual(z.write('\u0001', 3, 'ucs2'), 0);
assert.strictEqual(Buffer.compare(z, Buffer.alloc(4, 0)), 0);

// Large overrun could corrupt the process
assert.strictEqual(Buffer.alloc(4)
.write('ыыыыыы'.repeat(100), 3, 'utf16le'), 0);

0 comments on commit 0052926

Please sign in to comment.