Re: [exim] server_set_id no longer working?

On 2012-11-05 at 18:29 +0200, James Isolder wrote:
> The below auth when it fails
>

> spa_auth:
>                 driver                          = spa
>                 public_name               = NTLM
>                 hide server_password  = ${lookup
> mysql{NTLM_AUTH}{$value}fail}
>                 server_set_id              = $1

>
> used to give me
>
> 2012-11-05 17:20:18 spa_auth authenticator failed for <hostname> [IP
> number]: 535 Incorrect authentication
> data (set_id=<authid>)
>
> After the upgrade to 4.80.1 I only get
>
> 2012-11-05 18:10:59 spa_authenticator authenticator failed for <hostname>
> [IP number]: 535 Incorrect authentication
> data

This is my fault, and only affects NTLM authentication. The bad commit
is 08488c86; I was fixing Clang compiler warnings. In spa.c:

----------------------------8< cut here >8------------------------------
+/* clean up globals which aren't referenced, but still shouldn't be left
+pointing to stack memory */
+#define CLEANUP_RETURN(Code) do { auth_vars[0] = expand_nstring[1] = NULL; \
+ expand_nlength[1] = expand_nmax = 0; return (Code); } while (0);
----------------------------8< cut here >8------------------------------

This arose from:
http://mejor.pl/exim-4.77/report-IORFNM.html#EndPath

Basically, the address of stack memory was assigned to a global
variable, which is then referenced after return, kind-of-before another
function call. This is "problematic", so changing it was right. The
change applied was to wipe the data as not being needed after return,
which is false.

To get it working quickly, in this section of code in src/auths/spa.c:
----------------------------8< cut here >8------------------------------
241 /* success. we have a winner. */
242 {
243 int rc = auth_check_serv_cond(ablock);
244 CLEANUP_RETURN(rc);
245 }
----------------------------8< cut here >8------------------------------

you can change "CLEANUP_RETURN(rc);" to "return rc;".

That will still leave stack memory in a global, it's still not great,
but what the code in src/smtp_in.c does is this:
----------------------------8< cut here >8------------------------------
    c = (au->info->servercode)(au, smtp_cmd_data);
    if (au->set_id != NULL) set_id = expand_string(au->set_id);
----------------------------8< cut here >8------------------------------

So immediately after the authenticator returns, set_id will be expanded,
which uses the auth values. After _that_, nothing uses them. As long
as expand_string didn't extend its stack memory usage to more than that
of the SPA authenticator, we got away with it.

For bonus points, please change the bad code block in spa.c starting at
line 242 to this untested code: someone needs to test it, and I don't
have a SPA setup in which to test (thus this

----------------------------8< cut here >8------------------------------
  {
  int rc = auth_check_serv_cond(ablock);
  if (rc == OK)
    {
    auth_vars[0] = expand_nstring[1] = string_copy(msgbuf);
    return OK;
    }
  CLEANUP_RETURN(rc);
  }
----------------------------8< cut here >8------------------------------

Please do try that and let us know if it works; if it does, we'll add
that to Exim. And fix the test suite, entry 3600, to test a little more
for SPA.

Thanks,
-Phil