When verifying a signature we need to strip the value of the b= tag.
Today, we do this with a regexp that can accidentally match other tag
values. For example, "z=Subject:club=20".
This can cause valid emails to fail verification. But it can't cause an
invalid email to pass verification, so it's not a significant security
concern.
To fix this, this patch updates the regular expression to match the tag
more precisely. It also adds a few tests to validate the behaviour is
correct, which were cross-checked with other implementations.
Thanks to John JEAN <john@wargan.com> for reporting this bug.
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.
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.
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.
Alberto Bertogli