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 dc7e23fc706b6e48a4bde4b9863c9b04688d42b4 Author: Benoit TELLIER <[email protected]> AuthorDate: Fri Oct 30 08:50:17 2026 +0100 [FIX] DKIMVerifier: keep one result per DKIM-Signature field, in field order The body hash jobs and the failures were kept in maps keyed by the raw field text, so byte-identical fields were merged and getResults() had fewer entries than the message had signatures. A body hash mismatch was also stored under a rebuilt key that could overwrite the failure of another field, and the results came in map order. CompoundBodyHasher now holds one job per field, carrying its body hasher or its failure, and getResults() follows the field order. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../java/org/apache/james/jdkim/DKIMVerifier.java | 99 ++++++++++------------ .../james/jdkim/impl/CompoundBodyHasher.java | 75 ++++++++++------ .../test/java/org/apache/james/jdkim/DKIMTest.java | 25 ++++++ 3 files changed, 122 insertions(+), 77 deletions(-) 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 9585369..fdec5ae 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java @@ -50,14 +50,11 @@ import java.time.Duration; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; -import java.util.HashMap; -import java.util.Hashtable; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Locale; -import java.util.Map; import java.util.Set; public class DKIMVerifier { @@ -258,13 +255,15 @@ public class DKIMVerifier { // For each DKIM-signature we prepare an hashjob. // We calculate all hashes concurrently so to read // the inputstream only once. - Map<String, BodyHasherImpl> bodyHashJobs = new HashMap<>(); - Hashtable<String, FailException> signatureExceptions = new Hashtable<>(); + // One job per field, even for byte-identical fields: each one gets its own result. + List<CompoundBodyHasher.Job> jobs = new ArrayList<>(); int processedSignatures = 0; for (String signatureField : fields) { + CompoundBodyHasher.Job job = new CompoundBodyHasher.Job(signatureField); + jobs.add(job); // RFC 6376 section 6.1: verifiers MAY limit the number of signatures they try to verify. if (processedSignatures >= options.getMaxSignatures()) { - signatureExceptions.put(signatureField, new PermFailException("Too many DKIM-Signature fields: only the first " + job.setFailure(new PermFailException("Too many DKIM-Signature fields: only the first " + options.getMaxSignatures() + " are verified")); continue; } @@ -326,21 +325,21 @@ public class DKIMVerifier { // stream run. BodyHasherImpl bhj = newBodyHasher(signatureRecord); - bodyHashJobs.put(signatureField, bhj); + job.setBodyHasher(bhj); } else { throw new PermFailException( "unexpected bad signature field"); } } catch (TempFailException | PermFailException e) { - signatureExceptions.put(signatureField, e); + job.setFailure(e); } catch (RuntimeException e) { - signatureExceptions.put(signatureField, new PermFailException( + job.setFailure(new PermFailException( "Unexpected exception processing signature", e)); } } - return new CompoundBodyHasher(bodyHashJobs, signatureExceptions); + return new CompoundBodyHasher(jobs); } /** @@ -413,28 +412,28 @@ public class DKIMVerifier { private List<SignatureRecord> verify(CompoundBodyHasher compoundBodyHasher) throws FailException { List<SignatureRecord> verifiedSignatures = new LinkedList<>(); - for (BodyHasherImpl bhj : compoundBodyHasher.getBodyHashJobs().values()) { - byte[] computedHash = bhj.getDigest(); - byte[] expectedBodyHash = bhj.getSignatureRecord().getBodyHash(); - - if (!Arrays.equals(expectedBodyHash, computedHash)) { - compoundBodyHasher.getSignatureExceptions() - .put( - "DKIM-Signature:" + bhj.getSignatureRecord().toString(), - new PermFailException( - "Computed bodyhash is different from the expected one", bhj.getSignatureRecord())); - } else { - verifiedSignatures.add(bhj.getSignatureRecord()); + List<FailException> failures = new ArrayList<>(); + for (CompoundBodyHasher.Job job : compoundBodyHasher.getJobs()) { + BodyHasherImpl bhj = job.getBodyHasher(); + if (bhj != null) { + byte[] computedHash = bhj.getDigest(); + byte[] expectedBodyHash = bhj.getSignatureRecord().getBodyHash(); + + if (!Arrays.equals(expectedBodyHash, computedHash)) { + job.setFailure(new PermFailException( + "Computed bodyhash is different from the expected one", bhj.getSignatureRecord())); + } else { + verifiedSignatures.add(bhj.getSignatureRecord()); + result.add(new Result(bhj.getSignatureRecord())); + continue; + } } + failures.add(job.getFailure()); + result.add(resultFromException(job.getSignatureField(), job.getFailure())); } - for(SignatureRecord s: verifiedSignatures) { - result.add(new Result(s)); - } - result.addAll(resultsFromExceptions(compoundBodyHasher.getSignatureExceptions())); - if (verifiedSignatures.isEmpty()) { - throw prepareException(compoundBodyHasher.getSignatureExceptions()); + throw prepareException(failures); } else { return verifiedSignatures; } @@ -474,45 +473,39 @@ public class DKIMVerifier { result.clear(); } - private List<Result> resultsFromExceptions(Map<String, FailException> exceptions) { - List<Result> results = new ArrayList<>(); - for (Map.Entry<String, FailException> e : exceptions.entrySet()) { - SignatureRecord rec = e.getValue().getRelatedRecord(); - if (rec == null) { - rec = new SignatureRecordTemplate("v=1; d=invalid; h=from; s=invalid; b=invalidsig"); - } + private Result resultFromException(String signatureField, FailException e) { + SignatureRecord rec = e.getRelatedRecord(); + if (rec == null) { + rec = new SignatureRecordTemplate("v=1; d=invalid; h=from; s=invalid; b=invalidsig"); + } - Result.Type resultType = Result.Type.NONE; - if (e.getValue() instanceof TempFailException) { - resultType = Result.Type.TEMPERROR; - } else if (e.getValue() instanceof PermFailException) { - if (e.getValue().getRelatedRecord() == null) { - //FailException without the SignatureRecord - resultType = Result.Type.PERMERROR; - } else { - resultType = Result.Type.FAIL; - } + Result.Type resultType = Result.Type.NONE; + if (e instanceof TempFailException) { + resultType = Result.Type.TEMPERROR; + } else if (e instanceof PermFailException) { + if (e.getRelatedRecord() == null) { + //FailException without the SignatureRecord + resultType = Result.Type.PERMERROR; + } else { + resultType = Result.Type.FAIL; } - results.add(new Result(e.getValue().getMessage(), e.getKey() != null ? e.getKey() : "", rec, resultType)); } - - return results; + return new Result(e.getMessage(), signatureField != null ? signatureField : "", rec, resultType); } /** - * Given a map of exceptions prepares a human readable exception. + * Given a list of exceptions prepares a human readable exception. * This simply return the exception if it is only one, otherwise returns * a cumulative exception * * @param signatureExceptions input exceptions * @return the resulting "compact" exception */ - private FailException prepareException(Map<String, FailException> signatureExceptions) { + private FailException prepareException(List<FailException> signatureExceptions) { if (signatureExceptions.size() == 1) { - return signatureExceptions.values().iterator() - .next(); + return signatureExceptions.get(0); } else { - return new CompositeFailException(signatureExceptions.values(), "found " + signatureExceptions.size() + return new CompositeFailException(signatureExceptions, "found " + signatureExceptions.size() + " invalid signatures"); } } diff --git a/main/src/main/java/org/apache/james/jdkim/impl/CompoundBodyHasher.java b/main/src/main/java/org/apache/james/jdkim/impl/CompoundBodyHasher.java index 31f9fdd..e1fd359 100644 --- a/main/src/main/java/org/apache/james/jdkim/impl/CompoundBodyHasher.java +++ b/main/src/main/java/org/apache/james/jdkim/impl/CompoundBodyHasher.java @@ -20,10 +20,8 @@ package org.apache.james.jdkim.impl; import java.io.OutputStream; -import java.util.Hashtable; import java.util.LinkedList; import java.util.List; -import java.util.Map; import org.apache.james.jdkim.api.BodyHasher; import org.apache.james.jdkim.canon.CompoundOutputStream; @@ -33,27 +31,60 @@ import org.apache.james.jdkim.exceptions.FailException; * CompoundBodyHasher is used for verification purpose. * It contains a compund output stream that will calculate * the body hash for multiple signatures. - * This object is a container for "bodyHashJobs" and - * "signatureExceptions" for 2-stage verification process. + * This object is a container for one {@link Job} per DKIM-Signature field, + * in field order, for 2-stage verification process. */ public class CompoundBodyHasher implements BodyHasher { + /** + * The verification state of one DKIM-Signature field: either a body hasher + * still to be checked, or the failure that ended its verification. + */ + public static class Job { + private final String signatureField; + private BodyHasherImpl bodyHasher; + private FailException failure; + + public Job(String signatureField) { + this.signatureField = signatureField; + } + + public String getSignatureField() { + return signatureField; + } + + public BodyHasherImpl getBodyHasher() { + return bodyHasher; + } + + public void setBodyHasher(BodyHasherImpl bodyHasher) { + this.bodyHasher = bodyHasher; + } + + public FailException getFailure() { + return failure; + } + + public void setFailure(FailException failure) { + this.failure = failure; + this.bodyHasher = null; + } + } + private final OutputStream o; - private final Map<String, BodyHasherImpl> bodyHashJobs; - private final Map<String, FailException> signatureExceptions; - - public CompoundBodyHasher(Map<String, BodyHasherImpl> bodyHashJobs, - Hashtable<String, FailException> signatureExceptions) { - this.bodyHashJobs = bodyHashJobs; - this.signatureExceptions = signatureExceptions; - if (bodyHashJobs.size() == 1) { - o = bodyHashJobs.values().iterator().next() - .getOutputStream(); - } else { - List<OutputStream> outputStreams = new LinkedList<>(); - for (BodyHasherImpl bhj : bodyHashJobs.values()) { - outputStreams.add(bhj.getOutputStream()); + private final List<Job> jobs; + + public CompoundBodyHasher(List<Job> jobs) { + this.jobs = jobs; + List<OutputStream> outputStreams = new LinkedList<>(); + for (Job job : jobs) { + if (job.getBodyHasher() != null) { + outputStreams.add(job.getBodyHasher().getOutputStream()); } + } + if (outputStreams.size() == 1) { + o = outputStreams.get(0); + } else { o = new CompoundOutputStream(outputStreams); } } @@ -61,13 +92,9 @@ public class CompoundBodyHasher implements BodyHasher { public OutputStream getOutputStream() { return o; } - - public Map<String, BodyHasherImpl> getBodyHashJobs() { - return bodyHashJobs; - } - public Map<String, FailException> getSignatureExceptions() { - return signatureExceptions; + public List<Job> getJobs() { + return jobs; } } diff --git a/main/src/test/java/org/apache/james/jdkim/DKIMTest.java b/main/src/test/java/org/apache/james/jdkim/DKIMTest.java index 49c7ee4..3380821 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMTest.java @@ -47,6 +47,7 @@ import org.apache.james.jdkim.api.Headers; import org.apache.james.jdkim.api.Result; import org.apache.james.jdkim.api.SignatureRecord; import org.apache.james.jdkim.api.VerifierOptions; +import org.apache.james.jdkim.exceptions.CompositeFailException; import org.apache.james.jdkim.exceptions.PermFailException; import org.apache.james.jdkim.impl.Message; import org.apache.james.jdkim.tagvalue.SignatureRecordImpl; @@ -387,4 +388,28 @@ public class DKIMTest { .isInstanceOf(MimeException.class); } } + + @Test + public void duplicateSignatureFieldsShouldEachHaveAResult() throws Exception { + String field = "DKIM-Signature: v=1; a=rsa-sha256; d=messiah.edu; s=selector2; h=from; bh=AAAA; b=AAAA\r\n"; + String mail = field + field + "From: [email protected]\r\n\r\nbody\r\n"; + + assertThatThrownBy(() -> verifier.verify(new ByteArrayInputStream(mail.getBytes(StandardCharsets.US_ASCII)))) + .isInstanceOf(CompositeFailException.class); + + assertThat(verifier.getResults()).hasSize(2); + } + + @Test + public void resultsShouldFollowTheFieldOrder() throws Exception { + byte[] mail = "From: [email protected]\r\n\r\nbody\r\n".getBytes(StandardCharsets.US_ASCII); + String valid = dkimSigner.sign(new ByteArrayInputStream(mail)); + String invalid = "DKIM-Signature: v=1; a=rsa-sha256; d=messiah.edu; s=selector3; h=from; bh=AAAA; b=AAAA"; + String signed = invalid + "\r\n" + valid + "\r\n" + invalid + "\r\n"; + + verifier.verify(new SequenceInputStream( + new ByteArrayInputStream(signed.getBytes(StandardCharsets.US_ASCII)), new ByteArrayInputStream(mail))); + + assertThat(verifier.getResults()).extracting(Result::isSuccess).containsExactly(false, true, false); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
