-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Change timingSafeEqual
to use byteLength
.
#21397
Conversation
Former implementation of `timingSafeEqual` would allow different length; allowing a core dump. ```shell [zeen3@zeen3 ~]$ node Running node v10.4.1 (npm v6.1.0) > crypto.timingSafeEqual(new BigUint64Array(32), new Uint32Array(32)) node[16304]: ../src/node_crypto.cc:5158:void node::crypto::TimingSafeEqual(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `(buf_length) == (Buffer::Length(args[1]))' failed. 1: node::Abort() [node] 2: 0x8938b5 [node] 3: 0x97212a [node] 4: 0xb00989 [node] 5: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node] 6: 0x2dbaff1841bd Aborted (core dumped) [zeen3@zeen3 ~]$ node Running node v10.4.1 (npm v6.1.0) > crypto.timingSafeEqual(new BigUint64Array(32), new Uint32Array(64)) RangeError [ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH]: Input buffers must have the same length at Object.timingSafeEqual (internal/crypto/util.js:81:11) > crypto.timingSafeEqual(new BigUint64Array(32), new Uint32Array(32)) node[16856]: ../src/node_crypto.cc:5158:void node::crypto::TimingSafeEqual(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `(buf_length) == (Buffer::Length(args[1]))' failed. 1: node::Abort() [node] 2: 0x8938b5 [node] 3: 0x97212a [node] 4: 0xb00989 [node] 5: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node] 6: 0x128c0a5041bd Aborted (core dumped) [zeen3@zeen3 ~]$ ``` Though unlikely to occur it's possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but can you add a regression test and update the commit log per the contributing guide? Cheers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crypto: make timingSafeEqual use bytewise length instead of array length
crypto.timingSafeEqual() can cause the core to abort
if the length parameter matches; however the internal
byte length differs. This commit makes the length
validation use bytewise (ArrayBufferLike) length
rather than array content length.
@bnoordhuis Yep; umm... How would I change the commit to that?
https://nodejs.org/docs/latest/api/crypto.html#crypto_crypto_timingsafeequal_a_b There are no changes needed for the docs as it only allows interchangeability between different types of For regression tests: ... I'm actually more surprised it passed the tests. checks what the tests actually are and starts editing |
@ZaneHannanAU
That specific check predates general ArrayBufferView support. Back then, only Buffer existed and it wasn't an instance of Uint8Array. |
Cool... Adding more tests for the moment. Currently just cloning my tree. ...yeah I've been using the git webui so I didn't need to clone it myself haha... |
Add various modern ArrayBuffer based timingSafeEqual variants.
Hrmm... the linter failed? ... Okay what? |
Fwiw, this is the failure from the test run itself:
|
Linter length < 80 Fix one issue where I wrote the wrong amount of tests.
@addaleax thank you that is better |
Related failure from https://ci.nodejs.org/job/node-test-commit-linuxone/2528/nodes=rhel72-s390x/:
|
Odd. Aren't all int, uint, float, double and bigint arrays stored as little endian within JS? |
I actually wrote
so that it wouldn't error under any normal circumstances. hrmm… |
... please don't error this time. You have no reason to.
Removed the |
According to travis; it's an internal error. $ curl --compressed '-s' 'https://api.travis-ci.com/v3/job/131286282/log.txt' | grep 'not ok' -n -C 10
6871- duration_ms: 0.138
6872- ...
6873-ok 15 async-hooks/test-emit-init
6874- ---
6875- duration_ms: 0.521
6876- ...
6877-ok 16 async-hooks/test-enable-in-init
6878- ---
6879- duration_ms: 0.112
6880- ...
6881:not ok 17 async-hooks/test-fseventwrap
6882- ---
6883- duration_ms: 0.112
6884- severity: fail
6885- exitcode: 1
6886- stack: |-
6887- internal/fs/watchers.js:167
6888- throw error;
6889- ^
6890-
6891- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js'
--
9157- duration_ms: 0.113
9158- ...
9159-ok 582 parallel/test-fs-open-flags
9160- ---
9161- duration_ms: 0.138
9162- ...
9163-ok 583 parallel/test-fs-open-numeric-flags
9164- ---
9165- duration_ms: 0.112
9166- ...
9167:not ok 584 parallel/test-fs-options-immutable
9168- ---
9169- duration_ms: 0.138
9170- severity: fail
9171- exitcode: 1
9172- stack: |-
9173- internal/fs/watchers.js:167
9174- throw error;
9175- ^
9176-
9177- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/parallel/test-fs-options-immutable.js'
--
9447- duration_ms: 0.138
9448- ...
9449-ok 650 parallel/test-fs-utimes
9450- ---
9451- duration_ms: 0.138
9452- ...
9453-ok 651 parallel/test-fs-unlink-type-check
9454- ---
9455- duration_ms: 0.141
9456- ...
9457:not ok 652 parallel/test-fs-watch-close-when-destroyed
9458- ---
9459- duration_ms: 0.138
9460- severity: fail
9461- exitcode: 1
9462- stack: |-
9463- internal/fs/watchers.js:167
9464- throw error;
9465- ^
9466-
9467- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0/watched-directory'
--
9469- at Object.watch (fs.js:1234:11)
9470- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch-close-when-destroyed.js:15:20)
9471- at Module._compile (internal/modules/cjs/loader.js:689:30)
9472- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9473- at Module.load (internal/modules/cjs/loader.js:599:32)
9474- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9475- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9476- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9477- at startup (internal/bootstrap/node.js:260:19)
9478- ...
9479:not ok 653 parallel/test-fs-watch
9480- ---
9481- duration_ms: 0.154
9482- severity: fail
9483- exitcode: 1
9484- stack: |-
9485- internal/fs/watchers.js:167
9486- throw error;
9487- ^
9488-
9489- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/watch1/foo'
--
9491- at Object.watch (fs.js:1234:11)
9492- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch.js:50:22)
9493- at Module._compile (internal/modules/cjs/loader.js:689:30)
9494- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9495- at Module.load (internal/modules/cjs/loader.js:599:32)
9496- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9497- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9498- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9499- at startup (internal/bootstrap/node.js:260:19)
9500- ...
9501:not ok 654 parallel/test-fs-watch-encoding
9502- ---
9503- duration_ms: 0.114
9504- severity: fail
9505- exitcode: 1
9506- stack: |-
9507- internal/fs/watchers.js:167
9508- throw error;
9509- ^
9510-
9511- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0'
--
9513- at Object.watch (fs.js:1234:11)
9514- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch-encoding.js:45:21)
9515- at Module._compile (internal/modules/cjs/loader.js:689:30)
9516- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9517- at Module.load (internal/modules/cjs/loader.js:599:32)
9518- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9519- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9520- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9521- at startup (internal/bootstrap/node.js:260:19)
9522- ...
9523:not ok 655 parallel/test-fs-watch-enoent
9524- ---
9525- duration_ms: 0.140
9526- severity: fail
9527- exitcode: 1
9528- stack: |-
9529- internal/fs/watchers.js:167
9530- throw error;
9531- ^
9532-
9533- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/file-to-watch'
--
9547- duration_ms: 0.113
9548- ...
9549-ok 657 parallel/test-fs-watch-stop-async
9550- ---
9551- duration_ms: 0.146
9552- ...
9553-ok 658 parallel/test-fs-watch-stop-sync
9554- ---
9555- duration_ms: 0.119
9556- ...
9557:not ok 659 parallel/test-fs-watchfile
9558- ---
9559- duration_ms: 0.138
9560- severity: fail
9561- exitcode: 1
9562- stack: |-
9563- internal/fs/watchers.js:167
9564- throw error;
9565- ^
9566-
9567- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/watch'
--
15912- duration_ms: 0.138
15913- ...
15914-ok 2245 sequential/test-fs-readfile-tostring-fail # TODO : Fix flaky test
15915- ---
15916- duration_ms: 6.924
15917- ...
15918-ok 2246 sequential/test-fs-stat-sync-overflow
15919- ---
15920- duration_ms: 0.272
15921- ...
15922:not ok 2247 sequential/test-fs-watch
15923- ---
15924- duration_ms: 0.111
15925- severity: fail
15926- exitcode: 1
15927- stack: |-
15928- internal/fs/watchers.js:167
15929- throw error;
15930- ^
15931-
15932- Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0/watch.txt'
$ curl --compressed '-s' 'https://api.travis-ci.com/v3/job/131286282/log.txt' | grep 'Error' -n -C 10
6765-[ RUN ] InspectorSocketTest.GetThenHandshake
6766-[ OK ] InspectorSocketTest.GetThenHandshake (1 ms)
6767-[ RUN ] InspectorSocketTest.WriteBeforeHandshake
6768-[ OK ] InspectorSocketTest.WriteBeforeHandshake (0 ms)
6769-[ RUN ] InspectorSocketTest.CleanupSocketAfterEOF
6770-[ OK ] InspectorSocketTest.CleanupSocketAfterEOF (20 ms)
6771-[ RUN ] InspectorSocketTest.EOFBeforeHandshake
6772-[ OK ] InspectorSocketTest.EOFBeforeHandshake (0 ms)
6773-[ RUN ] InspectorSocketTest.Send1Mb
6774-[ OK ] InspectorSocketTest.Send1Mb (17 ms)
6775:[ RUN ] InspectorSocketTest.ErrorCleansUpTheSocket
6776:[ OK ] InspectorSocketTest.ErrorCleansUpTheSocket (0 ms)
6777-[ RUN ] InspectorSocketTest.NoCloseResponseFromClient
6778-[ OK ] InspectorSocketTest.NoCloseResponseFromClient (0 ms)
6779-[ RUN ] InspectorSocketTest.HostCheckedForGET
6780-[ OK ] InspectorSocketTest.HostCheckedForGET (1 ms)
6781-[ RUN ] InspectorSocketTest.HostCheckedForUPGRADE
6782-[ OK ] InspectorSocketTest.HostCheckedForUPGRADE (0 ms)
6783-[----------] 20 tests from InspectorSocketTest (61 ms total)
6784-
6785-[----------] 10 tests from InspectorSocketServerTest
6786-[ RUN ] InspectorSocketServerTest.InspectorSessions
--
6881-not ok 17 async-hooks/test-fseventwrap
6882- ---
6883- duration_ms: 0.112
6884- severity: fail
6885- exitcode: 1
6886- stack: |-
6887- internal/fs/watchers.js:167
6888- throw error;
6889- ^
6890-
6891: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js'
6892- at FSWatcher.start (internal/fs/watchers.js:161:26)
6893- at Object.watch (fs.js:1234:11)
6894- at Object.<anonymous> (/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js:16:20)
6895- at Module._compile (internal/modules/cjs/loader.js:689:30)
6896- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
6897- at Module.load (internal/modules/cjs/loader.js:599:32)
6898- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
6899- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
6900- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
6901- at startup (internal/bootstrap/node.js:260:19)
--
9167-not ok 584 parallel/test-fs-options-immutable
9168- ---
9169- duration_ms: 0.138
9170- severity: fail
9171- exitcode: 1
9172- stack: |-
9173- internal/fs/watchers.js:167
9174- throw error;
9175- ^
9176-
9177: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/parallel/test-fs-options-immutable.js'
9178- at FSWatcher.start (internal/fs/watchers.js:161:26)
9179- at Object.watch (fs.js:1234:11)
9180- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-options-immutable.js:50:20)
9181- at Module._compile (internal/modules/cjs/loader.js:689:30)
9182- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9183- at Module.load (internal/modules/cjs/loader.js:599:32)
9184- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9185- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9186- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9187- at startup (internal/bootstrap/node.js:260:19)
--
9457-not ok 652 parallel/test-fs-watch-close-when-destroyed
9458- ---
9459- duration_ms: 0.138
9460- severity: fail
9461- exitcode: 1
9462- stack: |-
9463- internal/fs/watchers.js:167
9464- throw error;
9465- ^
9466-
9467: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0/watched-directory'
9468- at FSWatcher.start (internal/fs/watchers.js:161:26)
9469- at Object.watch (fs.js:1234:11)
9470- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch-close-when-destroyed.js:15:20)
9471- at Module._compile (internal/modules/cjs/loader.js:689:30)
9472- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9473- at Module.load (internal/modules/cjs/loader.js:599:32)
9474- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9475- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9476- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9477- at startup (internal/bootstrap/node.js:260:19)
--
9479-not ok 653 parallel/test-fs-watch
9480- ---
9481- duration_ms: 0.154
9482- severity: fail
9483- exitcode: 1
9484- stack: |-
9485- internal/fs/watchers.js:167
9486- throw error;
9487- ^
9488-
9489: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/watch1/foo'
9490- at FSWatcher.start (internal/fs/watchers.js:161:26)
9491- at Object.watch (fs.js:1234:11)
9492- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch.js:50:22)
9493- at Module._compile (internal/modules/cjs/loader.js:689:30)
9494- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9495- at Module.load (internal/modules/cjs/loader.js:599:32)
9496- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9497- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9498- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9499- at startup (internal/bootstrap/node.js:260:19)
--
9501-not ok 654 parallel/test-fs-watch-encoding
9502- ---
9503- duration_ms: 0.114
9504- severity: fail
9505- exitcode: 1
9506- stack: |-
9507- internal/fs/watchers.js:167
9508- throw error;
9509- ^
9510-
9511: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0'
9512- at FSWatcher.start (internal/fs/watchers.js:161:26)
9513- at Object.watch (fs.js:1234:11)
9514- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch-encoding.js:45:21)
9515- at Module._compile (internal/modules/cjs/loader.js:689:30)
9516- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9517- at Module.load (internal/modules/cjs/loader.js:599:32)
9518- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9519- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9520- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9521- at startup (internal/bootstrap/node.js:260:19)
--
9523-not ok 655 parallel/test-fs-watch-enoent
9524- ---
9525- duration_ms: 0.140
9526- severity: fail
9527- exitcode: 1
9528- stack: |-
9529- internal/fs/watchers.js:167
9530- throw error;
9531- ^
9532-
9533: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/file-to-watch'
9534- at FSWatcher.start (internal/fs/watchers.js:161:26)
9535- at Object.watch (fs.js:1234:11)
9536- at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-fs-watch-enoent.js:45:22)
9537- at Module._compile (internal/modules/cjs/loader.js:689:30)
9538- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
9539- at Module.load (internal/modules/cjs/loader.js:599:32)
9540- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
9541- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
9542- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
9543- at startup (internal/bootstrap/node.js:260:19)
--
9557-not ok 659 parallel/test-fs-watchfile
9558- ---
9559- duration_ms: 0.138
9560- severity: fail
9561- exitcode: 1
9562- stack: |-
9563- internal/fs/watchers.js:167
9564- throw error;
9565- ^
9566-
9567: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/watch'
9568- at FSWatcher.start (internal/fs/watchers.js:161:26)
9569- at Object.watch (fs.js:1234:11)
9570- at /home/travis/build/nodejs/node/test/parallel/test-fs-watchfile.js:95:8
9571- at /home/travis/build/nodejs/node/test/common/index.js:467:15
9572- at FSReqWrap.oncomplete (fs.js:145:20)
9573- ...
9574-ok 660 parallel/test-fs-watchfile-bigint
9575- ---
9576- duration_ms: 0.138
9577- ...
--
15922-not ok 2247 sequential/test-fs-watch
15923- ---
15924- duration_ms: 0.111
15925- severity: fail
15926- exitcode: 1
15927- stack: |-
15928- internal/fs/watchers.js:167
15929- throw error;
15930- ^
15931-
15932: Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0/watch.txt'
15933- at FSWatcher.start (internal/fs/watchers.js:161:26)
15934- at Object.watch (fs.js:1234:11)
15935- at Object.<anonymous> (/home/travis/build/nodejs/node/test/sequential/test-fs-watch.js:48:22)
15936- at Module._compile (internal/modules/cjs/loader.js:689:30)
15937- at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
15938- at Module.load (internal/modules/cjs/loader.js:599:32)
15939- at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
15940- at Function.Module._load (internal/modules/cjs/loader.js:530:3)
15941- at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
15942- at startup (internal/bootstrap/node.js:260:19)
--
16226- duration_ms: 0.138
16227- ...
16228-ok 2319 sequential/test-util-debug
16229- ---
16230- duration_ms: 1.318
16231- ...
16232-ok 2320 sequential/test-vm-timeout-rethrow
16233- ---
16234- duration_ms: 0.329
16235- ...
16236:make: *** [test-ci] Error 1
16237-
16239-The command "make -j2 test-ci" exited with 2.
store build cache
change detected (content changed, file is created, or file is deleted):
16243-/home/travis/.ccache/0/0/0a523a8379701ceabdcb4d26156b4c-313883.d
16244-/home/travis/.ccache/0/0/0a523a8379701ceabdcb4d26156b4c-313883.o
16245-/home/travis/.ccache/0/0/1f32509f042695d48e474e9c75d9e2-10299.manifest
16246-/home/travis/.ccache/0/0/417400d75eb58c79cec26d2773c726-3100.manifest
$ tl;dr clean travis and run again |
addendum:
no assertion errors; just ENOSPC errors. |
@ZaneHannanAU could you please rebase this on master? I'll then start a CI. |
Yes |
ping @ZaneHannanAU |
... just need to clone it and redo ... |
Reissued on #23341 due to varying issues with the original. |
crypto.timingSafeEqual() can cause the core to abort if the length parameter matches; however the internal byte length differs. This commit makes the length validation use bytewise (ArrayBufferLike) byteLength rather than array content length. Reissuing of nodejs#21397 with various modifications and fixes.
crypto.timingSafeEqual() can cause the core to abort if the length parameter matches; however the internal byte length differs. This commit makes the length validation use bytewise (ArrayBufferLike) byteLength rather than array content length. Reissuing of nodejs#21397 with various modifications and fixes.
Former implementation of
timingSafeEqual
would allow different length; allowing a core dump.Though unlikely to occur it's possible.
Checklist