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.

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

New Uint8Arrays are sometimes filled with garbage #2930

Closed
dchest opened this issue Sep 17, 2015 · 7 comments
Closed

New Uint8Arrays are sometimes filled with garbage #2930

dchest opened this issue Sep 17, 2015 · 7 comments
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@dchest
Copy link

dchest commented Sep 17, 2015

Unfortunately, I don't have a neat small test case, so please bear with me.

One TweetNaCl-js test with Node.js 4.1 fails, while succeeding with 0.12 and io.js v3.3.1. The test is encrypting a message and comparing the result with a known good value.

To reproduce:

mkdir reproduce
cd reproduce
git clone https://github.com/dchest/tweetnacl-js.git
cd tweetnacl-js
npm install tape
NACL_SRC=nacl.js node ./test/04-secretbox.js | more

Output:

TAP version 13
# nacl.secretbox random test vectors
ok 1 box should be created
ok 2 should be equal
ok 3 box should open
ok 4 should be equal
ok 5 box should be created
not ok 6 should be equal
  ---
    operator: equal
    expected: 'VJViVugBhKeaOvfSlHkTzUQ='
    actual:   'ZpRiVugBhKeaOvfSlHkTzUQ='
    at: Test.<anonymous> (/Users/dchest/reproduce/tweetnacl-js/test/04-secretbox.js:10:17)
  ...

(actual may be different for you)

The cause is in this function in nacl.js:

https://github.com/dchest/tweetnacl-js/blob/master/nacl.js#L990

nacl.secretbox = function(msg, nonce, key) {
  checkArrayTypes(msg, nonce, key);
  checkLengths(key, nonce);
  var m = new Uint8Array(crypto_secretbox_ZEROBYTES + msg.length); // <-- BUG HERE
  var c = new Uint8Array(m.length);
  for (var i = 0; i < msg.length; i++) m[i+crypto_secretbox_ZEROBYTES] = msg[i];
  crypto_secretbox(c, m, m.length, nonce, key);
  return c.subarray(crypto_secretbox_BOXZEROBYTES);
};

This function requires that m must be zero-filled.

If I add console.log after creating m to see what's there:

nacl.secretbox = function(msg, nonce, key) {
  checkArrayTypes(msg, nonce, key);
  checkLengths(key, nonce);
  var m = new Uint8Array(crypto_secretbox_ZEROBYTES + msg.length); // <-- BUG HERE
  console.log(m); // <--- ADDED
  var c = new Uint8Array(m.length);
  for (var i = 0; i < msg.length; i++) m[i+crypto_secretbox_ZEROBYTES] = msg[i];
  crypto_secretbox(c, m, m.length, nonce, key);
  return c.subarray(crypto_secretbox_BOXZEROBYTES);
};

and run the test again, I see that m contains some garbage data (results differ from time to time):

Uint8Array {
  '0': 0,
  '1': 0,
  '2': 0,
  '3': 0,
  '4': 0,
  '5': 0,
  '6': 0,
  '7': 0,
  '8': 0,
  '9': 0,
  '10': 0,
  '11': 0,
  '12': 0,
  '13': 0,
  '14': 0,
  '15': 0,
  '16': 232,
  '17': 0,
  '18': 0,
  '19': 0,
  '20': 0,
  '21': 0,
  '22': 0,
  '23': 0,
  '24': 64,
  '25': 231,
  '26': 96,
  '27': 1,
  '28': 1,
  '29': 0,
  '30': 0,
  '31': 0,
  '32': 0 }

This only happens for this particular 33-byte Uint8Array in this particular setting (e.g. I couldn't reproduce this outside of this test). I can work around this by manually zeroing the array after creating it.

Any ideas? As I said, io.js v3.3.1 and previous Node versions doesn't have this bug.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Sep 17, 2015
@targos
Copy link
Member

targos commented Sep 17, 2015

/cc @trevnorris : maybe it has something to do with the new ArrayBufferAllocator from 74178a5 ?

@dchest
Copy link
Author

dchest commented Sep 17, 2015

FYI, can't reproduce with 4.0.0.

@trevnorris
Copy link
Contributor

Must be a side effect of that commit. Give me a bit to look at the issue. If I can't figure it out then I'll revert the change today.

@trevnorris
Copy link
Contributor

Issue identified and have a fix. Creating PR shortly.

@trevnorris trevnorris added confirmed-bug Issues with confirmed bugs. buffer Issues and PRs related to the buffer subsystem. labels Sep 17, 2015
@trevnorris trevnorris self-assigned this Sep 17, 2015
@dchest
Copy link
Author

dchest commented Sep 17, 2015

@trevnorris it fixes the issue for me, thanks!

trevnorris added a commit to trevnorris/node that referenced this issue Sep 17, 2015
The Uint8Array constructor doesn't hit the ArrayBuffer::Allocator() when
length == 0. So in these cases don't set the kNoZeroFill flag, since it
won't be reset.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: nodejs#2930
@trevnorris
Copy link
Contributor

@dchest thanks much for the report, and for the steps to reproduce. allowed me to find the problem and write the fix in under 30 mins. always makes life easier.

dchest added a commit to dchest/tweetnacl-js that referenced this issue Sep 17, 2015
There's a zero-filling bug with Uint8Arrays in 4.1, which breaks one
test: nodejs/node#2930 (comment)
trevnorris added a commit that referenced this issue Sep 18, 2015
Instantiating a Buffer of length zero would set the kNoZeroFill flag to
true but never actually call ArrayBuffer::Allocator(). Which means the
flag was never set back to false. The result was that the next
allocation would unconditionally not be zero filled.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: #2930
PR-URL: #2931
Reviewed-By: Rod Vagg <rod@vagg.org>
@trevnorris
Copy link
Contributor

Fixed by 0a329d2. Set to be released in v4.1.1. Should happen early next week.

trevnorris added a commit that referenced this issue Sep 20, 2015
Instantiating a Buffer of length zero would set the kNoZeroFill flag to
true but never actually call ArrayBuffer::Allocator(). Which means the
flag was never set back to false. The result was that the next
allocation would unconditionally not be zero filled.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: #2930
PR-URL: #2931
Reviewed-By: Rod Vagg <rod@vagg.org>
jasnell pushed a commit that referenced this issue Apr 26, 2016
This makes sure that `kNoZeroFill` flag is not accidentally set by
moving the all the flag operations directly inside `createBuffer()`.
It safeguards against logical errors like
#6006.

This also ensures that `kNoZeroFill` flag is always restored to 0 using
a try-finally block, as it could be not restored to 0 in cases of failed
or zero-size `Uint8Array` allocation.
It safeguards against errors like
#2930.
It also makes the `size > 0` check not needed there.

PR-URL: nodejs-private/node-private#30
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
This makes sure that `kNoZeroFill` flag is not accidentally set by
moving the all the flag operations directly inside `createBuffer()`.
It safeguards against logical errors like
#6006.

This also ensures that `kNoZeroFill` flag is always restored to 0 using
a try-finally block, as it could be not restored to 0 in cases of failed
or zero-size `Uint8Array` allocation.
It safeguards against errors like
#2930.
It also makes the `size > 0` check not needed there.

PR-URL: nodejs-private/node-private#30
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants