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.
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.
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.
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 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.
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.
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.
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.
Alberto Bertogli