Bug: headerPos set incorrectly due to varying byte length

[titanism]

Per https://github.com/nodemailer/wildduck/blob/master/lib/message-splitter.js the chunk might look like:

Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

H

But the raw email sent across actually was:

Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi,

Note that the i, gets chopped off, and the resulting body and headers joined is:

Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

i,

e.g. the H gets chopped off (first letter after header line break)

The trailing line break is the culprit. Curious if you think we should dummy-proof this @andris9 (e.g. for developers in case they accidentally send email with a trailing line break)?

I think it'd be nice to dummy-proof against this, perhaps using something similar to this code:

// <https://github.com/sindresorhus/strip-final-newline/blob/a1bfe78e3a3de2f73ed3a7600932d7cc952732b4/index.js#L18-26>
const LF = '\n';
const LF_BINARY = LF.codePointAt(0);
const CR = '\r';
const CR_BINARY = CR.codePointAt(0);

const stripFinalNewlineBinary = (input) =>
  input.at(-1) === LF_BINARY
    ? input.subarray(0, input.at(-2) === CR_BINARY ? -2 : -1)
    : input;

Then calling stripFinalNewlineBinary perhaps or just looking for it if headers were found. A rewrite might be needed.

[andris9]

I tried the following, seems to work fine. Is there something missing?

'use strict';

const { PassThrough } = require('stream');
const Splitter = require('mailsplit/lib/message-splitter');
const Joiner = require('mailsplit/lib/message-joiner');

let splitter = new Splitter();
let joiner = new Joiner();

const message = `Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi,
`.replace(/\r?\n/g, '\r\n');

const stream = new PassThrough();

// pipe all streams together
stream.pipe(splitter).pipe(joiner).pipe(process.stdout);

stream.end(message);

Output:

Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi,

[titanism]

This is with Wildduck message splitter, not mailsplit's @andris9

[titanism]

Also definitely try it without the .replace(/\r?\n/g, '\r\n');

[titanism]

I'm going to try to get a reproducible case with Wildduck here in the next hour or so.

[titanism]

I'm not able to reproduce this so it must be something specific to my application code. Taking a look now.

[titanism]

Perhaps this is a bug with smtp-server instead.

[titanism]

I have not been able to reproduce with smtp-server yet, only via tests. Going to try to see how I got this to reproduce in the first place now.

For example, this works OK:

const process = require('node:process');
const { Buffer } = require('node:buffer');

const Splitter = require('mailsplit/lib/message-splitter');

const getStream = require('get-stream');
const mxConnect = require('mx-connect');
const nodemailer = require('nodemailer');
const pify = require('pify');
const { SMTPServer } = require('smtp-server');
const { listen } = require('async-listen');

const asyncMxConnect = pify(mxConnect);

const message = `Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi,

`;

(async () => {
  const smtp = new SMTPServer({
    disabledCommands: ['AUTH'],
    onRcptTo(address, session, fn) {
      fn();
    },
    onConnect(session, fn) {
      fn();
    },

    async onData(stream, session, fn) {
      // <https://github.com/nodemailer/wildduck/issues/781>
      // const splitter = new Splitter();
      // const joiner = new Joiner();
      // pipe all streams together
      // stream.pipe(splitter).pipe(joiner).pipe(process.stdout);

      const splitter = new Splitter();
      stream.pipe(splitter);

      const body = await getStream.buffer(splitter);

      const { headers } = splitter;

      console.log('headers', headers.build().toString());
      console.log('body', body.toString());

      const raw = Buffer.concat([headers.build(), body]);
      console.log('raw', raw.toString()); // <--- this is OK

      fn();
    },
    logger: console,
    secure: false
  });

  const url = await listen(smtp.server, 0);

  console.log('server started', url);

  console.log('connecting to mx');

  const mx = await asyncMxConnect({
    target: 'localhost', // url.hostname, [::] doesn't work
    port: url.port
  });

  console.log('mx', mx);

  const transporter = nodemailer.createTransport({
    logger: console,
    debug: true,
    host: mx.host,
    port: mx.port,
    connection: mx.socket,
    ignoreTLS: true,
    secure: false,
    tls: { rejectUnauthorized: false }
  });

  const info = await transporter.sendMail({
    envelope: {
      from: '[email protected]',
      to: '[email protected]'
    },
    raw: message
  });

  console.log('info', info);

  process.exit(0);
})();

[titanism]

Okay I've been able to reproduce this. This is 100% some issue when you consume a stream from SMTP server with WildDuck's message splitter. The moment we swap from WildDuck's to mailsplit the issue goes away.

[titanism]

Going to try to get a reproducible code sample and then share it here, and try to find underlying cause.

