Re: RFC: Check permissions of .gdbinit files

[Daniel Jacobowitz <drow@false.org>]

On Tue, May 31, 2005 at 12:54:53AM +0300, Eli Zaretskii wrote:
> > Date: Mon, 30 May 2005 14:52:01 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > Gentoo recently published a security update for GDB, citing the fact that
> > GDB would load .gdbinit from the current directory even if that was owned by
> > another user.  I'm not sure how I feel about running GDB in an untrusted
> > directory or on untrusted binaries and expecting it to behave sensibly, but
> > this particular issue is easy to fix.  Here's my suggested fix; it's not the
> > same as Gentoo's.  If .gdbinit is world writable or owned by a different
> > user, refuse to open it (and warn the user).
> > 
> > Anyone have opinions on this change?
> 
> Hmm... bother.  This change might break some of the non-Posix
> platforms.  Perhaps I missed something, but AFAICS MinGW lacks the
> definition of S_IWOTH, so this will fail to compile for MinGW.  But
> even if it did compile, the MinGW version of `fstat' returns the
> S_IWOTH bit set for all files, so the MinGW port will probably always
> display the warning.
> 
> The DJGPP port will not be affected: it does have S_IWOTH and it
> reports the S_IWOTH bit as reset.  Can't easily check Cygwin here, but
> I'm guessing it will do TRT here as well.

Bother; I thought about the portability for a while, but didn't quite
consider this.  We're still OK though - the whole thing is surrounded
by HAVE_GETUID, and MinGW does not have GETUID, if I understand
correctly.  I think it's plausible to assume that S_IWOTH will be
defined if getuid() is; does that seem reasonable to you?

> > +    error (_("source command requires pathname of file to source."));
> 
> I think the message text should begin with a capital letter (yes, I
> know the original didn't do it, either).

I was pretty sure that the convention was lowercase letter, and no
trailing period.  But I don't think this is documented anywhere, and
the source is wildly inconsistent.  I could easily be remembering wrong
:-)

> > -  stream = fopen (file, FOPEN_RT);
> > -  if (!stream)
> > +  fd = open (file, O_RDONLY);
> > +  if (fd != -1)
> > +    stream = fdopen (fd, FOPEN_RT);
> 
> Could you please tell why you replaced `fopen' with `open'+`fdopen'?

In order to have the file descriptor, for fstat.  It occurs to me now
that "fileno" could be used for this; I am not sure how portable fileno
is (my system's man page for it is somewhat ambiguous on the subject)
but I see that we use it already, so the answer is "portable enough". 
That would probably be cleaner.

> > +      if (statbuf.st_uid != getuid () || (statbuf.st_mode & S_IWOTH))
> 
> Shouldn't we allow this to go unnoticed for root?

No, that's especially when we shouldn't.  It's root that wants to be
paranoid about silently using some user's .gdbinit file.  This is the
opposite problem to access checks in a privileged program; we want no
more "privileges" for the superuser here.

> > +	  warning (_("not using untrusted file \"%s\""), file);
> 
> I think the message text should begin with a capital letter.
> 
> Last, but not least: if we decide to make such a change (which to my
> HO sounds like a good idea, in general), we should describe this
> subtlety (and the warning it could produce) in the user's manual.

Where would you suggest?  Ah, never mind, I see there is a section on
.gdbinit already.

-- 
Daniel Jacobowitz
CodeSourcery, LLC