fix(auth): do more validation of TLV messages (#4064)

We were not validating:
- that the encoded tag is the one we expected
- that the first length byte is ≤ 0x80 if it isn't one of the special 0x81 or 0x82 values
- that the TLV response didn't contain more data than the length said it should

libs/auth/package.json

@@ -59,6 +59,7 @@
     "@votingworks/test-utils": "workspace:*",
     "esbuild-runner": "^2.2.1",
     "eslint-plugin-vx": "workspace:*",
+    "fast-check": "2.23.2",
     "is-ci-cli": "^2.2.0",
     "jest": "^29.6.2",
     "jest-junit": "^16.0.0",

libs/auth/src/apdu.test.ts

@@ -1,4 +1,5 @@
 import { Buffer } from 'buffer';
+import fc from 'fast-check';
 import { Byte } from '@votingworks/types';
 
 import {
@@ -152,7 +153,15 @@ test.each<{ valueLength: number; expectedTlvLength: Byte[] }>([
 
 test('constructTlv value length validation', () => {
   expect(() => constructTlv(0x01, Buffer.alloc(65536))).toThrow(
-    'TLV value is too large'
+    'value length is too large for TLV encoding: 0x10000 > 0xffff'
+  );
+});
+
+test('parseTlv invalid length', () => {
+  expect(() =>
+    parseTlv(0x01, Buffer.concat([Buffer.of(0x01, 0xff), Buffer.alloc(0xff)]))
+  ).toThrow(
+    'TLV length is invalid: received 0xff, but expected a value <= 0x82 for the first length byte'
   );
 });
 
@@ -163,24 +172,28 @@ test.each<{
 }>([
   {
     tagAsByteOrBuffer: 0x01,
-    tlv: Buffer.concat([Buffer.of(0x01, 0x7f), Buffer.alloc(127)]),
-    expectedOutput: [Buffer.of(0x01), Buffer.of(0x7f), Buffer.alloc(127)],
+    tlv: Buffer.concat([Buffer.of(0x01, 0x7f), Buffer.alloc(0x7f)]),
+    expectedOutput: [Buffer.of(0x01), Buffer.of(0x7f), Buffer.alloc(0x7f)],
   },
   {
     tagAsByteOrBuffer: 0x01,
-    tlv: Buffer.concat([Buffer.of(0x01, 0x81, 0xff), Buffer.alloc(255)]),
-    expectedOutput: [Buffer.of(0x01), Buffer.of(0x81, 0xff), Buffer.alloc(255)],
+    tlv: Buffer.concat([Buffer.of(0x01, 0x81, 0xff), Buffer.alloc(0xff)]),
+    expectedOutput: [
+      Buffer.of(0x01),
+      Buffer.of(0x81, 0xff),
+      Buffer.alloc(0xff),
+    ],
   },
   {
     tagAsByteOrBuffer: 0x01,
     tlv: Buffer.concat([
       Buffer.of(0x01, 0x82, 0xff, 0xff),
-      Buffer.alloc(65535),
+      Buffer.alloc(0xffff),
     ]),
     expectedOutput: [
       Buffer.of(0x01),
       Buffer.of(0x82, 0xff, 0xff),
-      Buffer.alloc(65535),
+      Buffer.alloc(0xffff),
     ],
   },
   {
@@ -202,3 +215,34 @@ test('ResponseApduError', () => {
   expect(error.hasStatusWord([0x6b, 0x82])).toEqual(false);
   expect(error.hasStatusWord([0x6a, 0x83])).toEqual(false);
 });
+
+test('constructTlv/parseTlv round trip', () => {
+  function asHex(b: Byte): string {
+    return b.toString(16).padStart(2, '0');
+  }
+
+  fc.assert(
+    fc.property(
+      fc.integer({ min: 0, max: 0xff }),
+      fc.integer({ min: 0, max: 0xffff }),
+      (tag, valueLength) => {
+        const value = Buffer.alloc(valueLength);
+        const tlv = constructTlv(tag as Byte, value);
+        const [parsedTag, parsedLength, parsedValue] = parseTlv(
+          tag as Byte,
+          tlv
+        );
+        expect(parsedTag).toEqual(Buffer.of(tag));
+        expect(parsedLength).toBeInstanceOf(Buffer);
+        expect(parsedValue.equals(value)).toBeTruthy();
+
+        const wrongTag = ((tag + 1) % 0x100) as Byte;
+        expect(() => parseTlv(wrongTag, tlv)).toThrow(
+          `TLV tag (<Buffer ${asHex(
+            tag as Byte
+          )}>) does not match expected tag (<Buffer ${asHex(wrongTag)}>)`
+        );
+      }
+    )
+  );
+});

libs/auth/src/apdu.ts

@@ -1,6 +1,7 @@
 /* eslint-disable max-classes-per-file */
 import { Buffer } from 'buffer';
-import { assert } from '@votingworks/basics';
+import { inspect } from 'util';
+import { assert, assertDefined } from '@votingworks/basics';
 import { asHexString, Byte, isByte } from '@votingworks/types';
 
 /**
@@ -179,7 +180,7 @@ export class CardCommand {
     this.ins = input.ins;
     this.p1 = input.p1;
     this.p2 = input.p2;
-    this.data = input.data ?? Buffer.from([]);
+    this.data = input.data ?? Buffer.of();
   }
 
   /**
@@ -225,35 +226,39 @@ export function constructTlv(
   tagAsByteOrBuffer: Byte | Buffer,
   value: Buffer
 ): Buffer {
-  const tag: Buffer = Buffer.isBuffer(tagAsByteOrBuffer)
+  const tag = Buffer.isBuffer(tagAsByteOrBuffer)
     ? tagAsByteOrBuffer
     : Buffer.of(tagAsByteOrBuffer);
 
   /**
    * The convention for TLV length is as follows:
-   * - 0xXX           if value length < 128 bytes
-   * - 0x81 0xXX      if value length > 128 and < 256 bytes
-   * - 0x82 0xXX 0xXX if value length > 256 and < 65536 bytes
+   * - 0xXX           if value length ≤ 0x80 bytes
+   * - 0x81 0xXX      if value length > 0x80 and ≤ 0xff bytes
+   * - 0x82 0xXX 0xXX if value length > 0xff and ≤ 0xffff bytes
    *
    * For example:
    * - 51 bytes   --> Buffer.of(51)            --> 0x33           (33 is 51 in hex)
    * - 147 bytes  --> Buffer.of(0x81, 147)     --> 0x81 0x93      (93 is 147 in hex)
    * - 3017 bytes --> Buffer.of(0x82, 11, 201) --> 0x82 0x0b 0xc9 (bc9 is 3017 in hex)
    */
-  let tlvLength: Buffer;
+  let lengthBytes: Buffer;
   const valueNumBytes = value.length;
-  if (valueNumBytes < 128) {
-    tlvLength = Buffer.of(valueNumBytes);
-  } else if (valueNumBytes < 256) {
-    tlvLength = Buffer.of(0x81, valueNumBytes);
-  } else if (valueNumBytes < 65536) {
+  if (valueNumBytes <= 0x80) {
+    lengthBytes = Buffer.of(valueNumBytes);
+  } else if (valueNumBytes <= 0xff) {
+    lengthBytes = Buffer.of(0x81, valueNumBytes);
+  } else if (valueNumBytes <= 0xffff) {
     // eslint-disable-next-line no-bitwise
-    tlvLength = Buffer.of(0x82, valueNumBytes >> 8, valueNumBytes & 255);
+    lengthBytes = Buffer.of(0x82, valueNumBytes >> 8, valueNumBytes & 0xff);
   } else {
-    throw new Error('TLV value is too large');
+    throw new Error(
+      `value length is too large for TLV encoding: 0x${valueNumBytes.toString(
+        16
+      )} > 0xffff`
+    );
   }
 
-  return Buffer.concat([tag, tlvLength, value]);
+  return Buffer.concat([tag, lengthBytes, value]);
 }
 
 /**
@@ -262,24 +267,54 @@ export function constructTlv(
 export function parseTlv(
   tagAsByteOrBuffer: Byte | Buffer,
   tlv: Buffer
-): [Buffer, Buffer, Buffer] {
-  const tagLength = Buffer.isBuffer(tagAsByteOrBuffer)
-    ? tagAsByteOrBuffer.length
-    : 1;
-
-  let tlvLengthLength = 1; // Derpiest variable name, I know
-  const tlvLengthFirstByte = tlv.at(tagLength);
-  if (tlvLengthFirstByte === 0x81) {
-    tlvLengthLength = 2;
-  } else if (tlvLengthFirstByte === 0x82) {
-    tlvLengthLength = 3;
+): [tag: Buffer, length: Buffer, value: Buffer] {
+  const expectedTagBytes = Buffer.isBuffer(tagAsByteOrBuffer)
+    ? tagAsByteOrBuffer
+    : Buffer.of(tagAsByteOrBuffer);
+  const expectedTagLength = expectedTagBytes.length;
+  const tagBytes = tlv.subarray(0, expectedTagLength);
+  assert(
+    tagBytes.equals(expectedTagBytes),
+    `TLV tag (${inspect(tagBytes)}) does not match expected tag (${inspect(
+      expectedTagBytes
+    )})`
+  );
+
+  let lengthBytesLength: number;
+  let valueBytesLength: number;
+  const lengthBytesFirst = assertDefined(tlv.at(expectedTagLength));
+  if (lengthBytesFirst === 0x81) {
+    const lengthBytesSecond = tlv.at(expectedTagLength + 1);
+    lengthBytesLength = 2;
+    valueBytesLength = assertDefined(lengthBytesSecond);
+  } else if (lengthBytesFirst === 0x82) {
+    const lengthBytesSecond = tlv.at(expectedTagLength + 1);
+    const lengthBytesThird = tlv.at(expectedTagLength + 2);
+    lengthBytesLength = 3;
+    valueBytesLength =
+      // eslint-disable-next-line no-bitwise
+      (assertDefined(lengthBytesSecond) << 8) + assertDefined(lengthBytesThird);
+  } else if (lengthBytesFirst <= 0x80) {
+    lengthBytesLength = 1;
+    valueBytesLength = assertDefined(lengthBytesFirst);
+  } else {
+    throw new Error(
+      `TLV length is invalid: received 0x${lengthBytesFirst.toString(
+        16
+      )}, but expected a value <= 0x82 for the first length byte`
+    );
   }
 
-  return [
-    tlv.subarray(0, tagLength), // Tag
-    tlv.subarray(tagLength, tagLength + tlvLengthLength), // Length
-    tlv.subarray(tagLength + tlvLengthLength), // Value
-  ];
+  const lengthBytes = tlv.subarray(
+    expectedTagLength,
+    expectedTagLength + lengthBytesLength
+  );
+  const valueBytes = tlv.subarray(
+    expectedTagLength + lengthBytesLength,
+    expectedTagLength + lengthBytesLength + valueBytesLength
+  );
+
+  return [tagBytes, lengthBytes, valueBytes];
 }
 
 /**