fmartingr/smtp2shoutrrr

Watch

Fork 0

FMG-3: fix SMTP authentication bypass, remote DoS, and silent delivery failures #9

Merged

fmartingr merged 2 commits from butterrobot/fmg-3-smtp-backend-fixes into main

2026-09-07 22:27:05 +02:00

AGit

butterrobot commented

2026-09-07 21:41:47 +02:00

Collaborator

Three defects in the SMTP backend, all predating the FMG-2 dependency upgrade.

Authentication bypass

The credential check joined its two comparisons with &&, so it only rejected a client that got both halves wrong — a correct username with any password authenticated, as did a correct password with any username. Both comparisons now have to pass, and both are evaluated before either is acted on so the timing does not distinguish a wrong username from a wrong password. They compare SHA-256 digests through subtle.ConstantTimeCompare, which keeps the length of the configured credential out of the timing as well.

Fixing the comparison alone does not close the hole. go-smtp advertises AUTH but leaves enforcement to the backend, so a client could skip AUTH entirely and still have its message forwarded. The session now tracks authentication and refuses MAIL, RCPT and DATA without it.

Nor does the gate mean anything while the credentials it checks can be guessed. SetDefaults invented Username = "username" and Password = "password" when the config omitted them, with only a slog.Warn — so a config.toml naming nothing but a port and its recipients relayed to anyone who tried the obvious pair, for exactly the deployments that had configured the least. Validate now rejects unset credentials the way it already rejects an unusable target, and the invented defaults are gone.

Behavior change: a client that used to send without issuing AUTH now has to be configured with the credentials from config.toml, and a deployment that never set them will not start until it does. Both documented in the README.

Remote denial of service

A malformed Content-Type reached log.Fatalf, which calls os.Exit(1). One message with an attacker-controlled header terminated the daemon and took every other in-flight message with it. Body() now returns the parse error like the rest of the function does.

Silent notification loss

Data logged forwarding errors and returned 250, so a client whose notification reached no target at all treated the message as delivered and never retried. The reply now reflects what happened:

250 — at least one target accepted it. A partial failure is logged but still accepted, since a retry redelivers to every target and would duplicate the notification on the ones that already have it. This keeps the existing TestPartialTargetFailure contract.
451 — no target accepted it. Worth retrying.
550 5.6.0 — the message cannot be parsed. No retry can fix the same bytes.
550 5.3.5 — the recipient has no notification targets configured. A configuration fault; no retry can add targets. LoadConfig blocks such a config, so this is only reachable from one built in code.

Session transaction state

Session.Reset was empty while go-smtp calls it after every message, so recipients accumulated across a connection and the second message on it was also forwarded to the first one's targets.

Fixing that introduced a data race, caught in review: Reset runs on the connection goroutine, but go-smtp runs Data on its own goroutine for a BDAT transfer and does not block RSET during one. A mutex now guards the recipients and the authentication flag across Rcpt, Data and Reset, and Data snapshots the recipients up front instead of reading the slice while forwarding.

Validation

make format, make ci-lint (0 issues), make test (-race, 94.2% coverage), make check and make build all pass locally.

Every regression test was confirmed to fail against the code it guards:

the six original ones against origin/main — the malformed Content-Type one by killing the test binary outright, which is the reported DoS;
TestResetDuringChunkedTransferIsSynchronized against da8e8e4, where -race reports Session.Reset racing Session.Data on all three runs attempted;
TestLoadConfigRejectsUnsetCredentials against da8e8e4's config.go.

Known gaps, not addressed here

