When signing repeated headers, today we hash each instance of a header
from top to bottom.
However, RFC 6376 section 5.4.2 is clear they should be processed bottom
to top, and this is also what our verifier (correctly) expects.
The bug causes our signatures to be (wrongly) invalid for messages that
have repeated signed headers with different values.
Note that repeated headers that are not signed (e.g. Received:) are not
problematic. This only affects headers we sign, like To:, From:,
Subject:.
This patch fixes the problem by adjusting the order in which Sign
processes repeated headers.
This bug was found by fuzzing the sign-verify cycle, which will be added
in a later patch.
Today, if a message does not have a blank line (which separates headers
and body), we also include all lines except the first as the body (even
the ones parsed as headers). This is incorrect.
This patch fixes the code so that in those messages, all the lines are
headers, and the body is empty.
Note that since SMTP messages have to end with a new line, that final
line will act as the separator if there is no body. So this bug only
impacts signing/verification of files on disk via chasquid-util.
When parsing a header that has a continuation line, today we append that
line to it, which copies the whole header each time.
On normal messages this is negligible, but a header with a very large
number of continuation lines would cause a DoS through work
amplification.
This patch fixes the problem by making header continuation handling
linear: we now accumulate the header lines in a list, and then join them
only once.
Thanks to John JEAN <john@wargan.com> who reported a similar issue
(fixed in a previous patch), which led to finding this bug.
When verifying emails, for each entry in the h= tag, we would scan all
headers, leading to quadratic behaviour.
Similarly, when signing, for each instance of a signed header we would
scan all headers.
This could lead to unauthenticated DoS via work amplification.
This patch fixes the problem by going through the headers only once, so
behaviour is now linear, and also limiting the size of the h= tag for
defense in depth.
The limit is in line with what other implementations use, so it should
not cause any interoperability problems.
Thanks to John JEAN <john@wargan.com> for finding and reporting this
problem. The detailed report can be found in
https://johnjean.io/advisories/chasquid-unauthenticated-remote-dos/.
Today, we treat l=0 to mean the whole body is signed. However, RFC 6376
is clear that means _none_ of the body is signed.
This patch fixes that behaviour to match the RFC.
Cross-checking other implementations for potential compatibility issues,
OpenDKIM and Exim both treat l=0 the same way this patch does.
In practice, l=0 is extremely rare, and that's why this incorrect
behaviour persisted unnoticed for so long.
Note this incorrect behaviour would cause valid signatures to fail to
verify, but never allow invalid signatures to pass.
Currently, if the value of the l= tag is larger than the body, that
causes an out-of-bound access and the server crashes.
The RFC is clear that the value should be <= the size of the
canonicalized body.
This patch fixes the problem by validating the length appropriately, and
returning a permanent failure if it's invalid.
Thanks to John JEAN <john@wargan.com> who reported a similar issue
(fixed in a previous patch), which led to finding this bug.
In mustIDNAToASCII, we accidentally return the unconverted domain on
success, and the failed conversion on error.
This affects the sender rewriting when forwarding, and can only be
triggered via aliases in local IDN domains.
This patch fixes the problem by adjusting the logic, and adds tests to
prevent future regressions.
Domains in addresses had very lax validation, which allowed them to
include characters like "/".
That could be exploited to trigger a daemon panic: a MAIL FROM
"x@evil/foo" could be accepted and, on failed delivery, a DSN queued.
Attempting to deliver the DSN would cause an MTA-STS cache lookup, which
would panic because the domain includes a "/" and crash the server.
Because the DSN would be queued, the server would crash again on
restarts, and only self-recover after give_up_send_after (20h by
default).
To fix this, we now explicitly validate the domain when normalizing it,
make MTA-STS cache not panic on invalid domains, and also make the SMTP
courier fail permanently on invalid domains.
Thanks to John JEAN <john@wargan.com> for finding and reporting this
problem. The detailed report can be found in
https://johnjean.io/advisories/chasquid-unauthenticated-remote-dos/.
When running govulncheck, if we use go.mod's version, that is the oldest
Go version that we expect to build chasquid.
However, that specific version is often 1.x.0 and can include
vulnerabilities that are fixed in later patch releases. This can cause
govulncheck to complain, which introduces noise in the CI.
Instead, we should run govulncheck on the latest Go version, to make
sure that any reports are about our code or the dependencies, and not
the toolchain.
That behaviour is in place today, but it's not obvious from the config.
This patch updates the config to note this more explicitly, to prevent
future confusion.
The monitoring http server is mostly read-only but shows sensitive data,
and also has one mutating endpoint, /exit.
This patch updates the routing to use method-based routing, and also
wraps it on CSRF to reject cross-origin requests from browsers.
Note that does not affect direct POST /exit requests via tools like
curl or wget, which continue to work just like today. This is exercised
in the (unchanged) integration tests.
The `fmt` make target does some auto-formatting (gofmt, black) and
vetting (go vet); this patch makes it include go fix too, to help us
keep the codebase updated to modern features and standards.
The tlsconst internal package was created to provide human-readable
names for TLS versions and ciphers, from the IANA registry.
These days, that functionality is provided by stdlib's tls package, so
the internal package is no longer needed, and this patch removes it.
The only observable change is that the descriptive version in the
Received header now says "TLS 1.3" instead of "TLS-1.3", but it's just
as standard-compliant since that is in a free-form text section.
Some integration tests start servers on predictable ports, and then the
servers are killed at exit.
However, we currently don't want for them to die. This can cause races
where the next test starts while the previous daemons are still in the
process of exiting, causing flakiness.
This patch fixes the problem by waiting for the child processes to exit
before exiting the test.
Delivery errors can be visible to the end-user via DSN responses, and
some of them are not very clear, or are missing useful debugging
information (such as the MX).
This patch improves the messages by including the MX where appropriate,
having explicit errors for cases where no servers are found/allowed, and
using less technical language for the common explanations.
There are no changes to the behaviour on errors, this change only
affects the messages.
Many places where we return errors don't use the %w wrapping, because
they predate that functionality.
This patch changes those to use %w, so callers can inspect the
underlying errors with errors.Is and errors.As if needed in the future.
The text of the errors does not change, and no existing callers inspect
these errors, so there are no behaviour changes.
Many of our tests rely on helpers that predate features which are now
available in the standard library.
This patch replaces them with the standard equivalents:
- MustTempDir/RemoveIfOk -> t.ArtifactDir() or t.TempDir() (depending on
whether it is useful to keep the data on request).
- GetFreePort + waitForServer -> opening the listeners before starting
the servers. To support this, localrpc.Server gains a Serve method. We
may re-introduce these in the future if more use cases appear.
- WaitFor and real-time timing checks -> testing/synctest.
- context.Background() -> t.Context().
- Benchmark b.N loops -> b.Loop().
- reflect.DeepEqual -> cmp.Diff, and proto.Equal for protobufs.
This is a follow-up on the `go fix` run in commit
0210c5fd92.
That commit recorded the automated updates.
This one applies further simplifications that are mechanical and do not
involve logic changes, but are outside of what `go fix` did.
It also removes some now-obsolete comments and statements.
This patch contains the result of running `go fix ./...`, which applies
simplifications that make use of modern language features.
There are no logic or behaviour changes.
In commit 3f9d0c0762, we added a new possible private file. If the file
exists but there's an error reading it (common for permission issues),
currently we just move on to the next file, and if it doesn't exist we
may give up with that error.
That can "hide" the access problems making them harder to debug.
To help with those kinds of errors, this patch updates the logic to
return an error if a private key exists but is inaccessible.
It also adds an unit test for this function to cover the different
scenarios, and updates the docs to match.
NixOS uses this path, and it seems generic enough to be worth adding
here.
The `/var/run` paths should probably also be updated to `/run`, but I'm
not going to risk breaking anything :)
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Adjusted comments.
This patch updates the version of the github actions we use in our CI
workflows.
While they still run correctly, they're showing some warnings about
being too out of date in the UI.
In Go commit edf006c9a3fbb23304dd53590e62fef9250ebb94 (released in Go
1.27.0), net/mail was changed to add escaping to some error messages.
In the cmd/chasquid-util/test_dkim.cmy test, we were validating output
against one of those messages, so in Go 1.27.0 the test now fails.
This patch updates the test to match the new string.
Note we don't increase the minimum version yet in go.mod: 1.27 is newly
released, and chasquid can be built fine with 1.25 for now.
We may end up increasing the minimum version later if this discrepancy
causes confusion or test errors.
In Go 1.25.11/1.26.4, net/textproto error formatting was changed, and
now the message is quoted. This is to protect from general string
injections.
See https://go-review.googlesource.com/c/go/+/778182 and
ec1c380418ec6a0da28d4519872e2b81ba9152ba.
After this change, the error strings from net/textproto (and net/smtp
which uses it) now include quotes.
For chasquid, the only user-observable change in the server is that the
errors included in the DSN messages are now quoted, which should not be
a problem.
However, some of the tests which validate error strings need to be
adjusted.
The patch also updates go.mod to set the Go version to match this new
minimum.
This patch removes the unnecessary newlines in the maillog printf calls.
Note that the library will trim them anyway, so this is only for code
readability, but the logs should be unchanged.
In maillog.Auth, we currently have a newline in the log entry args.
The blitiri.com.ar/go/log package handles trailing newlines, but only in
the format parameter, not in the args.
Because of this, the authentication log entries in the maillog currently
end with an extra newline.
This patch fixes the problem by removing the unnecessary newline from
the message.
https://github.com/albertito/chasquid/pull/82
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Adjusted commit message.
This patch adds a Prometheus integration test, that runs a Prometheus
instance which scrapes chasquid, and then does a basic query to ensure
the scraping is working correctly.
The openmetrics format doesn't support empty newlines or miscellaneous
comments, and the Prometheus parser has become more strict over time,
which is now causing scraping errors.
This patch fixes those issues by adjusting to the stricter openmetrics
format.
Thanks to Jakub Ječmínek for reporting this issue!
When running the container, there is a check to make sure we have at
least one certificate. This is very useful to detect a common problem
early, with a friendly error message.
However, when debugging, sometimes we want to skip that check and run
anyways. So this patch introduces a $SKIP_CERT_CHECK variable that can
be used to skip that check.
When Debian releases a new stable version, there could be breaking
changes that mean Docker build stops working.
New Debian stable versions are infrequent enough that it is better to
just do them by hand, to avoid accidentally breaking builds (and
subsequently having to fix them in a rush).
Thanks to Ernesto Alfonso (erjoalgo@github) for reporting this problem
in https://github.com/albertito/chasquid/issues/80.
This patch groups a series of commands that redirect to a file into a
single grouped redirect, for readability reasons.
Suggested by shellcheck SC2129.
In a few places, we call Printf-like functions, but for the format we
use either non-format messages (which is not tidy, but okay), or
variable messages (which can be problematic if they contain %-format
directives).
The patch fixes the calls by either moving to Print-like functions, or
using `Printf("%s", message)` instead.
These were found by a combination of `go vet` (which complains about
"non-constant format string in call"), and manual inspection.
staticcheck found a couple of minor code cleanup improvements, like
unused variables or an out-of-order defer, mostly in tests.
This patch fixes those problems by making the necessary adjustments.
They're all fairly small, and should not change the logic in any
significant way.
There are a couple of places in the tests when we attempt to build and
run simultaneously. Here, if the build is slow, there is a race where
"text file busy" can appear.
To fix this, build to a temporary file with a random name, then
atomically rename it to the final binary name.
When we put something in the queue and respond "250 ok" to the client,
that is taken as accepting the email.
As part of putting something in the queue, we write it to disk, but
today we don't do an fsync on that file.
That leaves a gap where a badly timed crash on some systems could lead
to the file being empty, causing us to lose an email that we accepted.
To elliminate (or drastically reduce on some filesystems) the chances of
that situation, we call fsync on the file that gets written when we put
something in the queue.
Thanks to nolanl@github for reporting this in
https://github.com/albertito/chasquid/issues/78.
The current fail2ban regexp catches all SMTP connection errors.
This works fine, but includes connection errors, that can be caused by
transient external causes, and accidentally delay email delivery.
This patch changes the regexp to be more targeted towards specific SMTP
errors that are likely to be caused by deliberate actions.
The expression was cross-checked with a few month of errors to confirm
it should not have false positives, and that it correctly left
connection errors alone.
Thanks to pepperbob@github for reporting this in
https://github.com/albertito/chasquid/issues/77.
Alberto Bertogli