Today we rely on net/mail.ParseAddress to do most of the validation of
the addresses, and it is quite effective at it. But it does not reject
addresses with control characters, which could end up verbatim in
problematic places (e.g. logs).
Specifically the C1 characters, like U+009B (Control Sequence
Introducer) will pass through and could be problematic, since it is
interpreted by some terminals.
It also allows (quoted) parenthesis, which could confuse Received header
parsers. This is mainly harmless, but it can be confusing when
troubleshooting.
Today, we also reject addresses with more than one @, but as a
side-effect of domain validation.
To reduce the chances of problems later, this patch rejects addresses if
they contain control characters (which is a behaviour change), or more
than one @ (which is pre-existing, but now we fail in a more explicit
way).
It also escapes the addresses containing parenthesis in the Received
header, to make debugging info clearer, and keep the header structure
compliant.
Today we use the HELO/EHLO domain in a variety of places, which are
informational, but some have specific syntax (like the Received header).
Using the un-sanitized values can lead to some confusing and/or invalid
values. This is not very risky today, but it could be problematic in the
future if these strings are used in other contexts.
To prevent those problems, this patch sanitizes the HELO/EHLO domain
when receiving it.
Hooks used to receive the sanitized and raw variants; now, both are
sanitized (the raw one is kept for backwards compatibility).
If the sanitization leaves the string empty, we use the hard-coded
no-valid-characters.invalid as a placeholder, to make this situation
easier to identify when troubleshooting.
We currently use the s= and d= tags as-is without any validation.
They are used to construct the DNS name to look up the key, which is
generally fine, but also the d= tag is used as-is in the
Authentication-Results header.
The latter can be used to inject malicious content, for example:
d=example.com dkim=pass header.d=example.net
can result in the header including:
header.d=example.com dkim=pass header.d=example.net
regardless of the validation status.
To fix that problem, this patch now validates the domain (using the same
normalization we use in other areas, like RCPT TO and MAIL FROM) on both
tags.
If any of the tags have invalid domains, we consider the signature
invalid as per RFC 6376 section 3.5.
Currently we trace everything we are given; sometimes in rare but valid
cases that can be multi-MiB strings. That gets logged and stays in
memory for tracing, which can blow up quite a lot and be a source of
problems and unexpected excessive resource usage.
To mitigate this and reduce the chances of issues, this patch truncates
very long messages to 8k utf8 characters. That's long enough to be
useful for debugging, without running the risk of causing problems on
extremely long messages.
We may adjust this issue in the future if this limit becomes a problem.
Today, trace.Printf and trace.Debugf format the message more than once:
at least once for logging, and once for tracing.
Some of that formatting can be expensive for large messages, causing CPU
and RAM spikes due to transient allocations. So this patch makes those
trace functions only format once, and avoid the logging calls if they're
below the logging level.
Operating on large RSA keys can be computationally expensive. Several
implementations (e.g. crypto/tls, OpenSSL, rspamd) limit their size
to prevent DoS via work amplification.
This patch introduces that limit in our dkim library, set to 8192 bits.
That's the same limit used by other implementations, like crypto/tls
(which limits size of RSA keys in certificates) and rspamd (specifically
for DKIM). It is also in line with the most recent DMARC.org report of
key sizes in the wild (https://dmarc.org/stats/dkim/), where 8192 is
already rare, and the largest one found.
Today, when looking up DKIM keys via DNS, we go through all the TXT
records that are published. This is already more lenient than what's
allowed by many implementations (as documented in the code).
Potentially, someone could craft a domain with a lot of TXT records to
make us waste time processing those keys.
To help reduce that possibility, this patch limits the lookup to 3
records. This is still more than normal and may be limited further to 1
in the future.
If chasquid runs out of file descriptors, that can cause problems
accepting connections, writing to the queue, and even reloading configs.
Some of those errors are fatal, as they should not happen under normal
conditions.
Since a remote client can open a lot of connections, this can end up
consuming all file descriptors, and resulting in a denial of service. To
reduce the impact of that kind of event, this patch makes chasquid limit
the number of concurrent SMTP connections to half the maximum number of
file descriptors.
Connections over that limit will get closed immediately. The server
will log (rate-limited) when such event happens, and new metrics
make these events visible through monitoring too.
This approach complements the previous patch, which made the server
retry Accept errors, to reduce the impact of connection overloads.
This mainly affects deployments with low file descriptor limits; most
modern deployments have very high limits by default, so they are
unlikely to experience this problem in the first place.
Users who have to be very defensive can deploy OS-level safeguards (such
as firewall rules to limit number of connections, etc.) which are more
effective and tunable.
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, if Accept returns an error, we exit the daemon. Unfortunately
this is not that difficult to trigger: a remote client can open a large
number of connections to reach the limit of file descriptors.
This patch makes the server retry instead, with an exponential backoff
(up to 1s).
This is a compromise to improve handling of such scenarios without
introducing a lot of complexity.
The problem can still be triggered but this change makes the daemon more
resilient to it (and it will be improved further in follow-up patches).
Users who have to be very defensive can deploy OS-level safeguards (such
as firewall rules to limit number of connections, etc.) which are more
effective and tunable.
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/.
This patch adds two fuzzing tests to the dkim package:
1) Fuzz the sign-verify cycle, checking we can verify any message we
sign.
2) Fuzz the message we verify, returning valid keys for any domain so it
gets past the key fetching, and exercises a substantial part of the
verification process.
The first test already found two issues, which were fixed in the
previous patches.
Today, we fold header names with strings.EqualFold when signing, but
strings.ToLower when verifying.
This should be harmless for valid messages (that have only ASCII
characters in the header names), but if one contains an (invalid) header
with a non-ASCII character, we could fail to verify the message we sign.
The inconsistency would only happen if a header has a non-ASCII
character that strings.EqualFold and strings.ToLower handle differently
(like "ſubject", with U+017F).
To make signing and verifying consistent, this patch makes signing use
strings.ToLower.
This bug was found by fuzzing the sign-verify cycle, which will be added
in a later patch.
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.
Alberto Bertogli