https://bugs.exim.org/show_bug.cgi?id=1853
Bug ID: 1853
Summary: DKIM attempts validation on bad signatures / segfaults
on invalid base64
Product: Exim
Version: N/A
Hardware: x86
OS: Linux
Status: NEW
Severity: bug
Priority: medium
Component: DKIM
Assignee: tom@???
Reporter: mrgus@???
CC: exim-dev@???
Created attachment 902
-->
https://bugs.exim.org/attachment.cgi?id=902&action=edit
The variations on a patch and some test cases
Sorry this is kind of long... Changed more than I'd originally meant to, so
feel the need to justify myself.
If the base64 strings for the b= or bh= tags in the DKIM-Signature header are
not actually valid base64, it will cause Exim to segfault.
This is the root problem I was trying to fix, but it took me down a bit of a
rabbit hole. Attached is a patch (dkim_sigfail_full.diff) that will fix this
segfault, as well as improve the handling of other missing or invalid tags in
the DKIM-Signature header.
There are also some test cases in "samples/".
I'll do this as a pull request on github as well.
DETAILS:
The segfault problem has a few different levels.
pdkim_parse_sig_header():
Where the problem starts. In pdkim_parse_sig_header(), if either of the two
pdkim_decode_base64 functions fail, signature parsing continues. Near the end
of this function, the code that makes pdkim_parse_sig fail if these (or other)
attributes have a problem was almost entirely removed in commits
2592e6c0eda522da0f6a33f4d32e33598288eb6e and
ca9cb170c15a0c996549c256997b57c30d4b30dd.
With the current code, this function can only fail if the v= tag is incorrect.
No other issues are checked anymore.
pdkim_header_complete():
If pdkim_parse_sig_header() fails, I believe it was (at some point) intended to
"return PDKIM_FAIL;" on failure, but I think it was overlooked. It continues
on.
pdkim_feed_finish():
Finally, here is where the actual segfault occurs. Since everything assumes the
DKIM-Signature header was just fine regardless of its content, it will try to
exim_rsa_verify sig->sigdata even though it's set to NULL (because base64
decoding failed), causing the segfault.
On an older version, putting the "return PDKIM_FAIL;" mentioned above would
prevent the segfaults. In the current version, if you fix
pdkim_parse_sig_header() so it also can return errors, these two changes alone
can fix the segfaults (and I included a minimal patch demonstrating this as
well).
However, this is *not* ideal as it will prevent Exim from parsing other DKIM
headers AND it will disable all reporting of DKIM results (even the failure).
Determining exactly what should happen here had me looking at RFC4871: Missing
tags, invalid syntax in tags and unsupported DKIM version all should be noted,
and then other DKIM-Signatures should be parsed until one passes.
So I added two invalid codes: PDKIM_VERIFY_INVALID_SIGNATURE_ERROR and
PDKIM_VERIFY_INVALID_DKIM_VERSION. I did not differentiate between invalid and
missing b/bh tags.
Since Exim now logs output when it didn't, I also had to adjust how some things
were printed in src/dkim.c (so b= wouldn't print -8 on error, missing algorithm
wouldn't display as rsa-sha1).
Here is a log of out the included test cases evaluated before/after this patch:
dkim_valid.txt [Valid DKIM Test Sample]
- Unpatched: Pass
- Patched: Pass
dkim_valid_v2.txt [Valid sample w/version set to 2]
- Unpatched: Error while parsing (no dkim error result)
- Patched: DKIM ouptut: [invalid - unsupported DKIM version]
dkim_noalgo.txt [Sample with algorithm tag removed]
- Unpatched: DKIM output: [verification failed - signature did not verify
(headers probably modified in transit]
- Patched: DKIM output: [invalid - signature tag missing or invalid], a=err
dkim_badhash_validbase64.txt [Incorrect hash, but still valid base64]
- Unpatched: DKIM output: [verification failed - signature did not verify
(headers probably modified in transit)]
- Patched: DKIM output: [verification failed - signature did not verify
(headers probably modified in transit)]
dkim_badhash_invalidbase64.txt [Incorrect hash, base64 does not parse]
- Unpatched: segfaults
- Patched: DKIM output: [invalid - signature tag missing or invalid], b=0
dkim_twosigs_badfirst.txt [2sigs, valid-but-incorrect first]
- Unpatched: fail (headers probably modified) + pass
- Patched: fail (headers probably modified) + pass
dkim_twosigs_badsecond.txt [2sigs, valid-but-incorrect second]
- Unpatched: pass + fail (headers probably modified)
- Patched: pass + fail (headers probably modified)
dkim_twosigs_invalidsecond.txt [2sigs, invalid base64 second]
- Unpatched: pass then segfaults
- Patched: pass + [invalid - signature tag missing or invalid]
dkim_twosigs_noalgosecond.txt [2sigs, no algorithm second]
- Unpatched: pass + fail (headers probably modified)
- Patched: pass + [invalid - signature tag missing or invalid]
dkim_twosigs_v2second.txt [2sigs, dkim v2 setting second]
- Unpatched: "Error while parsing" (but no output), then pass
- Patched: pass + [invalid - unsupported DKIM version]
--
You are receiving this mail because:
You are on the CC list for the bug.
