Repository: jhillyerd/enmime
Pull request #254 (2022-06-02 09:21:40)

fix: buffer overrun when quoted printable is at the end of line and output buffer
In QPCleaner, write behind the buffer end could occur, when there was a `=XX` quoted printable token, line length was 1023 and remaining dest buffer space was only 1 byte. Because of line length, QPCleaner inserted `=\r\n`, but only the equal sign fit into the output buffer. The rest went to qp.overflow. 

Then the algorithm wrote to `dest[n]` without any additional checks, but `n` already pointed behind the buffer end.

Now the equal sign is also written to overflow in such situation.

I've also noticed, that qp.lineLength is not incremented when qp.overflow is put at the beginning of new buffer and also when `=` from quoted printable token is inserted. Seems as a bug to me, but I don't fix it in this PR.

pavelbazika: > Thanks, one small change requested. Should I file a bug for the other issue you mentioned?

No need to file a bug, I'll fix it and come with a PR. I just wanted to ensure, that `lineLength` should be incremented in all circumstances.

Patch:
diff --git a/internal/coding/quotedprint.go b/internal/coding/quotedprint.go
index 6610a7a..3e2c81f 100644
--- a/internal/coding/quotedprint.go
+++ b/internal/coding/quotedprint.go
@@ -52,6 +52,16 @@ func (qp *QPCleaner) Read(dest []byte) (n int, err error) {
 		qp.lineLen++
 	}
 
+	// safeWriteByte outputs a signle byte, storing overflow for next read. Updates counters.
+	safeWriteByte := func(in byte) {
+		if n < destLen {
+			dest[n] = in
+			n++
+		} else {
+			qp.overflow = append(qp.overflow, in)
+		}
+	}
+
 	// writeBytes outputs multiple bytes, storing overflow for next read. Updates counters.
 	writeBytes := func(in []byte) {
 		nc := copy(dest[n:], in)
@@ -100,8 +110,7 @@ func (qp *QPCleaner) Read(dest []byte) (n int, err error) {
 				return 0, err
 			}
 			if validHexBytes(hexBytes) {
-				dest[n] = b
-				n++
+				safeWriteByte(b)
 			} else {
 				writeBytes(escapedEquals)
 			}


Test patch:
diff --git a/internal/coding/quotedprint_test.go b/internal/coding/quotedprint_test.go
index 2d3ea0a..36e407e 100644
--- a/internal/coding/quotedprint_test.go
+++ b/internal/coding/quotedprint_test.go
@@ -148,7 +148,41 @@ func TestQPCleanerLineBreakBufferFull(t *testing.T) {
 		t.Fatal(err)
 	}
 	if n != 1025 {
-		t.Errorf("Unexpected result length: %d", n)
+		t.Error("got:", n, "want:", 1025)
+	}
+}
+
+func TestQPCleanerEqualSignOverflow(t *testing.T) {
+	// Test processing of =3D quoted printable token being at the end of line
+	// in combination with almost full buffer.
+	// qp cleaner should add =\r\n and then output =3D without buffer overflow
+
+	input := append(bytes.Repeat([]byte("abc"), 341), []byte("=3D")...)
+	inbuf := bytes.NewBuffer(input)
+	qp := coding.NewQPCleaner(inbuf)
+
+	output := make([]byte, 1024)
+	n, err := qp.Read(output)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n != 1024 {
+		t.Error("got:", n, "want:", 1024)
+	}
+	if string(output[1020:]) != "abc=" {
+		t.Error("got:", string(output[1020:]), "want:", "abc=")
+	}
+
+	n, err = qp.Read(output)
+	if err != io.EOF {
+		t.Fatal(err)
+	}
+	if n != 5 {
+		t.Error("got:", n, "want:", 5)
+	}
+	output = output[:n]
+	if string(output) != "\r\n=3D" {
+		t.Error("got:", string(output), "want:", "\r\n=3D")
 	}
 }