Re: [PATCH] Unknown cfg function

On 30/07/2013 6:40 a.m., Alex Rousskov wrote:
> On 07/29/2013 11:25 AM, Amos Jeffries wrote:
>> Looks like nobody actually ran the trivial "-k parse" test using 3.HEAD
>> or the initial quoted strings code.
> Or perhaps nobody ran such a test with non-trivial regular expressions
> in squid.conf? I know I tested [very] early versions of the quoted
> strings code (which is now running on live servers), but I suspect my
> configs did not use regular expressions that trigger these problems or
> perhaps the early code was different enough not to trigger them: IIRC,
> there have been many revisions of the original patch.
>
>
>> Attached is a patch which adds a secondary form of undo for handling
>> incorrectly identified ConfigParser::FunctionNameToken elements. Instead
>> of aborting Squid on any non-function token containing a '(' we take the
>> wrongly identified assumed function name element and reform it with the
>> '(' delimiter and trailing element. The resulting aggregate token which
>> should have been identified initially is then sent to the upper parser
>> layer.
>
> After this patch, if I type parametres(foo) instead of parameters(foo),
> will Squid think that I am defining a regular expression instead of
> importing foo where my true regular expressions are stored?

Yes. And the only way to avoid that is to either prohibit blah(foo) in
places where regex is expected, or to ignore such typos at this level
and rely on the upper level data validation.

The core problem here is that '(' is treated as a primary delimiter just
like whitespace outside of quoted strings. So *any* use of brackets
inside a token shifts to the filename loading logics. You suggested on
IRC that we use perl syntax s/(foo)/ patterns. There is still a '('
present inside there which will be detected as end of opaque element
"s/" -> invalid function name -> self_destruct().

Regex are inherently dangerous to a very high degree simply from their
nature and operation. Overall I don't see typos in regex fields as being
made any more worse or problematic for the existence of "parameters("
tokens. Other non-regex config details should be able to and actively
applying more targeted token vlidation on top and detect these typos
with a better context-aware message - which may or may not result in
self_destruct() dependign on that extra context.

Amos
Received on Mon Jul 29 2013 - 20:13:22 MDT