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 12540753ac74d937bb83958c798743ae37098c3a Author: Benoit TELLIER <[email protected]> AuthorDate: Sun Nov 1 19:56:25 2026 +0000 [FIX] DKIM-Signature: reject a malformed q= getRecordLookupMethods split q= without validating its entries (a "TODO add validation"), and String#split dropped trailing empty ones: q=-x/y:dns/txt or q=:dns/txt passed, and each entry, even empty, was handed to the key retriever as a lookup method. With strict validation (-Djames.jdkim.strict.validation, default true), every entry must now be a hyphenated-word optionally followed by "/" and a qp-hdr-value (RFC 6376 section 3.5), empty entries included, and validate() parses q= so that the record is rejected before any lookup. Unknown but well-formed methods are still skipped. --- docs/modules/ROOT/pages/usage.adoc | 4 +-- .../james/jdkim/tagvalue/SignatureRecordImpl.java | 20 ++++++++++---- .../jdkim/tagvalue/SignatureRecordTemplate.java | 15 ++++++---- .../james/jdkim/SignatureRecordImplTest.java | 32 ++++++++++++++++++++++ 4 files changed, 59 insertions(+), 12 deletions(-) diff --git a/docs/modules/ROOT/pages/usage.adoc b/docs/modules/ROOT/pages/usage.adoc index 7b902bc..b25c00c 100644 --- a/docs/modules/ROOT/pages/usage.adoc +++ b/docs/modules/ROOT/pages/usage.adoc @@ -278,8 +278,8 @@ jDKIM rejects, as a syntax error (RFC 6376 sections 3.5, 3.6.1 and 6.1), tag values that do not follow the RFC grammar, such as an empty entry or a trailing `:` in the `h=`, `k=`, `s=` or `t=` lists of a key record, an empty `t=`, a DKIM-Signature `d=` or `s=` that is not a domain name (a `_`, an -empty label, a label over 63 octets), or an `i=` whose local part or domain is -malformed. A verifier stricter than the others may however reject real-world, +empty label, a label over 63 octets), an `i=` whose local part or domain is +malformed, or a `q=` with an empty or malformed entry. A verifier stricter than the others may however reject real-world, slightly malformed signatures or keys. Start the JVM with `-Djames.jdkim.strict.validation=false` to disable these syntax checks. Checks with a semantic effect -- a key for another service (`s=chat`), an expired 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 caf94de..e0e3382 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 @@ -137,6 +137,11 @@ public class SignatureRecordImpl extends TagValue implements SignatureRecord { checkDomainName("i= domain", value.substring(at + 1), 2); } + // SPEC: sig-q-tag-method = "dns/txt" / x-sig-q-tag-type ["/" x-sig-q-tag-args], the type a + // hyphenated-word and the args a qp-hdr-value (dkim-safe-char but "|", or "=" HEX HEX) + static final Pattern QUERY_METHOD = Pattern.compile( + "[A-Za-z](?:[A-Za-z0-9-]*[A-Za-z0-9])?(?:/(?:[\\x21-\\x3A\\x3C\\x3E-\\x7B\\x7D\\x7E]|=[0-9A-F]{2})*)?"); + /** * Checks the a= value of a signature record, see {@link #checkAlgorithm(CharSequence)}. * @@ -188,6 +193,7 @@ public class SignatureRecordImpl extends TagValue implements SignatureRecord { checkAlgorithm(getValue("a")); checkSdidAndSelector(getValue("d"), getValue("s")); + getRecordLookupMethods(); CharSequence identity; try { @@ -436,13 +442,17 @@ public class SignatureRecordImpl extends TagValue implements SignatureRecord { public List<CharSequence> getRecordLookupMethods() { String flags = getValue("q").toString(); - String[] flagsStrings = flags.split(":"); + boolean strict = isStrictValidation(); + // -1 keeps the trailing empty entries, to reject them + String[] flagsStrings = flags.split(":", strict ? -1 : 0); List<CharSequence> res = new LinkedList<>(); for (String flagsString : flagsStrings) { - // TODO add validation method[/option] - // if (VALIDATION) - res.add(trimFWS(flagsString, 0, flagsString.length() - 1, - true)); + CharSequence method = trimFWS(flagsString, 0, flagsString.length() - 1, true); + if (strict && !QUERY_METHOD.matcher(method).matches()) { + throw new IllegalStateException("Invalid q= [" + flags + "]: entry [" + method + + "] is not type [\"/\" qp-hdr-value] (RFC 6376 section 3.5)"); + } + res.add(method); } return res; } 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 7ac6ff8..a113ad0 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 @@ -72,6 +72,7 @@ public class SignatureRecordTemplate extends TagValue implements SignatureRecord SignatureRecordImpl.checkAlgorithm(getValue("a")); } SignatureRecordImpl.checkSdidAndSelector(getValue("d"), getValue("s")); + getRecordLookupMethods(); // TODO: what about v=0.5 and no v= at all? // do specs allow parsing? what should we check? if (!"1".equals(getValue("v"))) @@ -293,13 +294,17 @@ public class SignatureRecordTemplate extends TagValue implements SignatureRecord public List<CharSequence> getRecordLookupMethods() { String flags = getValue("q").toString(); - String[] flagsStrings = flags.split(":"); + boolean strict = isStrictValidation(); + // -1 keeps the trailing empty entries, to reject them + String[] flagsStrings = flags.split(":", strict ? -1 : 0); List<CharSequence> res = new LinkedList<>(); for (String flagsString : flagsStrings) { - // TODO add validation method[/option] - // if (VALIDATION) - res.add(trimFWS(flagsString, 0, flagsString.length() - 1, - true)); + CharSequence method = trimFWS(flagsString, 0, flagsString.length() - 1, true); + if (strict && !SignatureRecordImpl.QUERY_METHOD.matcher(method).matches()) { + throw new IllegalStateException("Invalid q= [" + flags + "]: entry [" + method + + "] is not type [\"/\" qp-hdr-value] (RFC 6376 section 3.5)"); + } + res.add(method); } return res; } 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 000f7e3..dedfa35 100644 --- a/main/src/test/java/org/apache/james/jdkim/SignatureRecordImplTest.java +++ b/main/src/test/java/org/apache/james/jdkim/SignatureRecordImplTest.java @@ -386,4 +386,36 @@ public class SignatureRecordImplTest { System.clearProperty(TagValue.STRICT_VALIDATION_PROPERTY); } } + + private static SignatureRecordImpl recordWithQuery(String q) { + return new SignatureRecordImpl("v=1; a=rsa-sha256; c=relaxed/relaxed; d=example.com; s=sel;" + + " h=from:subject; q=" + q + "; bh=AAAA; b=AAAA"); + } + + @Test + public void malformedQueryMethodsAreASyntaxError() { + for (String q : new String[] {"-x/y:dns/txt", ":dns/txt", "dns/txt:", "dns/txt::dns/txt", "/txt", "x-/y:dns/txt", "dns/t|xt"}) { + assertThatThrownBy(() -> recordWithQuery(q)).as("q=%s", q) + .isInstanceOf(IllegalStateException.class); + } + } + + @Test + public void wellFormedQueryMethodsAreAccepted() { + recordWithQuery("dns/txt"); + recordWithQuery("dns"); + recordWithQuery("x-foo/bar:dns/txt"); + recordWithQuery("dns/txt : x-other"); + recordWithQuery("dns/txt:\r\n x-other"); + } + + @Test + public void malformedQueryMethodsAreAcceptedWithoutStrictValidation() { + System.setProperty(TagValue.STRICT_VALIDATION_PROPERTY, "false"); + try { + Assert.assertEquals(2, recordWithQuery("dns/txt:-x/y:").getRecordLookupMethods().size()); + } finally { + System.clearProperty(TagValue.STRICT_VALIDATION_PROPERTY); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
