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

Doug Evans
2010-11-23T23:19:17Z

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.