From efdebf1dba999f2a74b5e131954fdf3394545fb3 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Wed, 6 May 2026 03:49:47 -0700 Subject: [PATCH] fix(sender): fail closed on rand error (#1233) ## 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 --- sender/sender.go | 37 +++++++++++++++++++------------ sender/sender_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 14 deletions(-) diff --git a/sender/sender.go b/sender/sender.go index 225b2952ca08a05c99b961907f6f63f2bde4e235..824a40fb3b62f6524fceebe2e9aa1351043bd6f7 100644 --- a/sender/sender.go +++ b/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) diff --git a/sender/sender_test.go b/sender/sender_test.go index 25b195b937d67c919cc52d9f5005abc6e9198f57..61eb95ec34a0753a86d7283d63eaccb77a6b2317 100644 --- a/sender/sender_test.go +++ b/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 := "test@example.com"