[titanism]

It appears that checkHeaders is getting invoked with data arg equal to this:

Message-ID: <5a1x54l4e1@日本語.org>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <yoshie@日本語.org>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

H

https://github.com/nodemailer/wildduck/blob/82445592301047d0b43992106091d10456ae5611/lib/message-splitter.js#L49

This indicates to me that it's not splitting it properly for some reason.

[titanism]

Okay @andris9 this appears to be due to 3 characters followed by the line break. Any less than 3 is OK, and any more than 3 is OK. So ending with a line break and immediately before it having AAA results in AA, or more visually you can do ABC followed by a line break and it will result in BC. The first character gets removed.

[titanism]

@andris9 the bug is somewhere in here I think due to length/integer comparison

https://github.com/nodemailer/wildduck/blob/82445592301047d0b43992106091d10456ae5611/lib/message-splitter.js#L64-L78

[titanism]

I think that this might be the culprit:

headerPos = i - lblen + 1;

Because it's using 4 as a hard-coded value, it's doing i which could be something like 63 + 4 + 1 so it's splitting incorrectly. It might instead need to use the index of the LF_BINARY or CR_BINARY.

Edit: I think this is definitely the bug. headerPos is getting set incorrectly. Then later this code occurs https://github.com/nodemailer/wildduck/blob/82445592301047d0b43992106091d10456ae5611/lib/message-splitter.js#L88 so the wrong chunk value is set.

[louis-lau]

Might be worth sending a single message once done instead of 11 messages within minutes of each other haha, kind of overwhelms the inbox ;)

Otherwise not complaining, keep up the investigation and bug fixing :)

[titanism]

This happens for these two cases (note that there are NO line breaks at the end and so that case is ruled out):

Input:

Message-ID: <iyezf8uz6i@日本語.org>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <xolani@日本語.org>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hiya,

Output:

Message-ID: <iyezf8uz6i@日本語.org>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <xolani@日本語.org>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

iya,

...and also...

Input:

Message-ID: <6d1aj0of25@日本語.org>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <eunice@日本語.org>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

ABC

Output:

Message-ID: <6d1aj0of25@日本語.org>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <eunice@日本語.org>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

BC

[titanism]

Sorry @louis-lau 🤦

[titanism]

I think (?) I found the fix...

https://github.com/nodemailer/wildduck/blob/82445592301047d0b43992106091d10456ae5611/lib/message-splitter.js#L87

-            if (data.length - 1 > headerPos) {
+            if (data.length - 1 >= headerPos) {
                let chunk = data.slice(headerPos);
                this.bodySize += chunk.length;
                // this would be the first chunk of data sent downstream
                // from now on we keep header and body separated until final delivery
                setImmediate(() => this.push(chunk));
            }

[titanism]

PR submitted at #782

[andris9]

I can confirm that there is something strange going on, there is some kind of a bug here. The proposed fix only fixes the occurrence of this specific input, but it does not fix the main issue.

I would say this issue is a very low priority because normally we operate on 64kb chunks, not a few byte chunks of input, and as such, it is nearly impossible to trigger this bug in WildDuck under normal operation.

My test case (change chunklen value to get different chunk lengths of input):

'use strict';

const MessageSplitter = require('wildduck/lib/message-splitter');

const message = `Message-ID: <[email protected]>
To: [email protected]
List-Unsubscribe: [email protected]
From: Test <[email protected]>
Subject: testing this
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi,
`.replace(/\r?\n/g, '\r\n');

let chunklen = 2;

console.log('<<<<<<');

let messageSplitter = new MessageSplitter();

messageSplitter.on('readable', () => {
    let c;

    while ((c = messageSplitter.read()) !== null) {
        process.stdout.write(c);
    }
});

messageSplitter.on('end', () => {
    console.log('>>>>>>');
    console.log('READY');
});

let writer = async () => {
    let d = Buffer.from(message);

    let buf;
    let i;

    let chunks = [];

    for (i = 0; i < d.length; i++) {
        let c = d[i];
        if (i % chunklen === 0) {
            let allocLen = chunklen;
            if (d.length - i < chunklen) {
                allocLen = d.length - i;
            }
            buf = Buffer.alloc(allocLen);
        }
        buf[i % chunklen] = c;
        if (i % chunklen === chunklen - 1) {
            chunks.push(buf);
        }
    }

    if (i % chunklen !== chunklen - 1) {
        chunks.push(buf);
    }

    for (let chunk of chunks) {
        messageSplitter.write(chunk);
        await new Promise(r => setTimeout(r, 1));
    }

    messageSplitter.end();
};

writer();

[github-actions]

This issue is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[titanism]

bump