[squid-dev] [RFC] on_crash

[Amos Jeffries]

On 10/12/2015 6:51 a.m., Alex Rousskov wrote:
> On 12/09/2015 10:29 AM, Kinkie wrote:
> 
>>   this sounds like an useful feature, especially if we also manage to
>> include a sample/default crash handler.
> 
> A sample sounds good (the script I posted can be a good starting point
> though it is far from perfect). There should probably be no default
> script because crash handling needs vary a lot and because there are
> probably security implications related to one.
> 
> 
>> Among the choices you provide I like "on_fatal_exit"; 
> 
> I am curious why you think on_crash is worse than on_fatal_exit?
> 
> 
>> I could also
>> suggest "crash_handler", "crash_helper", "crash_report_collector" or
>> somesuch.
> 
> IMO, we should use the "on_" prefix because it allows us to group many
> simple options related to crash handling instead of focusing on the
> handler alone (see my original post for examples). It is also a familiar
> prefix for all sorts of event handlers.
> 


I don't have much opinion either way right now. on_fatal / on_fatal_exit
seems fine.

Calling the binary bundled to do the job "squid_crash_reporter" seems
the right thing to do.


I am not going to +1 this right now, because the issues below are
security related. I would like to see at least one round of audit with
them resolved before it goes in.


As for the patch:

* function should be "static void" it seems.


* please never hard-code anything be executed from /tmp.
 - for security /tmp should always be considered to be under a remote
attackers control. The patch as-is enables a major vulnerability.
 - it should either default to not happening, or use a bundled
squid_crash_reporter binary installed alongside the other helpers.


* please do not use system() for this.

[sorry linking the draft, I can't seem to find the final PDF right now]
<http://www.secologic.org/downloads/c/051207_EUROSEC_Draft_Whitepaper_Secure_C_Programming.doc>
"
* Do not call a shell to invoke another program from within a C/C++
program.

>From within a C/C++ program it is possible to call a shell to invoke
another program. This can be accomplished with function calls like
system() or popen(). Using such function calls opens a program to
attacks by modification of the environment (particularly by changing the
PATH and IFS variables). To avoid dependencies on the environment, usage
of execve() is recommended instead.

* To call another program from within a process use execve() or execle().

To call another program from within a process one of the exec functions
may be used. The functions execl(), execv(), execlp() and execvp() do
use the current environment when calling a new program, whereas execve()
and execle() do have an input argument for the new environment to be
used. As it is safe programming practice not to depend on the
environment, the execve() or execle() function calls should be used.
Moreover execlp() and execvp() evaluate the PATH environment variable to
find the called executable. This makes these function calls vulnerable
to attacks that change the PATH variable.
"

I know we have a lot of system()/popen() legacy code hanging around. But
crash is one place we need to be extra careful.



For anyone thinking of bundling a helper for this please make the
reporter a binary. For the same reasons quoted above regarding system()
and environment contents. Also avoiding the speed (and memory) overheads
of bash/cash/ksh/python/php/whatever is a very good idea in these
situations.

Amos