Issure with isAligned32 in getDst with shared ArrayBuffer

[atomictag]

Thank you for the awesome library - and glad it got an official audit!

Using xchacha20poly1305here.
The getDst utility introduced in 1.1.0 now performs the check isAligned32(dst)

export function isAligned32(bytes: Uint8Array) {
  return bytes.byteOffset % 4 === 0;
}

which checks the alignment on the byteOffset on the underlying ArrayBuffer (not exactly sure why on the byteOffset - see below).
This breaks in 1.1.0 if the backing buffer contains more data than just the ciphertext, if it happens to be misaligned.
No issue with 1.0.0.

Say the raw byte array input for decryption contains concatenated metadata + nonce + data.
Now extract 3 subarrays "metadata", "nonce", "data", where the "data" subarray contains ciphertext and tag, so

const output = data.subarray(0, data.length - TAG_LEN)  
const pt = chacha.decrypt(data, output)

However now output.byteOffset depends on whatever was in the raw byte array input (incl. "metadata"), so it is not guaranteed to be aligned at all, therefore the isAligned32 check fails.

[paulmillr]

Could you provide a test vector?

[paulmillr]

  const data = new Uint8Array(41).fill(5);

  const key = new Uint8Array(32).fill(2);
  const nonce = new Uint8Array(24).fill(3);
  const chacha = xchacha20poly1305(key, nonce)

  console.log('initial', data.subarray(0, 8));
  const output = data.subarray(8, 32);
  const ciphertext = chacha.encrypt(data.subarray(0, 8), output);
  console.log('encrypted', ciphertext, output);

  chacha.decrypt(ciphertext, data.subarray(32, 40));
  console.log('decrypted', data.subarray(32, 40));
  deepStrictEqual(data.subarray(0, 8), data.subarray(32, 40))

This works properly.

[paulmillr]

Fixed in dfa5ba3

[atomictag]

I have changed your example to illustrate the issue.
The code below works fine with 1.0.0 but fails with 1.1.0

    const data = new Uint8Array(41).fill(5);

    const key = new Uint8Array(32).fill(2);
    const nonce = new Uint8Array(24).fill(3);
    const chacha = xchacha20poly1305(key, nonce);

    console.log("initial", data.subarray(0, 8));
    const output = data.subarray(8, 32);
    const ciphertext = chacha.encrypt(data.subarray(0, 8), output);
    console.log("encrypted", ciphertext, output);

    // Later on the receiver side receives the ciphertext in a buffer at an arbitrary offset
    // In my use case this is a CBOR-decoded byte array backed by a large buffer
    const buffer = new Uint8Array(256).fill(0);
    const offset = 31; // <== this offset must be aligned in 1.1.0 (so it will fail here). It works just fine with 1.0.0
    buffer.set(ciphertext, offset);
    {
      const ciphertext = buffer.subarray(offset, offset + 24); // 8 bytes + 16 bytes tag
      const output = buffer.subarray(offset, offset + 24 - 16); // 8 bytes
      chacha.decrypt(ciphertext, output);
      console.log("decrypted", output);
      expect(data.subarray(0, 8)).toEqual(output);
    }

Haven't tried the change in dfa5ba3 yet.
Many thanks for your support.

[atomictag]

Tested the modified example above with 1.1.1, and the failure is now "Invalid tag".
The reason is that

export function getOutput(expectedLength: number, out?: Uint8Array, onlyAligned = true) {
  if (out === undefined) return new Uint8Array(expectedLength);
  if (out.length !== expectedLength)
    throw new Error(
      'invalid output length, expected at least ' + expectedLength + ', got: ' + out.length
    );
  if (onlyAligned && !isAligned32(out)) throw new Error('invalid output, must be aligned');
  return out.subarray(0, expectedLength).fill(0);
}

in the last line creates a subarray of "out" of the same length and fills it with zero.
However if "out" and "ciphertext" are mapped on the same buffer this will also set part of the ciphertext to zero, and the tag check will fail.

