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 ebd3b607204814602cda6234d8fbc536885fdb9a Author: Benoit TELLIER <[email protected]> AuthorDate: Sun Nov 1 19:48:43 2026 +0000 [FIX] DKIMVerifier: reject signatures made with short RSA keys signatureVerify used the public key as it was: signatures made with 512-bit and 768-bit RSA keys passed, although such keys can be factored and RFC 8301 section 3.2 requires at least 1024 bits from signers. VerifierOptions gains withMinimumRsaKeyBits, defaulting to the james.dkim.min-rsa-key-bits system property, or 1024. A signature made with a shorter key gets a PERMERROR ("RSA key too short"). The Perl corpus test, whose keys are 512 bits long, lowers the minimum. --- docs/modules/ROOT/pages/usage.adoc | 7 ++++ .../java/org/apache/james/jdkim/DKIMVerifier.java | 7 ++++ .../apache/james/jdkim/api/VerifierOptions.java | 35 ++++++++++++++++ .../james/jdkim/DKIMVerifierOptionsTest.java | 20 ++++++++++ .../org/apache/james/jdkim/DKIMVerifierTest.java | 46 ++++++++++++++++++++++ .../java/org/apache/james/jdkim/PerlDKIMTest.java | 4 +- 6 files changed, 118 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/usage.adoc b/docs/modules/ROOT/pages/usage.adoc index 10e37a8..83298bc 100644 --- a/docs/modules/ROOT/pages/usage.adoc +++ b/docs/modules/ROOT/pages/usage.adoc @@ -222,6 +222,13 @@ get a `PERMERROR` result ("Too many DKIM-Signature fields") without any lookup. Defaults to the `james.dkim.max-signature.count` system property, or 32 when it is unset (e.g. `-Djames.dkim.max-signature.count=32`); must be strictly positive. +`withMinimumRsaKeyBits(int)`:: +The minimum length of the RSA keys whose signatures are accepted; shorter keys +get a `PERMERROR` result ("RSA key too short"), as +https://datatracker.ietf.org/doc/html/rfc8301#section-3.2[RFC 8301 section 3.2] +allows. Defaults to the `james.dkim.min-rsa-key-bits` system property, or 1024 +when it is unset; 0 accepts any key. + `withUnsignedDuplicateFieldPolicy(UnsignedDuplicateFieldPolicy)`:: What happens to a signature when the message holds more instances of a single-instance field listed in its `h=` than `h=` lists, and more than one in 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 62a2782..d610173 100644 --- a/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java +++ b/main/src/main/java/org/apache/james/jdkim/DKIMVerifier.java @@ -48,6 +48,7 @@ import java.security.NoSuchAlgorithmException; import java.security.PublicKey; import java.security.Signature; import java.security.SignatureException; +import java.security.interfaces.RSAPublicKey; import java.time.Duration; import java.time.Instant; import java.util.ArrayList; @@ -605,6 +606,12 @@ public class DKIMVerifier { } catch (IllegalStateException e) { throw new PermFailException("Invalid Public Key: " + e.getMessage(), sign, e); } + // RFC 8301 section 3.2: verifiers MAY take the key length into account. + if (publicKey instanceof RSAPublicKey + && ((RSAPublicKey) publicKey).getModulus().bitLength() < options.getMinimumRsaKeyBits()) { + throw new PermFailException("RSA key too short: " + ((RSAPublicKey) publicKey).getModulus().bitLength() + + " bits, at least " + options.getMinimumRsaKeyBits() + " required (RFC 8301)", sign); + } signature.initVerify(publicKey); signatureCheck(h, sign, headers, signature, signatureFieldName); diff --git a/main/src/main/java/org/apache/james/jdkim/api/VerifierOptions.java b/main/src/main/java/org/apache/james/jdkim/api/VerifierOptions.java index fd5bb8c..4f083be 100644 --- a/main/src/main/java/org/apache/james/jdkim/api/VerifierOptions.java +++ b/main/src/main/java/org/apache/james/jdkim/api/VerifierOptions.java @@ -38,6 +38,11 @@ public class VerifierOptions { */ public static final String MAX_SIGNATURE_COUNT_PROPERTY = "james.dkim.max-signature.count"; private static final int DEFAULT_MAX_SIGNATURE_COUNT = 32; + /** + * System property holding the default of {@link Builder#withMinimumRsaKeyBits(int)}. + */ + public static final String MIN_RSA_KEY_BITS_PROPERTY = "james.dkim.min-rsa-key-bits"; + private static final int DEFAULT_MIN_RSA_KEY_BITS = 1024; /** * System property holding the default of {@link Builder#withUnsignedDuplicateFieldPolicy(UnsignedDuplicateFieldPolicy)}. */ @@ -51,6 +56,7 @@ public class VerifierOptions { private final Duration clockDriftTolerance; private final int maxSignatures; + private final int minimumRsaKeyBits; private final UnsignedDuplicateFieldPolicy unsignedDuplicateFieldPolicy; private final Set<String> singleInstanceFields; private final PublicKeyRecordRetriever publicKeyRecordRetriever; @@ -59,6 +65,7 @@ public class VerifierOptions { public static class Builder { private Duration clockDriftTolerance = Duration.ofSeconds(300); private int maxSignatures = Integer.getInteger(MAX_SIGNATURE_COUNT_PROPERTY, DEFAULT_MAX_SIGNATURE_COUNT); + private int minimumRsaKeyBits = Integer.getInteger(MIN_RSA_KEY_BITS_PROPERTY, DEFAULT_MIN_RSA_KEY_BITS); private UnsignedDuplicateFieldPolicy unsignedDuplicateFieldPolicy = defaultUnsignedDuplicateFieldPolicy(); private Set<String> singleInstanceFields = DEFAULT_SINGLE_INSTANCE_FIELDS; private Resolver dnsResolver = Lookup.getDefaultResolver(); @@ -100,6 +107,20 @@ public class VerifierOptions { return this; } + /** + * Sets the minimum length, in bits, of the RSA keys whose signatures are accepted. Shorter keys get a + * PERMERROR result. RFC 8301 section 3.2: signers MUST use keys of at least 1024 bits, and verifiers + * MAY take the key length into account. Defaults to the {@value #MIN_RSA_KEY_BITS_PROPERTY} system + * property, or 1024. + * + * @param minimumRsaKeyBits a positive number of bits, 0 to accept any key + * @return {@link Builder} + */ + public Builder withMinimumRsaKeyBits(int minimumRsaKeyBits) { + this.minimumRsaKeyBits = minimumRsaKeyBits; + return this; + } + /** * Sets what happens to a signature when the message holds more instances of a single-instance field * (see {@link #withSingleInstanceFields(Collection)}) listed in its h= tag than h= lists, and more than one @@ -169,6 +190,10 @@ public class VerifierOptions { throw new IllegalArgumentException("maxSignatures must be strictly positive"); } + if (builder.minimumRsaKeyBits < 0) { + throw new IllegalArgumentException("minimumRsaKeyBits must not be negative"); + } + if (builder.unsignedDuplicateFieldPolicy == null) { throw new IllegalArgumentException("unsignedDuplicateFieldPolicy can not be null"); } @@ -187,6 +212,7 @@ public class VerifierOptions { this.clockDriftTolerance = builder.clockDriftTolerance; this.maxSignatures = builder.maxSignatures; + this.minimumRsaKeyBits = builder.minimumRsaKeyBits; this.unsignedDuplicateFieldPolicy = builder.unsignedDuplicateFieldPolicy; this.singleInstanceFields = builder.singleInstanceFields; this.dnsResolver = builder.dnsResolver; @@ -211,6 +237,15 @@ public class VerifierOptions { return maxSignatures; } + /** + * Gets the minimum length, in bits, of the RSA keys whose signatures are accepted + * + * @return a number of bits + */ + public int getMinimumRsaKeyBits() { + return minimumRsaKeyBits; + } + /** * Gets what happens to a signature leaving an instance of a single-instance field unsigned * diff --git a/main/src/test/java/org/apache/james/jdkim/DKIMVerifierOptionsTest.java b/main/src/test/java/org/apache/james/jdkim/DKIMVerifierOptionsTest.java index c55ed62..287e067 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMVerifierOptionsTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMVerifierOptionsTest.java @@ -174,4 +174,24 @@ public class DKIMVerifierOptionsTest { public void shouldRejectNullSingleInstanceFields() { new VerifierOptions.Builder().withSingleInstanceFields(null).build(); } + + @Test + public void shouldRequire1024BitRsaKeysByDefault() { + assertEquals(1024, new VerifierOptions.Builder().build().getMinimumRsaKeyBits()); + } + + @Test + public void shouldReadMinimumRsaKeyBitsFromSystemProperty() { + System.setProperty(VerifierOptions.MIN_RSA_KEY_BITS_PROPERTY, "2048"); + try { + assertEquals(2048, new VerifierOptions.Builder().build().getMinimumRsaKeyBits()); + } finally { + System.clearProperty(VerifierOptions.MIN_RSA_KEY_BITS_PROPERTY); + } + } + + @Test(expected = IllegalArgumentException.class) + public void shouldRejectNegativeMinimumRsaKeyBits() { + new VerifierOptions.Builder().withMinimumRsaKeyBits(-1).build(); + } } 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 a2b9468..b6b4ba2 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMVerifierTest.java @@ -28,6 +28,9 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.util.Base64; import java.util.Collections; import java.util.concurrent.atomic.AtomicInteger; @@ -191,4 +194,47 @@ public class DKIMVerifierTest { assertThat(key.getGranularityPattern().matcher("a\r\nb").matches()).isTrue(); assertThat(key.getGranularityPattern().matcher("ab\n").matches()).isFalse(); } + + private static DKIMVerifier signAndVerify(int keyBits, VerifierOptions.Builder options, StringBuilder mailOut) throws Exception { + KeyPairGenerator gen = KeyPairGenerator.getInstance("RSA"); + gen.initialize(keyBits); + KeyPair pair = gen.generateKeyPair(); + MockPublicKeyRecordRetriever retriever = new MockPublicKeyRecordRetriever(); + retriever.addRecord("sel", "example.com", + "v=DKIM1; p=" + Base64.getEncoder().encodeToString(pair.getPublic().getEncoded())); + String message = "From: [email protected]\r\nSubject: test\r\n\r\nbody\r\n"; + String field = new DKIMSigner("v=1; a=rsa-sha256; d=example.com; s=sel; h=from:subject; bh=; b=;", pair.getPrivate()) + .sign(new ByteArrayInputStream(message.getBytes(StandardCharsets.US_ASCII))); + mailOut.append(field).append("\r\n").append(message); + return new DKIMVerifier(options.withPublicKeyRecordRetriever(retriever).build()); + } + + @Test + public void shortRsaKeysShouldNotPass() throws Exception { + for (int bits : new int[] {512, 768}) { + StringBuilder mail = new StringBuilder(); + DKIMVerifier verifier = signAndVerify(bits, new VerifierOptions.Builder(), mail); + + assertThatThrownBy(() -> verifier.verify(new ByteArrayInputStream(mail.toString().getBytes(StandardCharsets.US_ASCII)))) + .isInstanceOf(PermFailException.class) + .hasMessageContaining("RSA key too short: " + bits + " bits"); + assertThat(verifier.getResults()).extracting(Result::getResultType).containsExactly(Result.Type.PERMERROR); + } + } + + @Test + public void shortRsaKeysShouldPassWhenAllowed() throws Exception { + StringBuilder mail = new StringBuilder(); + DKIMVerifier verifier = signAndVerify(512, new VerifierOptions.Builder().withMinimumRsaKeyBits(512), mail); + + assertThat(verifier.verify(new ByteArrayInputStream(mail.toString().getBytes(StandardCharsets.US_ASCII)))).hasSize(1); + } + + @Test + public void rsaKeysOfTheMinimumLengthShouldPass() throws Exception { + StringBuilder mail = new StringBuilder(); + DKIMVerifier verifier = signAndVerify(1024, new VerifierOptions.Builder(), mail); + + assertThat(verifier.verify(new ByteArrayInputStream(mail.toString().getBytes(StandardCharsets.US_ASCII)))).hasSize(1); + } } diff --git a/main/src/test/java/org/apache/james/jdkim/PerlDKIMTest.java b/main/src/test/java/org/apache/james/jdkim/PerlDKIMTest.java index e4e8d4b..16c29e9 100644 --- a/main/src/test/java/org/apache/james/jdkim/PerlDKIMTest.java +++ b/main/src/test/java/org/apache/james/jdkim/PerlDKIMTest.java @@ -113,7 +113,9 @@ public class PerlDKIMTest extends TestCase { expectFailure = true; try { - DKIMVerifier verifier = new DKIMVerifier(new VerifierOptions.Builder().withPublicKeyRecordRetriever(pkr).build()); + DKIMVerifier verifier = new DKIMVerifier(new VerifierOptions.Builder().withPublicKeyRecordRetriever(pkr) + // the corpus keys are 512 bits long + .withMinimumRsaKeyBits(512).build()); List<SignatureRecord> res = verifier.verify(is); if (getName().matches("good_dk_7|good_dk_6|dk_headers_2|good_dk_3") --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
