On Wed, Jul 13, 2005 at 03:23:42PM +0100, Brian Candler wrote:
> OK so far - but if I break either the signature or the secret, I still get a
> validated answer:
>
> # exim -be '${prvscheck {prvs=brian/0123456789@???}{foobar}{$prvscheck_result}}'
> brian@???
>
> # exim -be '${prvscheck {prvs=brian/19844dfc9a@???}{wibble}{$prvscheck_result}}'
> brian@???
>
> Actually, according to doc/NewStuff, $prvscheck_result should either be
> empty string or "1", not an E-mail address.
>
> Am I doing something wrong, or is the doc broken, or is the code broken?
I think I can answer my own question. ${prvscheck} does not work in the same
way as other Exim expansions.
Firstly, it expands its parameter 3 *before* deciding whether the test
passed or not. Secondly, even though it doesn't allow parameter 3 to be
omitted, it treats empty-string as a special case; an empty string here
returns the contents of $prvscheck_address (the unencoded address), whether
or not the cryptographic validation passed or failed.
So:
# exim -d+expand -be '${prvscheck {prvs=brian/0123456789@???}{foobar}{ok}}'
returns 'ok', even though the crypto validation failed
# exim -d+expand -be '${prvscheck {prvs=brian/0123456789@???}{foobar}{$prvscheck_result}}'
returns the E-mail address, because $prvscheck_result is the empty string
# exim -d+expand -be '${prvscheck {prvs=brian/19844dfc9a@???}{foobar}{$prvscheck_result}}'
is the same, even though the signature is valid, because $prvscheck_result
was empty at the time the string was expanded.
So, the only way I can think of to get a sensible answer out of this is:
# exim -d+expand -be '${prvscheck {prvs=brian/19844dfc9a@???}{foobar}{0}}$prvscheck_result'
which returns "0" for failure and "01" for success (yuk).
I think it would be better if it worked like ${expand}: expand parameter 3
if success, expand parameter 4 if fail, and allow 'fail' for explicit
expansion failure in that case. I don't think there's any need to return the
decoded address in the event that cryptographic validation fails; there's
always $prvscheck_address if you need it.
Even better, make prvscheck a condition for use in ${if}, like crypteq or
match.
The patch below fixes some spelling errors in debug messages, but I don't
know enough about exim internals to make ${prvscheck} work in a more
exim-like way.
Also, I'm pretty sure the iexpire/inow logic is hosed. I think it should
check that inow lies between iexpire-7 and iexpire (modulo 1000). At the
moment, a case like
iexpire = 900
inow = 100
will be accepted, even though we are 200 days past the signature's expiry
date. To be fair, the BATV draft is not well-designed here.
Regards,
Brian.
--- exim-4.52/src/expand.c.orig Fri Jul 1 12:09:15 2005
+++ exim-4.52/src/expand.c Wed Jul 13 17:03:56 2005
@@ -3371,7 +3371,7 @@
/* Ugliness: We want to expand parameter 1 first, then set
up expansion variables that are used in the expansion of
parameter 2. So we clone the string for the first
- expansion, where we only expand paramter 1. */
+ expansion, where we only expand parameter 1. */
uschar *s_backup = string_copy(s);
/* Reset expansion variables */
@@ -3448,18 +3448,18 @@
if (iexpire > inow)
{
prvscheck_result = US"1";
- DEBUG(D_expand) debug_printf("prvscheck: success, $pvrs_result set to 1\n");
+ DEBUG(D_expand) debug_printf("prvscheck: success, $prvscheck_result set to 1\n");
}
else
{
prvscheck_result = NULL;
- DEBUG(D_expand) debug_printf("prvscheck: signature expired, $pvrs_result unset\n");
+ DEBUG(D_expand) debug_printf("prvscheck: signature expired, $prvscheck_result unset\n");
}
}
else
{
prvscheck_result = NULL;
- DEBUG(D_expand) debug_printf("prvscheck: hash failure, $pvrs_result unset\n");
+ DEBUG(D_expand) debug_printf("prvscheck: hash failure, $prvscheck_result unset\n");
}
}
else
