Re: [PATCH v4] Improve ptrace-error detection on Linux targets

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, September 24 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> Changes from v3:
>
> Thanks for doing this.  I like this change quite a bit.  For one thing I
> think it would have saved me some time over the years, as I randomly
> forget about ptrace security measures on new machines.

:-)

Thanks for the review.

> Sergio> +++ b/gdb/gdbserver/server.c
> Sergio> @@ -2913,9 +2913,21 @@ handle_v_attach (char *own_buf)
> Sergio>  {
> Sergio>    client_state &cs = get_client_state ();
> Sergio>    int pid;
> Sergio> +  int ret;
>  
> Sergio>    pid = strtol (own_buf + 8, NULL, 16);
> Sergio> -  if (pid != 0 && attach_inferior (pid) == 0)
> Sergio> +
> Sergio> +  try
> Sergio> +    {
> Sergio> +      ret = attach_inferior (pid);
> Sergio> +    }
> Sergio> +  catch (const gdb_exception_error &e)
> Sergio> +    {
> Sergio> +      sprintf (own_buf, "E.%s", e.what ());
>
> Unrestricted sprintf gives me pause.  Do we know own_buf is large
> enough?  Or can/should we truncate the text instead?

I was also worried about this.  I found other cases using sprintf, but
their strings are not as big.  I know own_buf comes from struct
client_state, and its size of PBUFSIZ + 1.  One option would be to use
snprintf with this value.  WDYT?

> Sergio> -gdb_dlopen (const char *filename)
> Sergio> +gdb_dlopen (const char *filename, bool dont_throw)
>
> This parameter is only used in one spot, and it's on an error path that
> is computing the string to show to the user.  So, I think it's better to
> just remove the parameter and use try/catch in that one caller.

OK, I will do that.

> Sergio> +static std::string
> Sergio> +linux_ptrace_restricted_fail_reason (int err)
> Sergio> +{
> ...
> Sergio> +  std::string ret;
> Sergio> +  gdb_dlhandle_up handle = gdb_dlopen ("libselinux.so.1", true);
> Sergio> +
> Sergio> +  if (handle != nullptr)
> Sergio> +    {
> Sergio> +      selinux_ftype selinux_get_bool
> Sergio> +	= (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
> Sergio> +
> Sergio> +      if (selinux_get_bool != NULL
> Sergio> +	  && (*selinux_get_bool) ("deny_ptrace") == 1)
> Sergio> +	string_appendf (ret,
> Sergio> +			_("\n\
> Sergio> +The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
> Sergio> +from using 'ptrace'.  You can disable it by executing (as root):\n\
> Sergio> +\n\
> Sergio> +  setsebool deny_ptrace off\n"));
> Sergio> +    }
> Sergio> +
> Sergio> +  gdb_file_up yama_ptrace_scope
> Sergio> +    = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
> ...
>
> If SELinux was the problem, is it also possible that ptrace_scope is the
> problem?

Not at the same time, but if both restrictions are active, we can be
sure that both will have an impact when using GDB.

> I was wondering if each case should just return, or if checking each one
> is the correct thing to do here.

I don't have a strong opinion here, but I think it's more useful for the
user if we print everything wrong in the first pass.  I'm thinking of
cases when starting GDB may take a while, for example: if we tell the
user a list of problems that need to be solved, then she won't need to
keep retrying.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/