Mailing List Archive: servers expansion

servers expansion

Jun 11, 2021, 5:40 AM

Post #1 of 5 (349 views)

I'm wanting to be able to use expansion variables in the servers=
parameter of query-style lookups.

I can use variables if I put servers= inside the query, but if
servers= is used there I can't use tainted variables in the query.

I crawled around looking at the source code trying to
backtrace to the caller code and finally came up with this patch:

--- a/build/exim/src/lookups/lf_sqlperform.c
+++ b/build/exim/src/lookups/lf_sqlperform.c
@@ -129,7 +129,7 @@ else
uschar * ele;
for (int sep = ','; ele = string_nextinlist(&opts, &sep, NULL, 0); )
if (Ustrncmp(ele, "servers=", 8) == 0)
- { serverlist = ele + 8; break; }
+ { serverlist = expand_string( ele + 8 ); break; }
}

if (!serverlist)
---

This seems to work for simple variables which is enough for me. Full
brace expansion does not work (I think the parser gets confused).

As I understand it this is not going to cause a memory leak.

a few lines down from this serverlist is checked to be taint-free so
this feels safe to me.

--
Jasen.

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Re: servers expansion [ In reply to ]

exim-dev at exim

Jun 12, 2021, 12:56 PM

Post #2 of 5 (349 views)

Permalink

On Sat, 12 Jun 2021, Jasen Betts via Exim-dev wrote:

>
> I'm wanting to be able to use expansion variables in the servers=
> parameter of query-style lookups.
>
> I can use variables if I put servers= inside the query, but if
> servers= is used there I can't use tainted variables in the query.
>
> I crawled around looking at the source code trying to
> backtrace to the caller code and finally came up with this patch:
>
> --- a/build/exim/src/lookups/lf_sqlperform.c
> +++ b/build/exim/src/lookups/lf_sqlperform.c
> @@ -129,7 +129,7 @@ else
> uschar * ele;
> for (int sep = ','; ele = string_nextinlist(&opts, &sep, NULL, 0); )
> if (Ustrncmp(ele, "servers=", 8) == 0)
> - { serverlist = ele + 8; break; }
> + { serverlist = expand_string( ele + 8 ); break; }
> }
>
> if (!serverlist)
> ---
>
> This seems to work for simple variables which is enough for me. Full
> brace expansion does not work (I think the parser gets confused).
>
> As I understand it this is not going to cause a memory leak.
>
> a few lines down from this serverlist is checked to be taint-free so
> this feels safe to me.

Isn't the idea to check a string is taint-free *before* expanding it ?

--
Andrew C. Aitchison Kendal, UK
andrew@aitchison.me.uk

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Re: servers expansion [ In reply to ]

exim-dev at exim

Jun 12, 2021, 1:11 PM

Post #3 of 5 (349 views)

Permalink

On 12/06/2021 20:56, Andrew C Aitchison via Exim-dev wrote:
> On Sat, 12 Jun 2021, Jasen Betts via Exim-dev wrote:
>> I'm wanting to be able to use expansion variables in the servers=
>> parameter of query-style lookups.

This immediately sounds dangerous.

[suggested code change]

>> This seems to work for simple variables which is enough for me. Full
>> brace expansion does not work (I think the parser gets confused).
>>
>> As I understand it this is not going to cause a memory leak.
>>
>> a few lines down from this serverlist is checked to be taint-free so
>> this feels safe to me.
>
> Isn't the idea to check a string is taint-free *before* expanding it ?

Precisely. Consider what an attacker might present you with to get
expanded, and the extensive facilities that Exim expansion offers.
--
Cheers,
Jeremy

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Re: servers expansion [ In reply to ]

exim-dev at exim

Jun 12, 2021, 7:39 PM

Post #4 of 5 (349 views)

Permalink

On 2021-06-12, Andrew C Aitchison via Exim-dev <exim-dev@exim.org> wrote:
> On Sat, 12 Jun 2021, Jasen Betts via Exim-dev wrote:
>
>>
>> I'm wanting to be able to use expansion variables in the servers=
>> parameter of query-style lookups.
>>
>> I can use variables if I put servers= inside the query, but if
>> servers= is used there I can't use tainted variables in the query.
>>
>> I crawled around looking at the source code trying to
>> backtrace to the caller code and finally came up with this patch:
>>
>> --- a/build/exim/src/lookups/lf_sqlperform.c
>> +++ b/build/exim/src/lookups/lf_sqlperform.c
>> @@ -129,7 +129,7 @@ else
>> uschar * ele;
>> for (int sep = ','; ele = string_nextinlist(&opts, &sep, NULL, 0); )
>> if (Ustrncmp(ele, "servers=", 8) == 0)
>> - { serverlist = ele + 8; break; }
>> + { serverlist = expand_string( ele + 8 ); break; }
>> }
>>
>> if (!serverlist)
>> ---
>>
>> This seems to work for simple variables which is enough for me. Full
>> brace expansion does not work (I think the parser gets confused).
>>
>> As I understand it this is not going to cause a memory leak.
>>
>> a few lines down from this serverlist is checked to be taint-free so
>> this feels safe to me.
>
> Isn't the idea to check a string is taint-free *before* expanding it ?

If the expansion being done is a safe expansion then the taintness is
preserved and the checking is deferred until a dangerous expansion is
encountered. (this is AIUI done by some sort of pointer magic)

Variable substitution is taint-preserving so if tainted data
is used in the servers parameter it will be rejected by the test.
(because servers is rightly considered dangerous)

--
Jasen.

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Re: servers expansion [ In reply to ]

exim-dev at exim

Jun 12, 2021, 8:04 PM

Post #5 of 5 (349 views)

Permalink

On 2021-06-12, Jeremy Harris via Exim-dev <exim-dev@exim.org> wrote:
> On 12/06/2021 20:56, Andrew C Aitchison via Exim-dev wrote:
>> On Sat, 12 Jun 2021, Jasen Betts via Exim-dev wrote:
>>> I'm wanting to be able to use expansion variables in the servers=
>>> parameter of query-style lookups.
>
> This immediately sounds dangerous.

It can't use tainted values, so the value will be untainted data like the result of some
lookup or arithmetic.

> [suggested code change]
>
>>> This seems to work for simple variables which is enough for me. Full
>>> brace expansion does not work (I think the parser gets confused).
>>>
>>> As I understand it this is not going to cause a memory leak.
>>>
>>> a few lines down from this serverlist is checked to be taint-free so
>>> this feels safe to me.
>>
>> Isn't the idea to check a string is taint-free *before* expanding it ?
>
> Precisely. Consider what an attacker might present you with to get
> expanded, and the extensive facilities that Exim expansion offers.

Isn't the value computed and then checked for taint before the
dangerous thing is done. In this case the dangerous thing using the
servers setting to connect to and to query a variable server.

The check is on line 161

if (is_tainted(server))

--
Jasen.

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##