Removing the fill(0) solves the issue trivially.

I guess fill(0) is just there to ensure the output is initialized as new Uint8Array(expectedLength) does automatically, or is there a specific expectation that the output is zero-initialized? In the latter case I don't see how ciphertext and output can share the same physical buffer in this fashion

[paulmillr]

@atomictag removing fill(0) will make tests fail for xsalsa20poly1305, which places things differently. Try doing this, npm install && npm run build && npm test.

[paulmillr]

7e444bf ensures input and output do not overlap. Your example can be rewritten as follows:

  const ciphertext = buffer.subarray(offset, offset + 24); // 8 bytes + 16 bytes tag
  const output = buffer.subarray(offset + 24, offset + 24 + 8); // 8 bytes

Reusing input + output makes sense for stream ciphers (plain salsa, plain chacha, plain ctr), however, it's much harder to get correctly working for everyone else.

[atomictag]

As far as I can tell (just ran a quick test with 1.1.2) the sample code in the README "Reuse array for input and output" fails

import { chacha20poly1305 } from '@noble/ciphers/chacha';
import { utf8ToBytes } from '@noble/ciphers/utils';
import { randomBytes } from '@noble/ciphers/webcrypto';

const key = randomBytes(32);
const nonce = randomBytes(12);
const inputLength = 12;
const tagLength = 16;

const buf = new Uint8Array(inputLength + tagLength);
const _data = utf8ToBytes('hello, noble'); // length == 12
buf.set(_data, 0); // first inputLength bytes
const _start = buf.subarray(0, inputLength);

const chacha = chacha20poly1305(key, nonce);
chacha.encrypt(_start, buf);
chacha.decrypt(buf, _start); // _start now same as _data

This was also failing with 1.1.1 with the same invalid tag error for exactly the same reason as in my test.
So it seems we lost a very useful feature of this library after 1.1.0 - namely the sharing of the "input" and "output" buffers.

The way 1.0.0 worked was perfect - if ciphertext is in 1Mb I don't need to allocate another 1M to decrypt.
Forcing input/output buffers to not overlap clearly makes this impossible - and changes a very core feature (at least for my use cases) of this library.

May I ask what requirements triggered this change in behavior between 1.0.0 and 1.1.0?

Many thanks

[paulmillr]

It only worked in ONE cipher. If it works in one case and not the other then it is unreliable.

Why can’t you use one big array with separate parts for input and output?

[paulmillr]

I don't need to allocate another 1M to decrypt

what are params of your system?

[atomictag]

It only worked in ONE cipher. If it works in one case and not the other then it is unreliable.

The docs weren't very clear on which cypher supported the "Reuse array for input and output" feature, but the example in the README is using chacha20poly1305 and it worked fine with xchacha20poly1305 as well up to 1.0.0.

Anyways, AFAICT the exact code in the README (which uses chacha20poly1305) does not work with neither 1.1.2 nor 1.1.1

what are params of your system?

It's a bit complicated but in a nutshell imagine a pipeline of thousands of messages ranging from a few hundred bytes to megabytes streamed as CBOR envelopes containing encrypted bytes - from response buffer to decoded output not a single additional byte is allocated (CBOR decoding and decryption are both "in-place").

Why can’t you use one big array with separate parts for input and output?

Of course I could arrange a scratch buffer for decryption but that would impact some aspects of the system and break some other parts - so I am wondering if it is worth taking the risk for the sake of staying up to date with my preferred cipher toolkit, or whether I should stick to 1.0.0 which served me well so far until I fully understand what has changed between 1.0.0 and 1.1.x to break this

Note that I totally understand that the fill(0) removal trick would break other ciphers - but at the same time it shows it is still perfectly possible to get xchacha20poly1305 in 1.1.2 to work exactly as in 1.0.0 re "Reuse array for input and output" by just having a specific getOutput() (or the new overlap-check could be used to determine if zeroing is required or not?) - but I probably don't fully understand the ramifications of this yet

Many thanks

[paulmillr]

readme example fixed