fix(sender): fail closed on rand error (#1233)

Sai Asish Y and SAY-5 created 1 month ago

## What?

Treats `crypto/rand` failure as fatal in `sender.go` instead of falling
back to a `time.UnixNano()`-based outer boundary. Replaces both the
signed-only and signed+encrypted call sites with a
`smimeOuterBoundary()` helper that propagates the error, plus regression
tests using an injectable `randReader`.

## Why?

The previous fallback produced a predictable boundary that could be
collided by an attacker-controlled body part, breaking S/MIME envelope
integrity. Failing closed on rand errors removes the predictable path
entirely; tests pin the new behavior so the fallback can't silently
return.

Closes #1127.

Co-authored-by: SAY-5 <[e-mail]>

Change summary

sender/sender.go      | 37 ++++++++++++++++++++------------
sender/sender_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+), 14 deletions(-)

Detailed changes

sender/sender.go

@@ -9,6 +9,7 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
+	"io"
 	"mime"
 	"mime/multipart"
 	"mime/quotedprintable"
@@ -75,6 +76,22 @@ func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) {
 	}
 }
 
+// randReader is the source of randomness for boundary generation. It is a
+// variable so tests can swap it with a deterministic or failing reader. By
+// default it is crypto/rand.Reader.
+var randReader io.Reader = rand.Reader
+
+// smimeOuterBoundary returns a fresh, high-entropy MIME boundary for an S/MIME
+// multipart/signed wrapper. If crypto/rand cannot supply randomness it returns
+// an error rather than degrading to a predictable, time-based fallback.
+func smimeOuterBoundary() (string, error) {
+	var rb [12]byte
+	if _, err := io.ReadFull(randReader, rb[:]); err != nil {
+		return "", fmt.Errorf("smime: failed to read random bytes for outer boundary: %w", err)
+	}
+	return "signed-" + fmt.Sprintf("%x", rb[:]), nil
+}
+
 // generateMessageID creates a unique Message-ID header.
 func generateMessageID(from string) string {
 	buf := make([]byte, 16)
@@ -278,13 +295,9 @@ func SendEmail(account *config.Account, to, cc, bcc []string, subject, plainBody
 				return nil, err
 			}
 
-			var rb [12]byte
-			var outerBoundary string
-			if _, rerr := rand.Read(rb[:]); rerr == nil {
-				outerBoundary = "signed-" + fmt.Sprintf("%x", rb[:])
-			} else {
-				// fallback to time-based boundary if crypto/rand fails
-				outerBoundary = "signed-" + fmt.Sprintf("%d", time.Now().UnixNano())
+			outerBoundary, err := smimeOuterBoundary()
+			if err != nil {
+				return nil, err
 			}
 			var signedMsg bytes.Buffer
 			fmt.Fprintf(&signedMsg, "Content-Type: multipart/signed; protocol=\"application/pkcs7-signature\"; micalg=\"sha-256\"; boundary=\"%s\"\r\n\r\n", outerBoundary)
@@ -482,13 +495,9 @@ func SendEmail(account *config.Account, to, cc, bcc []string, subject, plainBody
 			return nil, err
 		}
 
-		var rb [12]byte
-		var outerBoundary string
-		if _, rerr := rand.Read(rb[:]); rerr == nil {
-			outerBoundary = "signed-" + fmt.Sprintf("%x", rb[:])
-		} else {
-			// fallback to time-based boundary if crypto/rand fails
-			outerBoundary = "signed-" + fmt.Sprintf("%d", time.Now().UnixNano())
+		outerBoundary, err := smimeOuterBoundary()
+		if err != nil {
+			return nil, err
 		}
 		var signedMsg bytes.Buffer
 		fmt.Fprintf(&signedMsg, "Content-Type: multipart/signed; protocol=\"application/pkcs7-signature\"; micalg=\"sha-256\"; boundary=\"%s\"\r\n\r\n", outerBoundary)

sender/sender_test.go

@@ -1,10 +1,61 @@
 package sender
 
 import (
+	"errors"
+	"io"
 	"strings"
 	"testing"
 )
 
+type failingReader struct{}
+
+func (failingReader) Read(p []byte) (int, error) {
+	return 0, errors.New("simulated crypto/rand failure")
+}
+
+// TestSMIMEOuterBoundary_RandFailure ensures that a crypto/rand failure surfaces
+// as an error rather than silently producing a predictable, time-based
+// boundary that an attacker could collide with (issue #1127).
+func TestSMIMEOuterBoundary_RandFailure(t *testing.T) {
+	orig := randReader
+	t.Cleanup(func() { randReader = orig })
+	randReader = failingReader{}
+
+	got, err := smimeOuterBoundary()
+	if err == nil {
+		t.Fatalf("expected error when crypto/rand fails, got boundary %q", got)
+	}
+	if got != "" {
+		t.Errorf("expected empty boundary on error, got %q", got)
+	}
+}
+
+// TestSMIMEOuterBoundary_Success ensures the happy path returns a non-empty,
+// random-looking boundary with the expected prefix.
+func TestSMIMEOuterBoundary_Success(t *testing.T) {
+	b1, err := smimeOuterBoundary()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !strings.HasPrefix(b1, "signed-") {
+		t.Errorf("boundary should start with 'signed-', got %q", b1)
+	}
+	// 12 random bytes => 24 hex chars; total length 7 + 24 = 31.
+	if len(b1) != len("signed-")+24 {
+		t.Errorf("unexpected boundary length: got %d (%q)", len(b1), b1)
+	}
+	b2, err := smimeOuterBoundary()
+	if err != nil {
+		t.Fatalf("unexpected error on second call: %v", err)
+	}
+	if b1 == b2 {
+		t.Errorf("two consecutive boundaries should differ, both got %q", b1)
+	}
+}
+
+// Ensure io is referenced even if a future refactor removes it indirectly.
+var _ io.Reader = failingReader{}
+
 // TestGenerateMessageID ensures the Message-ID has the correct format.
 func TestGenerateMessageID(t *testing.T) {
 	from := "[e-mail]"