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

Sergio Durigan Junior
2019-09-25T14:14:19Z

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