Inconsistent escaping of regex parentheses

[Alan DeKok]

On Jan 21, 2020, at 9:37 AM, Tim <tim at yetanother.net> wrote:
> Using a fresh install of 3.0.20 on CentOS 8, I added the following Unlang procedure to the start of the authorise section within the default server; in order to trigger the behaviour:
> 
> testRegex {
>       update control {
>               Tmp-String-0 = '@(DOMAIN1|DOMAIN2)$'
>       }
> 
>       if ( &User-Name =~ /%{control:Tmp-String-0}/i ) {

  Ah... you're using attributes to define the entire content of a regular expression.  That's generally a bad thing to do.

  The reason is similar to SQL escaping and SQL injection attacks.  You *don't* want user input to control what matches and what doesn't.

> (1)       if ( &User-Name =~ /%{control:Tmp-String-0}/i ) {
> (1)       EXPAND %{control:Tmp-String-0}
> (1)          --> @\(DOMAIN1\|DOMAIN2)\$

  The escaping makes sense, mostly.  But it should be consistent.  i.e. it's not escaping the final ')'.

> (1)       if ( &User-Name =~ /%{control:Tmp-String-0}/i )  -> FALSE
> (1)     } # policy testRegex = noop
> 
> 
> The different error behaviour made me slightly curious, so I went back to a completely clean v3.0.17 CentOS deployment, used the same Unlang procedure as above - and the originally seen error is seen.
> ...{
> (1)       EXPAND %{control:Tmp-String-0}
> (1)          --> @\(DOMAIN1\|DOMAIN2)\$
> (1) ERROR: @\(DOMAIN1\|DOMAIN2)\$
> (1) ERROR:           ^ Pattern compilation failed: unmatched parentheses

  Hmm... I suspect some other change related to how it uses the regex libraries.

> In both cases the regex fails as it is syntactically broken once escaping takes place.

  Yes.

  In short, the expansions inside of regex are for text, not for regex "special characters".  This prevents attacks similar to SQL injections.

  We're working on fixing this in v4, where it will track the "tainted" status of data.  Data taken from user input are tainted / untrusted.  Data taken from configuration files is untainted, and trusted.  So your use-case should work.

  But in v3, it's just too hard to fix.  I'll see if I can figure out why the closing brace isn't escaped.  The code is from 2015, and the comments say that this is intentional.  I don't recall why.

  The simple answer is put regular expressions directly into the configuration.  Don't put them into attributes.

  Alan DeKok.