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/.
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 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.
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.
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.
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.
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!
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.
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.
When constructing the "Received" header, in some cases we want to
include the remote IP address in addition to the EHLO domain.
The way we did that is not fully compliant with RFC 5321 (section 4.4),
and this has the potential to confuse some tools that parse the header.
This patch fixes this problem by adjusting the order of the two pieces
of data, which makes it comply with the RFC.
Before:
Received: from [1.2.3.4] (ehlo.domain.example.com)
After:
Received: from ehlo.domain.example.com ([1.2.3.4])
Thanks to nolanl@github for reporting this problem in
https://github.com/albertito/chasquid/issues/76.
On NixOS, `/bin` is basically empty and this causes the courier tests
(which invoke `/bin/sleep`) to fail.
This patch fixes the tests by removing the hardcoded path.
https://github.com/albertito/chasquid/pull/73
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Adjusted commit message.
Today, the maximum number of items in the queue, as well as how long we
keep attempting to send each item, is hard-coded and not changed by end
users.
While they are totally adequate for chasquid's main use cases, it can
still be useful for some users to change them.
So this patch adds two new configuration options for those settings.
They're marked experimental for now, so we can adjust them if needed
after they get more exposure.
Thanks to Lewis Ross-Jones <lewis_r_j@hotmail.com> for suggesting this
improvement, and help with testing it.
This patch implements "via" aliases, which let us explicitly select a
server to use for delivery.
This feature is useful in different scenarios, such as a secondary MX
server that forwards all incoming email to a primary.
For now, it is experimental and the syntax and semantics are subject to
change.
We have a few Python scripts which over the years ended up with a
variety of formatting.
This patch auto-formats them using `black -l 79` to make them more
uniform, and easier to read and write.
In the tests, we create a lot of Recipient{}s, and that ends up being
very verbose and sometimes cumbersome.
Also, if we ever want to extend it, it would result in a lot of
unnecessary refactoring.
So this patch replaces the Recipient{} instantiations with helper
functions, to help readability and extendability.
This only affects the tests, and there are no changes to them either, it
is purely a refactor.
Today, aliases parsing is too lax, silently ignoring most kinds of invalid
lines.
That behaviour can cause a lot of confusion when users think the aliases
are being parsed, and also cause problems when extending the syntax.
This patch fixes that problem by making aliases parsing return errors on
the invalid lines.
Unfortunately this will cause some previously-accepted files to now be
rejected, but it should be quite visible.
This patch implements support for aliases that contain '*' as the
destination user.
In that case, we replace it with the original user.
For example, `*: *@pond` will redirect `lilly@domain` to `lilly@pond`.
This is experimental for now, and marked as such in the documentation.
The semantics can be subtle, so we may need to adjust them later.
This patch fixes some minor typos in comments and strings found by
codespell.
While at it, also expand some variable names that were not typos, but
caused false positives, and end up being more readable anyway.
Microsoft SMTP servers have a bug that prevents them from successfully
establishing a TLS connection against modern Go TLS servers, and some
OpenSSL versions. It also doesn't fall back to plain-text, so this has
been causing deliverablity issues.
The problem started by the end of 2024 and it's still not fixed.
Unfortunately, because they're quite a big provider and are not fixing
their problem, it is worth to do a server-side workaround.
This patch implements that workaround: it disables TLS session tickets.
There is no security impact for doing so, and there is a small
performance penalty which is likely to be insignificant for chasquid's
main use cases.
This workaround should be removed once Microsoft fixes their problem.
We are going to make a 1.15.1 release for this, which this patch also
documents.
Thanks to Michael (l6d-dev@github) for reporting this issue and
suggesting this workaround!
See https://github.com/albertito/chasquid/issues/64 and
https://github.com/golang/go/issues/70232 for more details.
This commit updates the uses of math/rand to math/rand/v2, which was
released in Go 1.22 (2024-02).
The new package is generally safer, see https://go.dev/blog/randv2 for
the details.
There are no user-visible changes, it is only adjusting the name of
functions, simplify some code thanks to v2 having a better API, etc.
This patch regenerates the auto-generated files. There are no
significant changes.
- Protobuf files updated the comment formatting to match recent changes
in Go libraries.
- IANA assignment for a AEGIS (currently an IETF draft) has been
updated.
- The link to the human-readable IANA assignment tables from the
generator was manually updated.
The ToCRLF/StringToCRLF functions are not very performance critical, but
we call it for each mail, and the current implementation is very
inefficient (mainly because it goes one byte at a time).
This patch replaces it with a better implementation that goes line by line.
The new implementation of ToCRLF is ~40% faster, and StringToCRLF is ~60%
faster.
```
$ benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: blitiri.com.ar/go/chasquid/internal/normalize
cpu: 13th Gen Intel(R) Core(TM) i9-13900T
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
ToCRLF-32 162.96µ ± 6% 95.42µ ± 12% -41.44% (p=0.000 n=10)
StringToCRLF-32 190.70µ ± 14% 76.51µ ± 6% -59.88% (p=0.000 n=10)
geomean 176.3µ 85.44µ -51.53%
```
The timestamp string in the t= and x= headers is an "unsigned decimal
integer", but time.Unix takes an int64. Today we parse it as uint64 and
then cast it, but this can cause issues with overflow and type
conversion.
This patch fixes that by parsing the timestamps as signed integers, and
then checking they're positive.
This patch makes chasquid log how many users, aliases and DKIM keys were
loaded for each domain.
This makes it easier to confirm changes, and troubleshoot problems
related to these per-domain configuration files.
Today, when starting up, if there's an error reading the users or
aliases files, we only log but do not exit. And then those files will
not be attempted to be read on the periodic reload.
We also treat "file does not exist" as an error for users file, but not
aliases file, resulting in inconsistent behaviour between the two.
All of this makes some classes of problems (like permission errors) more
difficult to spot and troubleshoot. For example,
https://github.com/albertito/chasquid/issues/55.
So this patch makes errors reading users/aliases files on startup a
fatal error, and also unifies the "file does not exist" behaviour to
make it not an error in both cases.
Note that the behaviour on the periodic reload is unchanged: treat these
errors as fatal too. This may be changed in future patches.
The current .gitignore pattern doesn't work when the private test cases
are a symlink, which can be convenient.
This patch fixes it by changing the pattern to match symlinks as well as
directories.
This patch adds tests to verify how safeio behaves when *os.File
operations return various errors.
To do this, we allow the possibility of wrapping os.CreateTemp, so we
can simulate the errors during testing.
This is not pretty, but the code is small enough that the readability
overhead is minimal, and we get a lot of tests from it.
The RFCs are very clear that in DATA contents:
> CR and LF MUST only occur together as CRLF; they MUST NOT appear
> independently in the body.
https://www.rfc-editor.org/rfc/rfc5322#section-2.3https://www.rfc-editor.org/rfc/rfc5321#section-2.3.8
Allowing "independent" CR and LF can cause a number of problems.
In particular, there is a new "SMTP smuggling attack" published recently
that involves the server incorrectly parsing the end of DATA marker
`\r\n.\r\n`, which an attacker can exploit to impersonate a server when
email is transmitted server-to-server.
https://www.postfix.org/smtp-smuggling.htmlhttps://sec-consult.com/blog/detail/smtp-smuggling-spoofing-e-mails-worldwide/
Currently, chasquid is vulnerable to this attack, because Go's standard
libraries net/textproto and net/mail do not enforce CRLF strictly.
This patch fixes the problem by introducing a new "dot reader" function
that strictly enforces CRLF when reading dot-terminated data, used in
the DATA input processing.
When an invalid newline terminator is found, the connection is aborted
immediately because we cannot safely recover from that state.
We still keep the internal representation as LF-terminated for
convenience and simplicity.
However, the MDA courier is changed to pass CRLF-terminated lines, since
that is an external program which could be strict when receiving email
messages.
See https://github.com/albertito/chasquid/issues/47 for more details and
discussion.
Some use cases, like receive-only MTAs, need domain users for receiving
emails, but have no real need for passwords since they will never use
submission.
Today, that is not supported, and those use-cases require the
administrator to come up with a password unnecessarily, adding
complexity and possibly risk.
This patch implements "receive-only users", which don't have a valid
password, thus exist for the purposes of delivering mail, but always
fail authentication.
See https://github.com/albertito/chasquid/issues/44 for more details and
rationale.
Thanks to xavierg who suggested this feature on IRC.
When logging the configuration, we currently don't quote the string
values, which can make whitespace-induced problems difficult to identify
and troubleshoot.
This patch changes the formatting to always quote string values when
logging the configuration.
This patch adds the embedded aliases file to the fuzz corpus, because it
is trivial to do so, and is a reasonable seed which will be naturally
adjusted over time as the package evolves (as it happened in recent
commits).
Alberto Bertogli