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 327ad8d29c83deaa3cc448810a6fd89947aa9532 Author: Benoit TELLIER <[email protected]> AuthorDate: Sun Nov 1 19:47:32 2026 +0000 [FIX] DKIM-Signature: accept only the RFC 6376 a= algorithms The JCA algorithm names were built from a=: any hash the JDK knows was accepted, so a=rsa-md5 and a=rsa-md2 signatures verified, as did a=rsa-sha512/256, a=rsa-sha384 or a=rsa-SHA256, which RFC 6376 section 3.3 does not define (tag values are case-sensitive, section 3.2). SignatureRecordImpl and SignatureRecordTemplate now reject any a= that is not exactly rsa-sha256 or rsa-sha1, and the signer, the verifier and the body hasher derive the JCA names from a fixed map. The verifier reports such signatures as permerror; the signer refuses to sign. --- .../java/org/apache/james/jdkim/DKIMCommon.java | 18 +++++++++ .../java/org/apache/james/jdkim/DKIMSigner.java | 4 +- .../java/org/apache/james/jdkim/DKIMVerifier.java | 4 +- .../apache/james/jdkim/impl/BodyHasherImpl.java | 5 ++- .../james/jdkim/tagvalue/SignatureRecordImpl.java | 44 ++++++++++++++++++++++ .../jdkim/tagvalue/SignatureRecordTemplate.java | 3 ++ .../test/java/org/apache/james/jdkim/DKIMTest.java | 19 ++++++++++ .../org/apache/james/jdkim/DKIMVerifierTest.java | 2 +- .../james/jdkim/SignatureRecordImplTest.java | 20 +++++++--- .../james/jdkim/SignatureRecordTemplateTest.java | 21 ++++++++--- 10 files changed, 121 insertions(+), 19 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 c5315d7..bfc6873 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMCommon.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMCommon.java @@ -19,9 +19,12 @@ package org.apache.james.jdkim; +import org.apache.james.jdkim.api.HashMethod; import org.apache.james.jdkim.api.Headers; import org.apache.james.jdkim.api.SignatureRecord; import org.apache.james.jdkim.exceptions.PermFailException; +import org.apache.james.jdkim.tagvalue.SignatureRecordImpl; +import org.apache.james.jdkim.api.SigningAlgorithm; import java.io.IOException; import java.io.InputStream; @@ -38,6 +41,21 @@ public abstract class DKIMCommon { private static final boolean DEEP_DEBUG = false; + /** + * The JCA signature algorithm of a signature record, from a fixed map rather than from the a= value: + * only the RFC 6376 section 3.3 algorithms are accepted. + * + * @throws PermFailException for any other a= value + */ + protected static String jcaSignatureAlgorithm(SignatureRecord sign) throws PermFailException { + try { + HashMethod hashMethod = SignatureRecordImpl.checkAlgorithm(sign); + return SigningAlgorithm.RSA.asJdkSignatureAlgorithm(hashMethod); + } catch (IllegalStateException e) { + throw new PermFailException(e.getMessage(), sign, e); + } + } + /** * Header fields are carried as Strings where each char is one raw octet * (see {@link Headers}): ISO-8859-1 gives them back unchanged. diff --git a/main/src/main/java/org/apache/james/jdkim/DKIMSigner.java b/main/src/main/java/org/apache/james/jdkim/DKIMSigner.java index d539fdf..a77296f 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMSigner.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMSigner.java @@ -137,9 +137,7 @@ public class DKIMSigner { throws NoSuchAlgorithmException, InvalidKeyException, SignatureException, PermFailException { - Signature signature = Signature.getInstance(sign.getHashMethod() - .toString().toUpperCase(Locale.US) - + "with" + sign.getHashKeyType().toString().toUpperCase(Locale.US)); + Signature signature = Signature.getInstance(DKIMCommon.jcaSignatureAlgorithm(sign)); signature.initSign(key); signatureCheck(h, sign, headers, signature); 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 1d4c85f..62a2782 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java @@ -598,9 +598,7 @@ public class DKIMVerifier { String signatureFieldName) throws PermFailException { try { - Signature signature = Signature.getInstance(sign.getHashMethod() - .toString().toUpperCase(Locale.US) - + "with" + sign.getHashKeyType().toString().toUpperCase(Locale.US)); + Signature signature = Signature.getInstance(DKIMCommon.jcaSignatureAlgorithm(sign)); PublicKey publicKey; try { publicKey = key.getPublicKey(); diff --git a/main/src/main/java/org/apache/james/jdkim/impl/BodyHasherImpl.java b/main/src/main/java/org/apache/james/jdkim/impl/BodyHasherImpl.java index 713434f..b058fad 100644 --- a/main/src/main/java/org/apache/james/jdkim/impl/BodyHasherImpl.java +++ b/main/src/main/java/org/apache/james/jdkim/impl/BodyHasherImpl.java @@ -31,6 +31,7 @@ import org.apache.james.jdkim.canon.LimitedOutputStream; import org.apache.james.jdkim.canon.RelaxedBodyCanonicalizer; import org.apache.james.jdkim.canon.SimpleBodyCanonicalizer; import org.apache.james.jdkim.exceptions.PermFailException; +import org.apache.james.jdkim.tagvalue.SignatureRecordImpl; public class BodyHasherImpl implements BodyHasher { @@ -42,7 +43,9 @@ public class BodyHasherImpl implements BodyHasher { public BodyHasherImpl(SignatureRecord sign) throws PermFailException { MessageDigest md; try { - md = MessageDigest.getInstance(sign.getHashAlgo().toString()); + md = MessageDigest.getInstance(SignatureRecordImpl.checkAlgorithm(sign).asMessageDigestAlgorithm()); + } catch (IllegalStateException e) { + throw new PermFailException(e.getMessage(), sign, e); } catch (NoSuchAlgorithmException e) { throw new PermFailException("Unsupported algorythm: " + sign.getHashAlgo(), sign, e); diff --git a/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordImpl.java b/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordImpl.java index d72e4d0..e8806ee 100644 --- a/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordImpl.java +++ b/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordImpl.java @@ -21,15 +21,57 @@ package org.apache.james.jdkim.tagvalue; import static org.apache.james.jdkim.parser.DKIMQuotedPrintable.dkimQuotedPrintableDecode; +import org.apache.james.jdkim.api.HashMethod; import org.apache.james.jdkim.api.SignatureRecord; import java.util.Base64; +import java.util.Collections; +import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.regex.Pattern; public class SignatureRecordImpl extends TagValue implements SignatureRecord { + /** + * RFC 6376 section 3.3: the a= values, case-sensitive (section 3.2), and their hash method. + */ + private static final Map<String, HashMethod> ALGORITHMS; + + static { + Map<String, HashMethod> algorithms = new LinkedHashMap<>(); + algorithms.put("rsa-sha256", HashMethod.SHA256); + algorithms.put("rsa-sha1", HashMethod.SHA1); + ALGORITHMS = Collections.unmodifiableMap(algorithms); + } + + /** + * Checks an a= value against the algorithms of RFC 6376 section 3.3. + * + * @param algorithm the a= value + * @return its hash method + * @throws IllegalStateException if it is not rsa-sha256 or rsa-sha1, exactly + */ + public static HashMethod checkAlgorithm(CharSequence algorithm) { + HashMethod hashMethod = algorithm == null ? null : ALGORITHMS.get(algorithm.toString()); + if (hashMethod == null) { + throw new IllegalStateException("Unsupported signature algorithm (a=): " + algorithm + + ", expected one of " + ALGORITHMS.keySet()); + } + return hashMethod; + } + + /** + * Checks the a= value of a signature record, see {@link #checkAlgorithm(CharSequence)}. + * + * @param sign a signature record + * @return its hash method + */ + public static HashMethod checkAlgorithm(SignatureRecord sign) { + return checkAlgorithm(sign.getHashKeyType() + "-" + sign.getHashMethod()); + } + // TODO ftext is defined as a sequence of at least one in %d33-57 or // %d59-126 private static final Pattern hdrNamePattern = Pattern.compile("^[^: \r\n\t]+$"); @@ -69,6 +111,8 @@ public class SignatureRecordImpl extends TagValue implements SignatureRecord { if (getValue("h").length() == 0) throw new IllegalStateException("Tag h= cannot be empty."); + checkAlgorithm(getValue("a")); + CharSequence identity; try { identity = getIdentity(); diff --git a/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordTemplate.java b/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordTemplate.java index 96438db..017c5ba 100644 --- a/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordTemplate.java +++ b/main/src/main/java/org/apache/james/jdkim/tagvalue/SignatureRecordTemplate.java @@ -68,6 +68,9 @@ public class SignatureRecordTemplate extends TagValue implements SignatureRecord */ public void validate() throws IllegalStateException { super.validate(); + if (getValue("a") != null) { + SignatureRecordImpl.checkAlgorithm(getValue("a")); + } // TODO: what about v=0.5 and no v= at all? // do specs allow parsing? what should we check? if (!"1".equals(getValue("v"))) 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 a1fc67b..c4564da 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMTest.java @@ -662,4 +662,23 @@ public class DKIMTest { .isInstanceOf(PermFailException.class); assertThat(verifier.getResults()).extracting(Result::getResultType).containsExactly(Result.Type.FAIL); } + + @Test + public void signerShouldRejectAlgorithmsOutsideRfc6376() { + assertThatThrownBy(() -> new DKIMSigner("v=1; a=rsa-md5; c=simple; d=messiah.edu; h=from:subject; q=dns/txt; s=selector2;", TestKeys.privateKey) + .sign(new ByteArrayInputStream(SINGLE_FROM_MESSAGE.getBytes(StandardCharsets.US_ASCII)))) + .hasMessageContaining("algorithm"); + } + + @Test + public void verifierShouldReportAlgorithmsOutsideRfc6376AsPermerror() { + String mail = "DKIM-Signature: v=1; a=rsa-md5; c=simple; d=messiah.edu; s=selector2; h=from:subject; bh=AAAA; b=AAAA\r\n" + + SINGLE_FROM_MESSAGE; + + assertThatThrownBy(() -> verifier.verify(new ByteArrayInputStream(mail.getBytes(StandardCharsets.US_ASCII)))) + .isInstanceOf(PermFailException.class) + .hasMessageContaining("algorithm"); + assertThat(verifier.getResults()).extracting(Result::getResultType).containsExactly(Result.Type.PERMERROR); + } + } 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 ba5651a..a2b9468 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java @@ -63,7 +63,7 @@ public class DKIMVerifierTest { new SignatureRecordImpl( "v=1; a=dsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:date:message-id:subject:from:to:content-type; bh=AbPsrGRdyVjM2e5ZZdhQA/aLe305f2bPvPRohUxrGjo=; b=ksNsfQJv20M9/Vf66sMJT1WHM/fUfcqli1NfkyxSOjr8jlNTH4JNCGacb2neWuwMN4C4TFXqMR8BENkn+XrCV1FjrlW1mCxlLDilVypP/uqqq04KzJpVyJG6zZLd/0DeknSLN6sDGKdCvIdS+YpHEhUxoEuf6QizCs8PTXhnJiA=")); fail("This is not a signature for that key"); - } catch (PermFailException e) { + } catch (PermFailException | IllegalStateException e) { } pkr = new PublicKeyRecordImpl( "k=rsa; t=y:s; p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDIhyR3oItOy22ZOaBrIVe9m/iME3RqOJeasANSpg2YTHTYV+Xtp4xwf5gTjCmHQEMOs0qYu0FYiNQPQogJ2t0Mfx9zNu06rfRBDjiIU9tpx2T+NGlWZ8qhbiLo5By8apJavLyqTLavyPSrvsx0B3YzC63T4Age2CDqZYA+OwSMWQIDAQAB"); diff --git a/main/src/test/java/org/apache/james/jdkim/SignatureRecordImplTest.java b/main/src/test/java/org/apache/james/jdkim/SignatureRecordImplTest.java index e9212a9..656dab4 100644 --- a/main/src/test/java/org/apache/james/jdkim/SignatureRecordImplTest.java +++ b/main/src/test/java/org/apache/james/jdkim/SignatureRecordImplTest.java @@ -122,12 +122,22 @@ public class SignatureRecordImplTest { @Test public void testWrongHashSyntaxes() { - SignatureRecord sr = new SignatureRecordImpl("v=1; a=nothyphenedword; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com"); - assertThatThrownBy(sr::getHashAlgo) - .hasMessageContaining("hash"); + assertThatThrownBy(() -> new SignatureRecordImpl("v=1; a=nothyphenedword; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("algorithm"); + } - assertThatThrownBy(sr::getHashMethod) - .hasMessageContaining("hash"); + @Test + public void algorithmsOutsideRfc6376ShouldBeRejected() { + for (String a : new String[] {"rsa-md5", "rsa-md2", "rsa-sha512/256", "rsa-sha512", "rsa-sha384", "rsa-sha224", + "rsa-SHA256", "RSA-SHA256", "Rsa-Sha1", "dsa-sha256", "rsa-sha3-256"}) { + assertThatThrownBy(() -> new SignatureRecordImpl("v=1; a=" + a + "; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com")) + .as("a=%s", a) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("algorithm"); + } + new SignatureRecordImpl("v=1; a=rsa-sha256; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com"); + new SignatureRecordImpl("v=1; a=rsa-sha1; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com"); } @Test diff --git a/main/src/test/java/org/apache/james/jdkim/SignatureRecordTemplateTest.java b/main/src/test/java/org/apache/james/jdkim/SignatureRecordTemplateTest.java index 4fb500a..66f83bf 100644 --- a/main/src/test/java/org/apache/james/jdkim/SignatureRecordTemplateTest.java +++ b/main/src/test/java/org/apache/james/jdkim/SignatureRecordTemplateTest.java @@ -122,13 +122,22 @@ public class SignatureRecordTemplateTest { @Test public void testWrongHashSyntaxes() { - SignatureRecord sr = new SignatureRecordTemplate("v=1; a=nothyphenedword; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com"); - - assertThatThrownBy(sr::getHashAlgo) - .hasMessageContaining("hash"); + assertThatThrownBy(() -> new SignatureRecordTemplate("v=1; a=nothyphenedword; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("algorithm"); + } - assertThatThrownBy(sr::getHashMethod) - .hasMessageContaining("hash"); + @Test + public void algorithmsOutsideRfc6376ShouldBeRejected() { + for (String a : new String[] {"rsa-md5", "rsa-md2", "rsa-sha512/256", "rsa-sha512", "rsa-sha384", "rsa-sha224", + "rsa-SHA256", "RSA-SHA256", "Rsa-Sha1", "dsa-sha256", "rsa-sha3-256"}) { + assertThatThrownBy(() -> new SignatureRecordTemplate("v=1; a=" + a + "; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com")) + .as("a=%s", a) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("algorithm"); + } + new SignatureRecordTemplate("v=1; a=rsa-sha256; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com"); + new SignatureRecordTemplate("v=1; a=rsa-sha1; bh=abcdef; b=1235345987; h=from:to; s=select; d=example.com"); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
