This is an automated email from the ASF dual-hosted git repository. chibenwa pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-jdkim.git
commit 55f875ff875d7d99dba62f90f73cefea3992566a Author: Benoit TELLIER <[email protected]> AuthorDate: Sun Nov 1 19:57:30 2026 +0000 [FIX] DKIMVerifier: check signatures locally before the key lookup newBodyHasher looked up the key right after parsing the signature ("TODO ... before running a network operation like the dns lookup"), and checked a=, c=, b=, bh= and l= only afterwards. A signature broken on its own cost a DNS query, and got a TEMPERROR instead of a permanent error when the lookup failed temporarily. These checks now run before the lookup (RFC 6376 section 6.1.1 before 6.1.2): the a= algorithm, the header and body canonicalization, the base64 of b= and bh=, the hash and l= through the body hasher, which is built there and reused. Such signatures get a PERMERROR whatever the DNS does. The unsupported header canonicalization exception now carries its signature record. --- .../java/org/apache/james/jdkim/DKIMCommon.java | 2 +- .../java/org/apache/james/jdkim/DKIMVerifier.java | 29 +++++++++++----- .../org/apache/james/jdkim/DKIMVerifierTest.java | 40 ++++++++++++++++++++++ 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/main/src/main/java/org/apache/james/jdkim/DKIMCommon.java b/main/src/main/java/org/apache/james/jdkim/DKIMCommon.java index bfc6873..de14849 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMCommon.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMCommon.java @@ -129,7 +129,7 @@ public abstract class DKIMCommon { if (!relaxedHeaders && !SignatureRecord.SIMPLE.equals(sign.getHeaderCanonicalisationMethod())) { throw new PermFailException("Unsupported canonicalization algorythm: " - + sign.getHeaderCanonicalisationMethod()); + + sign.getHeaderCanonicalisationMethod(), sign); } // NOTE: this could be improved by using iterators. diff --git a/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java b/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java index 38f0b8e..b83ff5e 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java @@ -314,6 +314,26 @@ public class DKIMVerifier { } } + // RFC 6376 section 6.1.1 before 6.1.2: what needs no key is checked before the key lookup, + // so that such a signature never costs a DNS query nor gets a TEMPERROR. + byte[] decoded; + BodyHasherImpl bhj; + try { + DKIMCommon.jcaSignatureAlgorithm(signatureRecord); + CharSequence headerCanonicalisation = signatureRecord.getHeaderCanonicalisationMethod(); + if (!SignatureRecord.SIMPLE.equals(headerCanonicalisation) + && !SignatureRecord.RELAXED.equals(headerCanonicalisation)) { + throw new PermFailException("Unsupported canonicalization algorythm: " + + headerCanonicalisation, signatureRecord); + } + decoded = signatureRecord.getSignature(); + signatureRecord.getBodyHash(); + // checks the hash, the body canonicalization and l= + bhj = newBodyHasher(signatureRecord); + } catch (IllegalArgumentException | IllegalStateException e) { + throw new PermFailException("Invalid signature record: " + e.getMessage(), signatureRecord, e); + } + // RFC 8301 section 3.1: rsa-sha1 MUST NOT be used for verifying. if (!options.isRsaSha1Allowed() && SignatureRecordImpl.checkAlgorithm(signatureRecord) == HashMethod.SHA1) { @@ -329,18 +349,11 @@ public class DKIMVerifier { } } - // TODO here we could check more parameters for - // validation before running a network operation like the - // dns lookup. - // e.g: the canonicalization method could be checked now. PublicKeyRecord publicKeyRecord = publicRecordLookup(signatureRecord); List<CharSequence> signedHeadersList = signatureRecord.getHeaders(); try { - // decoded now so that verify(CompoundBodyHasher) cannot fail on it - signatureRecord.getBodyHash(); - byte[] decoded = signatureRecord.getSignature(); signatureVerify(withoutSignature(messageHeaders, signatureIndex), signatureRecord, decoded, publicKeyRecord, signedHeadersList, signatureField.substring(0, pos)); } catch (IllegalArgumentException e) { @@ -350,8 +363,6 @@ public class DKIMVerifier { // we track all canonicalizations+limit+bodyHash we // see so to be able to check all of them in a single // stream run. - BodyHasherImpl bhj = newBodyHasher(signatureRecord); - job.setBodyHasher(bhj); } else { diff --git a/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java b/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java index 5bde0f5..a8a73d6 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java @@ -275,4 +275,44 @@ public class DKIMVerifierTest { assertThat(verifier.verify(new ByteArrayInputStream(mail.toString().getBytes(StandardCharsets.US_ASCII)))).hasSize(1); } + + @Test + public void signaturesBrokenOnTheirOwnShouldBeRejectedWithoutKeyLookup() { + AtomicInteger calls = new AtomicInteger(); + PublicKeyRecordRetriever retriever = (method, selector, token) -> { + calls.incrementAndGet(); + throw new TempFailException("DNS timeout"); + }; + String valid = "bh=frcCV1k9oG9oKj3dpUqdJg1PxRT2RSN/XKdLCPjaYaY=; b=AAAA"; + String mail = + "DKIM-Signature: v=1; a=rsa-sha256; c=foo; d=example.com; s=s0; h=from; " + valid + "\r\n" + + "DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/foo; d=example.com; s=s1; h=from; " + valid + "\r\n" + + "DKIM-Signature: v=1; a=rsa-sha999; d=example.com; s=s2; h=from; " + valid + "\r\n" + + "DKIM-Signature: v=1; a=rsa-sha256; d=example.com; s=s3; h=from; bh=frcCV1k9oG9oKj3dpUqdJg1PxRT2RSN/XKdLCPjaYaY=; b=A\r\n" + + "DKIM-Signature: v=1; a=rsa-sha256; d=example.com; s=s4; h=from; bh=A; b=AAAA\r\n" + + "DKIM-Signature: v=1; a=rsa-sha256; d=example.com; s=s5; h=from; l=x; " + valid + "\r\n" + + "From: [email protected]\r\n\r\nbody\r\n"; + DKIMVerifier verifier = new DKIMVerifier(retriever); + + assertThatThrownBy(() -> verifier.verify(new ByteArrayInputStream(mail.getBytes(StandardCharsets.US_ASCII)))) + .isInstanceOf(FailException.class); + assertThat(calls.get()).isZero(); + assertThat(verifier.getResults()).hasSize(6) + .allMatch(r -> r.getResultType() == Result.Type.PERMERROR); + } + + @Test + public void wellFormedSignatureShouldStillGetATemperrorWhenTheLookupFails() { + PublicKeyRecordRetriever retriever = (method, selector, token) -> { + throw new TempFailException("DNS timeout"); + }; + String mail = "DKIM-Signature: v=1; a=rsa-sha256; d=example.com; s=s0; h=from; " + + "bh=frcCV1k9oG9oKj3dpUqdJg1PxRT2RSN/XKdLCPjaYaY=; b=AAAA\r\n" + + "From: [email protected]\r\n\r\nbody\r\n"; + DKIMVerifier verifier = new DKIMVerifier(retriever); + + assertThatThrownBy(() -> verifier.verify(new ByteArrayInputStream(mail.getBytes(StandardCharsets.US_ASCII)))) + .isInstanceOf(TempFailException.class); + assertThat(verifier.getResults()).extracting(Result::getResultType).containsExactly(Result.Type.TEMPERROR); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