Body() returns ("", nil) for a multipart/* with an unusable boundary and for any media type that is neither multipart/ nor text/, so an empty notification is pushed to every target and answered 250.
forwardEmail breaks after the first matching [[Recipients]] entry, so a message addressed to two matching entries notifies only the first and still answers 250. Pre-existing.
A message for an address matching no entry, with no [CatchAll], is accepted with 250 and dropped. Rejecting at RCPT would be more honest.
smtp.ErrAuthRequired is go-smtp's 502 5.7.0 where RFC 4954 §6 specifies 530 5.7.0. Left on the library's own constant.

Closes FMG-3

Three defects in the SMTP backend, all predating the FMG-2 dependency upgrade. ## Authentication bypass The credential check joined its two comparisons with `&&`, so it only rejected a client that got *both* halves wrong — a correct username with any password authenticated, as did a correct password with any username. Both comparisons now have to pass, and both are evaluated before either is acted on so the timing does not distinguish a wrong username from a wrong password. They compare SHA-256 digests through `subtle.ConstantTimeCompare`, which keeps the length of the configured credential out of the timing as well. Fixing the comparison alone does not close the hole. go-smtp advertises `AUTH` but leaves enforcement to the backend, so a client could skip `AUTH` entirely and still have its message forwarded. The session now tracks authentication and refuses `MAIL`, `RCPT` and `DATA` without it. Nor does the gate mean anything while the credentials it checks can be guessed. `SetDefaults` invented `Username = "username"` and `Password = "password"` when the config omitted them, with only a `slog.Warn` — so a `config.toml` naming nothing but a port and its recipients relayed to anyone who tried the obvious pair, for exactly the deployments that had configured the least. `Validate` now rejects unset credentials the way it already rejects an unusable target, and the invented defaults are gone. **Behavior change:** a client that used to send without issuing `AUTH` now has to be configured with the credentials from `config.toml`, and a deployment that never set them will not start until it does. Both documented in the README. ## Remote denial of service A malformed `Content-Type` reached `log.Fatalf`, which calls `os.Exit(1)`. One message with an attacker-controlled header terminated the daemon and took every other in-flight message with it. `Body()` now returns the parse error like the rest of the function does. ## Silent notification loss `Data` logged forwarding errors and returned `250`, so a client whose notification reached no target at all treated the message as delivered and never retried. The reply now reflects what happened: - `250` — at least one target accepted it. A *partial* failure is logged but still accepted, since a retry redelivers to every target and would duplicate the notification on the ones that already have it. This keeps the existing `TestPartialTargetFailure` contract. - `451` — no target accepted it. Worth retrying. - `550 5.6.0` — the message cannot be parsed. No retry can fix the same bytes. - `550 5.3.5` — the recipient has no notification targets configured. A configuration fault; no retry can add targets. `LoadConfig` blocks such a config, so this is only reachable from one built in code. ## Session transaction state `Session.Reset` was empty while go-smtp calls it after every message, so recipients accumulated across a connection and the second message on it was also forwarded to the first one's targets. Fixing that introduced a data race, caught in review: `Reset` runs on the connection goroutine, but go-smtp runs `Data` on its own goroutine for a `BDAT` transfer and does not block `RSET` during one. A mutex now guards the recipients and the authentication flag across `Rcpt`, `Data` and `Reset`, and `Data` snapshots the recipients up front instead of reading the slice while forwarding. ## Validation `make format`, `make ci-lint` (0 issues), `make test` (`-race`, 94.2% coverage), `make check` and `make build` all pass locally. Every regression test was confirmed to fail against the code it guards: - the six original ones against `origin/main` — the malformed `Content-Type` one by killing the test binary outright, which is the reported DoS; - `TestResetDuringChunkedTransferIsSynchronized` against `da8e8e4`, where `-race` reports `Session.Reset` racing `Session.Data` on all three runs attempted; - `TestLoadConfigRejectsUnsetCredentials` against `da8e8e4`'s `config.go`. ## Known gaps, not addressed here - `Body()` returns `("", nil)` for a `multipart/*` with an unusable boundary and for any media type that is neither `multipart/` nor `text/`, so an empty notification is pushed to every target and answered `250`. - `forwardEmail` breaks after the first matching `[[Recipients]]` entry, so a message addressed to two matching entries notifies only the first and still answers `250`. Pre-existing. - A message for an address matching no entry, with no `[CatchAll]`, is accepted with `250` and dropped. Rejecting at `RCPT` would be more honest. - `smtp.ErrAuthRequired` is go-smtp's `502 5.7.0` where RFC 4954 §6 specifies `530 5.7.0`. Left on the library's own constant. Closes FMG-3

butterrobot added 1 commit

2026-09-07 21:41:47 +02:00

fix: require authentication, survive malformed headers, report failed delivery (FMG-3)

CI / goreleaser-lint (pull_request) Successful in 3s

Details

CI / test (pull_request) Successful in 9m20s

Details

CI / format (pull_request) Successful in 7m30s

Details

CI / lint (pull_request) Successful in 8m25s

Details

CI / build (pull_request) Successful in 3m22s

Details

da8e8e478c

The SMTP credential check joined its two comparisons with `&&`, so it only
rejected a client that got both halves wrong: a correct username with any
password authenticated, as did a correct password with any username. Both
comparisons now have to pass, and both run before either is acted on so the
timing does not distinguish a wrong username from a wrong password. They
compare SHA-256 digests through subtle.ConstantTimeCompare, which keeps the
length of the configured credential out of the timing too.

Fixing the comparison alone does not close the hole. go-smtp advertises AUTH
but leaves enforcement to the backend, so a client could skip AUTH entirely
and still have its message forwarded. The session now tracks authentication
and refuses MAIL, RCPT and DATA without it.

A malformed Content-Type reached log.Fatalf, which exits the process. One
message with an attacker-controlled header terminated the daemon and took
every other in-flight message with it. Body() now returns the parse error like
the rest of the function does.

Data logged forwarding errors and returned 250, so a client whose notification
reached no target at all considered the message delivered and never retried.
It now answers 451 when no target accepted the message and 550 when the
message cannot be parsed, which no retry could fix. A partial failure is still
accepted, since a retry would redeliver to every target and duplicate the
notification on the ones that already have it.

Session.Reset was empty while go-smtp calls it after every message, so
recipients accumulated across a connection and the second message was also
forwarded to the first one's targets.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>

butterrobot added 1 commit

2026-09-07 22:01:00 +02:00

fix: guard session transaction state and require SMTP credentials (FMG-3)

CI / goreleaser-lint (pull_request) Successful in 3s

Details

CI / format (pull_request) Successful in 2m23s

Details

CI / lint (pull_request) Successful in 2m43s

Details

CI / test (pull_request) Successful in 11m10s

Details

CI / build (pull_request) Successful in 11m16s

Details

d028358be7

Addresses the two merge-blocking findings from the review of PR #9.

Session.Reset wrote the transaction's recipients on the connection goroutine
while go-smtp runs Data on its own goroutine for a BDAT transfer, and RSET is
not blocked during one — so a reset could land in the middle of a chunked
transfer and race the forward. A mutex now guards the recipients and the
authentication flag across Rcpt, Data and Reset, and Data snapshots the
recipients up front rather than reading the slice while forwarding. The
regression drives a raw BDAT transfer with an RSET pipelined behind the chunk
that completes the headers; under -race it reports the race reliably without
the guard.

SetDefaults invented Username = "username" and Password = "password" when the
config omitted them, with only a warning, so a config naming nothing but a
port and its recipients relayed to anyone who tried the obvious guess — for
exactly the deployments that had configured the least, and while the README
promised an authentication that was never configured. Validate now rejects
unset credentials the way it already rejects an unusable target, and the
invented defaults are gone. The fixtures of the neighbouring config tests
carry credentials so each still fails for the reason it names.

Also corrects a 550/451 inversion introduced by the previous commit and
flagged in the same review: a recipient with no usable targets is a
configuration fault, so it now answers 550 5.3.5 rather than asking the sender
to retry something no redelivery can fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>