chore: fix review comments

libraries/adb-credential-nodejs/src/index.ts

@@ -1,7 +1,7 @@
 // cspell: ignore adbkey
 
 import { existsSync } from "node:fs";
-import { mkdir, readFile, writeFile } from "node:fs/promises";
+import { mkdir, opendir, readFile, stat, writeFile } from "node:fs/promises";
 import { homedir, hostname, userInfo } from "node:os";
 import { resolve } from "node:path";
 
@@ -15,7 +15,11 @@ import {
 import type { TangoKey, TangoKeyStorage } from "@yume-chan/adb-credential-web";
 
 export class TangoNodeStorage implements TangoKeyStorage {
-    constructor() {}
+    #logger: ((message: string) => void) | undefined;
+
+    constructor(logger: ((message: string) => void) | undefined) {
+        this.#logger = logger;
+    }
 
     async #getAndroidDirPath() {
         const dir = resolve(homedir(), ".android");
@@ -59,35 +63,76 @@ export class TangoNodeStorage implements TangoKeyStorage {
     }
 
     async #readPrivateKey(path: string) {
-        const pem = await readFile(path, "utf8");
+        try {
-        return decodeBase64(
+            const pem = await readFile(path, "utf8");
-            pem
+            return decodeBase64(
-                // Parse PEM in Lax format (allows spaces/line breaks everywhere)
+                pem
-                // https://datatracker.ietf.org/doc/html/rfc7468
+                    // Parse PEM in Lax format (allows spaces/line breaks everywhere)
-                .replaceAll(/-----(BEGIN|END) PRIVATE KEY-----/g, "")
+                    // https://datatracker.ietf.org/doc/html/rfc7468
-                .replaceAll(/\x20|\t|\r|\n|\v|\f/g, ""),
+                    .replaceAll(/-----(BEGIN|END) PRIVATE KEY-----/g, "")
-        );
+                    .replaceAll(/\x20|\t|\r|\n|\v|\f/g, ""),
+            );
+        } catch (e) {
+            throw new Error("Invalid private key file: " + path, { cause: e });
+        }
     }
 
-    async #readPublicKeyName(path: string) {
+    async #readPublicKeyName(path: string): Promise<string | undefined> {
-        // NOTE: Google ADB actually never reads the `.pub` file for name,
+        // Google ADB actually never reads the `.pub` file for name,
         // it always returns the default name.
+        // So we won't throw an error if the file can't be read.
 
-        const publicKeyPath = path + ".pub";
+        try {
-        if (!existsSync(publicKeyPath)) {
+            const publicKeyPath = path + ".pub";
-            return this.#getDefaultName();
+            if (!(await stat(publicKeyPath)).isFile()) {
+                return undefined;
+            }
+
+            const publicKey = await readFile(publicKeyPath, "utf8");
+            return publicKey.split(" ")[1]?.trim();
+        } catch {
+            return undefined;
         }
-
-        const publicKey = await readFile(publicKeyPath, "utf8");
-        return publicKey.split(" ")[1]?.trim() ?? this.#getDefaultName();
     }
 
     async #readKey(path: string): Promise<TangoKey> {
         const privateKey = await this.#readPrivateKey(path);
-        const name = await this.#readPublicKeyName(path);
+        const name =
+            (await this.#readPublicKeyName(path)) ?? this.#getDefaultName();
         return { privateKey, name };
     }
 
+    async *#readVendorKeys(path: string) {
+        const stats = await stat(path);
+
+        if (stats.isFile()) {
+            try {
+                yield await this.#readKey(path);
+            } catch (e) {
+                this.#logger?.(String(e));
+            }
+            return;
+        }
+
+        if (stats.isDirectory()) {
+            for await (const dirent of await opendir(path)) {
+                if (!dirent.isFile()) {
+                    continue;
+                }
+
+                if (!dirent.name.endsWith(".adb_key")) {
+                    continue;
+                }
+
+                try {
+                    yield await this.#readKey(resolve(path, dirent.name));
+                } catch (e) {
+                    this.#logger?.(String(e));
+                }
+            }
+        }
+    }
+
     async *load(): AsyncGenerator<TangoKey, void, void> {
         const userKeyPath = await this.#getUserKeyPath();
         if (existsSync(userKeyPath)) {
@@ -98,7 +143,7 @@ export class TangoNodeStorage implements TangoKeyStorage {
         if (vendorKeys) {
             const separator = process.platform === "win32" ? ";" : ":";
             for (const path of vendorKeys.split(separator)) {
-                yield await this.#readKey(path);
+                yield* this.#readVendorKeys(path);
             }
         }
     }

libraries/adb-credential-web/src/storage/prf/storage.ts

@@ -107,7 +107,13 @@ export class TangoPrfStorage implements TangoKeyStorage {
         const salt = new Uint8Array(HkdfSaltLength);
         crypto.getRandomValues(salt);
 
-        const aesKey = await deriveAesKey(prfOutput, info, salt);
+        let aesKey: CryptoKey;
+        try {
+            aesKey = await deriveAesKey(prfOutput, info, salt);
+        } finally {
+            // Clear secret memory
+            toUint8Array(prfOutput).fill(0);
+        }
 
         const iv = new Uint8Array(AesIvLength);
         crypto.getRandomValues(iv);
@@ -128,13 +134,6 @@ export class TangoPrfStorage implements TangoKeyStorage {
         });
 
         await this.#storage.save(bundle, name);
-
-        // Clear secret memory
-        //   * No way to clear `aesKey`
-        //   * `info`, `salt`, `iv`, `encrypted` and `bundle` are not secrets
-        //   * `data` is owned by caller and will be cleared by caller
-        //   * Need to clear `prfOutput`
-        toUint8Array(prfOutput).fill(0);
     }
 
     async *load(): AsyncGenerator<TangoKey, void, void> {
@@ -153,11 +152,17 @@ export class TangoPrfStorage implements TangoKeyStorage {
 
             this.#prevId = bundle.id as Uint8Array<ArrayBuffer>;
 
-            const aesKey = await deriveAesKey(
+            let aesKey: CryptoKey;
-                prfOutput,
+            try {
-                bundle.hkdfInfo as Uint8Array<ArrayBuffer>,
+                aesKey = await deriveAesKey(
-                bundle.hkdfSalt as Uint8Array<ArrayBuffer>,
+                    prfOutput,
-            );
+                    bundle.hkdfInfo as Uint8Array<ArrayBuffer>,
+                    bundle.hkdfSalt as Uint8Array<ArrayBuffer>,
+                );
+            } finally {
+                // Clear secret memory
+                toUint8Array(prfOutput).fill(0);
+            }
 
             const decrypted = await crypto.subtle.decrypt(
                 {
@@ -168,16 +173,13 @@ export class TangoPrfStorage implements TangoKeyStorage {
                 bundle.encrypted as Uint8Array<ArrayBuffer>,
             );
 
-            yield { privateKey: new Uint8Array(decrypted), name };
+            try {
-
+                yield { privateKey: new Uint8Array(decrypted), name };
-            // Clear secret memory
+            } finally {
-            //   * No way to clear `aesKey`
+                // Clear secret memory
-            //   * `info`, `salt`, `iv`, `encrypted` and `bundle` are not secrets
+                // Caller is not allowed to use `decrypted` after `yield` returns
-            //   * `data` is owned by caller and will be cleared by caller
+                new Uint8Array(decrypted).fill(0);
-            //   * Caller is not allowed to use `decrypted` after `yield` returns
+            }
-            //   * Need to clear `prfOutput`
-            toUint8Array(prfOutput).fill(0);
-            new Uint8Array(decrypted).fill(0);
         }
     }
 }

libraries/adb/src/utils/base64.ts

@@ -295,6 +295,15 @@ function encodeBackward(
     }
 }
 
+function getCharIndex(input: string, offset: number) {
+    const charCode = input.charCodeAt(offset);
+    const index = charToIndex[charCode];
+    if (index === undefined) {
+        throw new Error("Invalid Base64 character: " + input[offset]);
+    }
+    return index;
+}
+
 export function decodeBase64(input: string): Uint8Array<ArrayBuffer> {
     let padding: number;
     if (input[input.length - 2] === "=") {
@@ -309,17 +318,18 @@ export function decodeBase64(input: string): Uint8Array<ArrayBuffer> {
     let sIndex = 0;
     let dIndex = 0;
 
-    while (sIndex < input.length - (padding !== 0 ? 4 : 0)) {
+    const loopEnd = input.length - (padding !== 0 ? 4 : 0);
-        const a = charToIndex[input.charCodeAt(sIndex)]!;
+    while (sIndex < loopEnd) {
+        const a = getCharIndex(input, sIndex);
         sIndex += 1;
 
-        const b = charToIndex[input.charCodeAt(sIndex)]!;
+        const b = getCharIndex(input, sIndex);
         sIndex += 1;
 
-        const c = charToIndex[input.charCodeAt(sIndex)]!;
+        const c = getCharIndex(input, sIndex);
         sIndex += 1;
 
-        const d = charToIndex[input.charCodeAt(sIndex)]!;
+        const d = getCharIndex(input, sIndex);
         sIndex += 1;
 
         result[dIndex] = (a << 2) | ((b & 0b11_0000) >> 4);
@@ -333,23 +343,23 @@ export function decodeBase64(input: string): Uint8Array<ArrayBuffer> {
     }
 
     if (padding === 1) {
-        const a = charToIndex[input.charCodeAt(sIndex)]!;
+        const a = getCharIndex(input, sIndex);
         sIndex += 1;
 
-        const b = charToIndex[input.charCodeAt(sIndex)]!;
+        const b = getCharIndex(input, sIndex);
         sIndex += 1;
 
-        const c = charToIndex[input.charCodeAt(sIndex)]!;
+        const c = getCharIndex(input, sIndex);
 
         result[dIndex] = (a << 2) | ((b & 0b11_0000) >> 4);
         dIndex += 1;
 
         result[dIndex] = ((b & 0b1111) << 4) | ((c & 0b11_1100) >> 2);
     } else if (padding === 2) {
-        const a = charToIndex[input.charCodeAt(sIndex)]!;
+        const a = getCharIndex(input, sIndex);
         sIndex += 1;
 
-        const b = charToIndex[input.charCodeAt(sIndex)]!;
+        const b = getCharIndex(input, sIndex);
 
         result[dIndex] = (a << 2) | ((b & 0b11_0000) >> 4);
     }