The SMTP smuggling vulnerability fixed in 1.13 (and 1.11.1) has been
given a CVE number: CVE-2023-52354
(https://nvd.nist.gov/vuln/detail/CVE-2023-52354).
This patch adds a link to it in the release notes, for ease of reference.
This prevents chasquid from attempting to look for certs under a
non-directory, e.g. `/etc/chasquid/domains/.gitignore/certs`.
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Adjusted commit message, applied `go fmt`.
chasquid v1.11.1 was released on 2023-12-26 with a backport of the
security fixes from 1.13.
This was requested by users of Debian stable, who are on 1.11.
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.
This patch makes mail_diff more strict in its parsing, to ensure we
catch any encoding issues that may otherwise be masked by the default
compatibility policy.
The minor dialogs test covers some very specific SMTP exchanges, and
some of those include delivering email.
Today, we don't verify the final mailbox, we just check the SMTP
exchange. However, it can be very useful for some of the tests to do
end-to-end checking of the final mailbox.
This patch implements that ability in the test itself, and on the
(currently only) email delivering dialog.
Subsequent patches that introduce new tests will make use of this
feature.
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.
Using an empty listening address will result in chasquid listening on a
random port, which is a dangerous misconfiguration.
That is most likely done to prevent it from listening at all.
To prevent this misconfiguration, explicitly reject empty listening
addresses early and with a warning, so there is no ambiguity.
Users can still prevent chasquid from listening by just commenting out
the entry in the config (and not passing any systemd file descriptors).
See https://github.com/albertito/chasquid/issues/45 for more details and
discussion, including alternatives considered.
Thanks to xavierg who reported this via 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.
When a single dovecot user exists and their password is being updated via
docker/add-user.sh, the `grep -v` command intended to remove the user's
old password will not match any lines and exit with error code 1, causing
the entire script to fail.
This patch fixes it by replacing the if-grep logic with a simpler sed
invocation.
https://github.com/albertito/chasquid/pull/43
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Minor edits to the commit message.
This patch extends docker/add-user.sh to support getting the email and
password from environment variables.
That way, docker/add-user.sh can be used in scripts.
https://github.com/albertito/chasquid/pull/43
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Minor edits to the commit message.
There is a bug causing an invalid config in the generated
/etc/dovecot/auto-ssl.conf, e.g.:
ssl_cert = </etc/letsencrypt/live//etc/letsencrypt/live/mail.grouplok.com/fullchain.pem
ssl_key = </etc/letsencrypt/live//etc/letsencrypt/live/mail.grouplok.com/privkey.pem
This patch fixes it by using the domain name instead of the path, which
matches the original intent.
https://github.com/albertito/chasquid/pull/42
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Minor edits to the commit message.
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).
The testing package does not allow t.Fatal to be called from a different
goroutine; however, we do that if the testing server fails listen or
accept connections.
Since that is unexpected and rare, this patch turns those calls into
panics.
Today, when a user sets an alias with drop characters and/or suffixes,
those go unused, since we always "clean" addresses before alias
resolution.
This results in unexpected and surprising behaviour, and it's not
properly documented either.
This patch resolves this unexpected behaviour as follows:
- Drop characters are ignored, both at parsing time and at lookup time.
- Lookups are done including the suffixes first, and if that results in
no matches, they are retried without suffixes.
This results in aliases working more intuitively for the most common use
cases: of users wanting to have different aliases for specific suffixes,
and not having to care for drop characters.
Hooks can be used to get different behaviour if needed, since the first
lookup is done with the address as-is.
Thanks to znerol@ (lo+github@znerol.ch) for reporting this, and the
discussion on how to fix it, in
https://github.com/albertito/chasquid/issues/41.
Today, the parsing functions are stand-alone since they don't need
anything from the resolver.
But in future patches, that will change.
In anticipation of that, move those functions to be methods of the
resolver.
The aliases.Resolver.Exists function currently returns the "clean"
address (with the drop characters and suffixes removed), which is relied
upon in its only caller.
That, however, makes the logic more difficult to follow, hiding some
of the address manipulation behind what should be a read-only check.
So this patch reorganizes that code a little bit, removing the
"cleaning" of the address as part of Exists, and making it explicit when
needed instead.
This patch does not have any user-visible change in behaviour, it is
just internal reorganization.
This is in preparation for further patches which will improve the
handling of some aliases corner cases.
Since commit e6c6df45, `chasquid-util alias-resolve` talks to the server
and returns authoritative answers.
This patch updates docs/aliases.md to reflect that, which was missed in
the original set of changes.
Today we check the aliases deliver mail to the expected locations, but
we don't fail if there are unexpected deliveries.
Doing so can help catch bugs (including test bugs), so this patch
implements that.
In addition, fix two of the tests that were printing on error, but not
causing the tests to fail (which was the original intention).
This patch moves the top-level certificate loading logic out of main and
into a separate function.
This is only for readability and consistency with how we handle domains
(which have a similar structure already). There are no logic changes.
When starting up, for each domain we parse aliases and users files (if
they exist).
Today we print a line for each, which gets quite verbose and doesn't
offer much useful information.
This patch adjusts that logic so that we only print errors (unless a
file does not exist, which is a normal case).
This also improves the scenario where chasquid does not have permissions
to access the users file: before this patch, that would fail silently,
but with this patch we show the correct error.
While at it, make the "Loading" messages consistent with each other, for
readability.
The t-20-bad_configs test can sometimes have false positives because
the last line recorded in the log isn't always the one causing the
failure, due to asynchronous logging and in particular in combination
with coverage tests (which alter the os.Exit behaviour subtly).
This patch fixes that by having the tests look at the last 4 lines of
output instead. This is not super pretty, but it should be good enough
to cover for any timing issues that arise in these particular tests.
Currently, if the `certs/` directory has a symlink inside, we skip it.
That is not really intended, it's an unfortunate side-effect of skipping
regular files.
To fix this, this patch adjusts the logic to only ignore regular files
instead. It also adds a message when a directory is skipped, to make it
easier to debug permission issues.
Thanks to @erjoalgo for reporting this in
https://github.com/albertito/chasquid/pull/39, and providing an
alternative patch!
The aliases-add subcommand was added before aliases hooks were
implemented and polished, it is undocumented, and the implementation is
nowadays a bit brittle, has some rough edges, and adds significant code
complexity to chasquid-util.
AFAIK nobody is using it either (checked with some specific folks
directly, and it's not very discoverable either).
For all those reasons, this patch removes it.
This patch makes chasquid-util's aliases-resolve and domaininfo-remove
commands talk to the chasquid server (via the new localrpc server).
For aliases-resolve, currently has fairly hacky logic which reimplements
a bunch of the servers', and is also incomplete because it does not
support hooks.
In this patch we fix that by having it talk to the server, where we get
authoritative responses and have no issues with aliases hooks. This
resolves https://github.com/albertito/chasquid/issues/18.
For domaininfo-remove, currently its implementation is also very hacky
since it manipulates files behind the servers' back and without even
using the internal library.
In this patch we fix that by doing the operation through the server,
avoiding the need for those hacks, and also remove the need to manually
reload the server afterwards.
This patch makes chasquid run a localrpc server, exporting two methods:
alias resolve, and domaininfo clear.
They will be used by chasquid-util in later patches.
This patch adds a new package for doing local lightweight RPC calls over UNIX
sockets. This will be used in later patches for communication between chasquid
and chasquid-util.
This patch adds a Clear method to the domaininfo database, which removes
information for the given domain.
This can be used to manually make the server forget about a domain, in
case there are operational reasons to do so.
Today, this is done via chasquid-util (which removes the backing file),
but that is hacky, and this is part of replacing it with a cleaner
implementation.
Some systems, like GitHub, can use `docs/SECURITY.md` to inform users on
how to report security issues.
This patch adds one to the tree. It is not linked explicitly in mkdocs
because the same information is already covered in the doc index and
README already.
It is not expected that the user modifies the domaininfo database behind
chasquid's back, and reloading it can be somewhat expensive.
So this patch removes the periodic reload, and instead makes it triggered
by SIGHUP so the user can trigger a reload manually if needed.
Some MTAs reject client connections unless the local name (used in the
HELO/EHLO command) looks like an FQDN. Currently, smtp-check always uses
`localhost`, which does not look like an FQDN.
This patch adds a command line flag to smtp-check to specify the
local name to be used.
Fixes https://github.com/albertito/chasquid/issues/37.
Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
Minor edits to the commit message, adjust flag name, go fmt.
The current badge, which covers all the branch checks doesn't work, it
always says "pending".
This patch replaces it with the gotest workflow status. It is much more
narrow, but at least it works and gives a decent indication of the
testing status.
This patch updates tests.md to reflect the recent changes around
coverage testing, specifically we no longer have coverage issues with
fatal/non-zero exits, so remove that section from the docs.
Also while at it, add links to the software we reference, for
convenience.
This patch regenerates the auto-generated files.
There are no significant changes, the protobuf just get an updated
comment due to protoc version change, but it is just informational.
Two new TLS ciphers are added, matching the new IANA assignments.
When the SMTP courier gets an error on STARTTLS (either because the
command itself failed, or because there was a low-level TLS negotiation
error), today we just fail that attempt.
This can cause messages to never be delivered if the underlying reason
is a server misconfiguration (e.g. a server certificate that Go cannot
parse). This is quite rare in practice, but it can happen.
To prevent this situation, this patch adds logic so that the SMTP
courier retries over plaintext when STARTTLS fails.
This is still subject to security level checks, so this type of failures
cannot be used to downgrade connections to domains we successfully
established a TLS connection previously.
Note that certificate validation issues are NOT included in this
type of failure, so they will not trigger a retry. The certificate
validation handling is unchanged by this patch.
