This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi, Eli,

On 11/20/2010 01:42 AM, Eli Zaretskii wrote:
In that discussion, Andreas suggested to avoid the warning if the user
belongs to the same group as the file's owner.  I don't see your patch
addressing that part.  Why not?

It was not clear to me that this was generally considered desirable. Daniel noted, "I'm trying not to encode too much site policy
into GDB".


I can add this, but to be honest, I'm no longer certain how to write a test for this, since it is not possible (AFAIK) to change permissions of a file in the test suite that would cause this to trigger.

I realize that it would be inappropriate to ask you to do that as a
prerequisite for accepting the patch, but maybe a TODO comment should
be placed there about the Windows case.  Then someone else could do
that at some point.

Done.


I would suggest to spell out why it is untrusted.  Otherwise the
warning sounds grave, but doesn't give enough information to make the
decision.

Done.



+ if (!query (_("Read file anyway? ")))

This could be automatically answered YES in some situations. Do we care?

Good question. I always seem to forget that the user can do this. To err on the side of safety, I've changed to using nquery.


+If @file{.gdbinit} is untrusted (it is not owned by the current user
+or the file is world-writable), @value{GDBN} will warn the user and ask

This should be qualified by "on some platforms", because not every platform that supports file ownership will issue this warning.

Yes, indeed. Fixed.


And a minor stylistic issue.  You say "it is not owned" and then "the
file is world-writable".  This is inconsistent, and could confuse the
reader into thinking that "it" and "the file" are two different
things.  Suggest to rephrase:

   If @file{.gdbinit} is @dfn{untrusted} (either not owned by the
   current user or world-writable), ...

You are absolutely correct. I've added a bit about the group ID addition and reworded this to help readability (I hope):


"On some platorms, @value{GDBN} will perform security check of
@file{.gdbinit} before it is executed. If @file{.gdbinit} is not owned
by the current user or the file is world-writable, @value{GDBN} will
warn the user and ask if the file should be read anyway. These warnings are suppressed when the group ID of the file's owner matches the group ID of the user."


I've attached an updated version of the patch (without the test case, which I don't think can work without some sort of administrative permissions).

Keith

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.

Attachment: gdbinit-security-2.patch
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]