fix: Create DataView with correct byteLength in timingSafeEqual #3208
Conversation
crypto/timing_safe_equal.ts
Outdated
| @@ -12,10 +12,10 @@ export function timingSafeEqual( | |||
| return false; | |||
| } | |||
| if (!(a instanceof DataView)) { | |||
| a = new DataView(ArrayBuffer.isView(a) ? a.buffer : a); | |||
| a = new DataView(ArrayBuffer.isView(a) ? a.buffer : a, 0, a.byteLength); | |||
There was a problem hiding this comment.
This looks fine when a is an ArrayBuffer, but the second argument seems to need to be a.byteOffset when a is a TypedArray.
Especially the example given in #3207 doesn't return true with this change
There was a problem hiding this comment.
Great catch. Added the test to cover it as well.
There was a problem hiding this comment.
Tested original case as well to double-check:
import { hash } from "https://deno.land/x/argontwo/mod.ts";
import { encode, decode } from "https://deno.land/std/encoding/base64url.ts";
import { timingSafeEqual } from "../../deno/deno_std/node/crypto.ts";
let x = hash("bong", decode("4ereginClY4QuqtT2j-8_g"));
let dec = decode("QAEyt51JgnxV8sc0wVKv63OcLO5BAzqFooCxXAnuBUo");
console.log(timingSafeEqual(dec, x)); // true
| Deno.test({ | ||
| name: "[timingSafeEqual] - Uint8Array w. different buffer sizes (a > b)", | ||
| fn() { | ||
| const a = new SharedArrayBuffer(4); |
There was a problem hiding this comment.
minor point, but do we need to use SharedArrayBuffer here? How about ArrayBuffer?
There was a problem hiding this comment.
ArrayBuffer is a transferable object, so we won't be able to create Uint8Array after manipulating it with the DataView. And we do want to do that, to make Uint8Array a specific slice of the buffer for the purpose of these specific tests.
|
@kamilogorek Thanks for looking into this! I left a few comments |
| const va = new DataView(a); | ||
| va.setUint8(1, 212); | ||
| va.setUint8(2, 213); | ||
| const ua = new Uint8Array(a, 1, 2); |
|
@kt3k I believe this API might have been copied into |
The example presented in #3207 uses
argontwo, which under the hood returnsUint8Arraywhich memory buffer is taken directly from a WASM instance, making originalb.getUint8(i)access out of bounds for 2nd argument totimingSafeEqual.ref: https://deno.land/x/[email protected]/wasm/mod.ts?source#L14
ref: https://deno.land/x/[email protected]/mod.ts?source#L126
Fixes #3207
Note#1: I use
SharedArrayBufferfor cleaner assertions.Note#2: changed names of 2 tests, because VS Code integration deduplicates tests with the same name.