Re: [RFA] .gdbinit security (revived) [incl doc]

[Doug Evans <dje at google dot com>]

On Tue, Nov 23, 2010 at 10:26 AM, Keith Seitz <keiths@redhat.com> wrote:
> [...]
> ChangeLog
> 2010-11-23 ?Keith Seitz ?<keiths@redhat.com>
>
> ? ? ? ?From ?Daniel Jacobowitz ?<dan@codesourcery.com>
> ? ? ? ?and Jeff Johnston ?<jjohnstn@redhat.com>:
> ? ? ? ?* cli/cli-cmds.h (find_and_open_script): Add from_tty argument.
> ? ? ? ?* cli/cli-cmds.c (find_and_open_script): Likewise. ?When
> ? ? ? ?from_tty is -1, perform a security check of the file. ?If it
> ? ? ? ?fails, warn the user and whether he wants to read the file anyway.
> ? ? ? ?(source_script_with_search): Update call to find_and_open_script.
> ? ? ? ?Only print an error if from_tty is greater than zero.
> ? ? ? ?* main.c (captured_main): Pass from_tty = -1 when sourcing
> ? ? ? ?gdbinit files.
> ? ? ? ?* python/py-auto-load.c (source_section_scripts): Update call
> ? ? ? ?to find_and_open_script.
>
> doc/ChangeLog
> 2010-11-23 ?Keith Seitz ?<keiths@redhat.com>
>
> ? ? ? ?* gdb.texinfo (Startup): Document security handling of
> ? ? ? ?.gdbinit files.

Hi.
A few comments inline.

>-    catch_command_errors (source_script, home_gdbinit, 0, RETURN_MASK_ALL);
>+    catch_command_errors (source_script, home_gdbinit, -1, RETURN_MASK_ALL);

I don't mind using -1 for from_tty here  (especially if there is
precedent :-)), but a #define/enum would be nicer.
catch_command_errors has a limited API so overloading from_tty is a
pragmatic tradeoff.
Feel free to save for a separate patch.  Just mentioning it to prime
the pumps doing something like this down the road.

>+   If FROM_TTY is -1, then this script is being automatically loaded
>+   at runtime, and a security check will be performed on the file
>+   (supported only on hosts with HAVE_GETUID).

We're combining two concepts here: "is the command from the tty?" and
"do security checks?".
IWBN to keep them separate here.
Maybe specify both separately or just have check_security instead of from_tty?

>+	  if (statbuf.st_uid != getuid ())

I wonder if you also need to watch for file owner == root (and not
world writable).  E.g. scripts like --with-system-gdbinit.
That won't happen with the patch as is, but that feels like a
high-level detail that this function shouldn't have to know about.
Then again, why not do this security check for system.gdbinit too?

>       opened = find_and_open_script (file, 1 /*search_path*/,
>-				     &stream, &full_path);
>+				     &stream, &full_path, 1 /* from_tty */);

Passing 1 for from_tty feels wrong here.
If find_and_open_script had a check_security parameter instead of
from_tty, then one could just pass 0 here.