crypto: use byteLength in timingSafeEqual by ZaneHannanAU · Pull Request #23341 · nodejs/node

ZaneHannanAU · 2018-10-08T23:17:30Z

Checklist

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

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 #21397 with various modifications and fixes.

ZaneHannanAU · 2018-10-09T03:01:08Z

Hm?

ZaneHannanAU · 2018-10-09T03:02:58Z

ah

ZaneHannanAU · 2018-10-09T04:47:24Z

... weird.

oyyd · 2018-10-09T06:14:14Z

Try npx core-validate-commit --no-validate-metadata 3dd887d which validates commit messages .

thefourtheye · 2018-10-09T06:26:51Z

cc @nodejs/crypto

ZaneHannanAU · 2018-10-09T06:59:08Z

... uh what should I do?

tniessen · 2018-10-09T12:49:55Z

Please follow the guidelines from the CONTRIBUTING file to correctly format your (first) commit message. You can use interactive rebasing to rewrite the first commit message, but there are other possibilities.

ZaneHannanAU · 2018-10-10T00:05:31Z

… I should really figure out git …

ZaneHannanAU · 2018-10-10T00:06:04Z

Is there a way to do it through the gh ui?

ZaneHannanAU · 2018-10-10T05:27:26Z

... I have no idea what I'm doing

ZaneHannanAU · 2018-10-10T05:53:40Z

... does this do anything?

thefourtheye · 2018-10-10T06:09:15Z

@ZaneHannanAU What are you trying to do? You can run the tests locally by ./configure && make -j4 test, if you are on Unix based OSes.

ZaneHannanAU · 2018-10-10T06:11:23Z

I'm trying to edit the commit message and as you can tell, I have no idea what I'm doing.

ZaneHannanAU · 2018-10-10T06:14:37Z

Okay; how can I rebase the mess I've made myself to just have 1 commit saying

crypto,errors: use byteLength in safeTimingEqual

for the time being?

add buffer level tests

tniessen · 2018-10-10T15:35:55Z

I took the liberty of fixing your branch accordingly, the old HEAD was c177921.

ZaneHannanAU · 2018-10-10T22:15:19Z

Thank you! You've absolutely saved me there…

bnoordhuis

Thanks, mostly LGTM but see comments.

bnoordhuis · 2018-11-19T11:14:36Z

 E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error);
 E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
- 'Input buffers must have the same length', RangeError);
+ 'Input buffers must have the same byteLength', RangeError);


You might want to consider undoing this change. Error message changes are considered semver-major, meaning this couldn't be released until Node.js 12.

I’m not sure, are we still doing that? I was under the impression that we don’t necessarily consider it semver-major when there’s an associated message code…

Agreed that error message changes shouldn't be semver major if there is an error code, since that was (one of) the advertised advantage in migrating to them. That said, I'm not sure there is much benefit to changing this. IMO, the error message was better before, even if it didn't completely reflect the implementation details.

Perhaps, Input buffers must have the same number of bytes? Same idea, a bit more human readable?

bnoordhuis · 2018-11-19T11:20:38Z

+ 0, 1, // 26
+ 1, 0, // 28
+ 1, 1, // 30
+ 0) // 31


Can you factor out these Buffer.of() calls? Just assign it once to a const expected = Buffer.of(...) and compare against that.

nodejs-github-bot · 2019-09-15T00:12:48Z

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

tniessen · 2019-09-25T19:06:22Z

I am closing this PR since it has become inactive and the relevant changes were merged in #29657.

@ZaneHannanAU I added you to 6174306 as a co-author.

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Oct 8, 2018

ZaneHannanAU mentioned this pull request Oct 8, 2018
Change timingSafeEqual to use byteLength. #21397
Closed
1 task

ZaneHannanAU changed the title ~~crypto: make timingSafeEqual use bytewise length instead of array length~~crypto,errors: make timingSafeEqual use bytewise length instead of array lengthOct 9, 2018

richardlau mentioned this pull request Oct 9, 2018
tools: apply linting to first commit in PRs #22452
Closed
2 tasks

ZaneHannanAU added 7 commits October 10, 2018 17:28

crypto: use byteLength in timingSafeEqual
b07480a

Update test-crypto-timing-safe-equal.js
e5ddde8
add buffer level tests

Input buffers byteLenght
976b4ca

Update test-crypto-timing-safe-equal.js
24f01c8

Update test-crypto-timing-safe-equal.js
e032fcf

semicolon
9902544

spelling
70febc0

tniessen force-pushed the patch-1 branch from c177921 to 70febc0Compare October 10, 2018 15:35

tniessen changed the title ~~crypto,errors: make timingSafeEqual use bytewise length instead of array length~~crypto: use byteLength in timingSafeEqualOct 10, 2018

bnoordhuis reviewed Nov 19, 2018
View reviewed changes

srl295 added the stalled Issues and PRs that are stalled. label Jul 16, 2019

Trott force-pushed the master branch from 1ecc406 to 49cf67eCompare September 17, 2019 16:52

tniessen mentioned this pull request Sep 23, 2019
crypto: use byteLength in timingSafeEqual #29657
Closed
4 tasks

tniessen closed this Sep 25, 2019

coderabbitaiBot mentioned this pull request Aug 5, 2026
KAN-354: extraction worker — country pre-build + on-demand Place mapping Alcagoita/Brush#318
Open
5 tasks

Uh oh!

Uh oh!

Conversation

ZaneHannanAU commented Oct 8, 2018

Checklist

Uh oh!

ZaneHannanAU commented Oct 9, 2018

Uh oh!

ZaneHannanAU commented Oct 9, 2018

Uh oh!

ZaneHannanAU commented Oct 9, 2018

Uh oh!

oyyd commented Oct 9, 2018

Uh oh!

thefourtheye commented Oct 9, 2018

Uh oh!

ZaneHannanAU commented Oct 9, 2018

Uh oh!

tniessen commented Oct 9, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

thefourtheye commented Oct 10, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

tniessen commented Oct 10, 2018

Uh oh!

ZaneHannanAU commented Oct 10, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuisNov 19, 2018

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 19, 2018

Choose a reason for hiding this comment

Uh oh!

cjihrigNov 20, 2018

Choose a reason for hiding this comment

Uh oh!

jasnellNov 21, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuisNov 19, 2018

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 15, 2019

Uh oh!

tniessen commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants