Patch for checking file parameters to psql before password prompt

Started by Alastair Turnerabout 13 years ago6 messages
#1Alastair Turner
bell@ctrlf5.co.za
1 attachment(s)

Patch for the changes discussed in
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
attached (eventually ...)

In summary: If the input file (-f) doesn't exist or the ouput or log
files (-o and -l) can't be created psql exits before prompting for a
password.

Regards,

Alastair.

Attachments:

psql_check_file_params_before_login.patchapplication/octet-stream; name=psql_check_file_params_before_login.patchDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index 8ccd00d..2a324ae
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** int
*** 2049,2058 ****
  process_file(char *filename, bool single_txn, bool use_relative_path)
  {
  	FILE	   *fd;
! 	int			result;
! 	char	   *oldfilename;
! 	char		relpath[MAXPGPATH];
! 	PGresult   *res;
  
  	if (!filename)
  	{
--- 2049,2077 ----
  process_file(char *filename, bool single_txn, bool use_relative_path)
  {
  	FILE	   *fd;
! 	
! 	fd = get_fd_for_process(filename, use_relative_path);
! 	if (!fd)
! 	{
! 		return EXIT_FAILURE;
! 	}
! 
! 	return process_fd(fd, filename, single_txn);
! }
! 
! 
! 
! /*
!  * get_fd_for_process
!  *
!  * Open a file to read and process commands from, used by process_file and
!  * the -f startup option
!  */
! FILE *
! get_fd_for_process(char *filename, bool use_relative_path)
! {
! 	FILE	*fd;
! 	char	relpath[MAXPGPATH];
  
  	if (!filename)
  	{
*************** process_file(char *filename, bool single
*** 2085,2091 ****
  		if (!fd)
  		{
  			psql_error("%s: %s\n", filename, strerror(errno));
- 			return EXIT_FAILURE;
  		}
  	}
  	else
--- 2104,2109 ----
*************** process_file(char *filename, bool single
*** 2094,2099 ****
--- 2112,2133 ----
  		filename = "<stdin>";	/* for future error messages */
  	}
  
+ 	return fd;
+ }
+ 
+ 
+ 
+ /*
+  * process_fd
+  *
+  */
+ int
+ process_fd(FILE *fd, char *filename, bool single_txn)
+ {
+ 	int		result;
+ 	char		*oldfilename;
+ 	PGresult   	*res;
+ 
  	oldfilename = pset.inputfile;
  	pset.inputfile = filename;
  
*************** error:
*** 2132,2137 ****
--- 2166,2172 ----
  		fclose(fd);
  
  	pset.inputfile = oldfilename;
+ 
  	return result;
  }
  
diff --git a/src/bin/psql/command.h b/src/bin/psql/command.h
new file mode 100644
index f0bcea0..b44ea16
*** a/src/bin/psql/command.h
--- b/src/bin/psql/command.h
*************** extern backslashResult HandleSlashCmds(P
*** 28,33 ****
--- 28,35 ----
  				PQExpBuffer query_buf);
  
  extern int	process_file(char *filename, bool single_txn, bool use_relative_path);
+ extern FILE *	get_fd_for_process(char *filename, bool use_relative_path);
+ extern int	process_fd(FILE *fd, char *filename, bool single_txn);
  
  extern bool do_pset(const char *param,
  		const char *value,
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
new file mode 100644
index 1fcc47f..513e357
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** struct adhoc_opts
*** 65,70 ****
--- 65,71 ----
  	char	   *logfilename;
  	enum _actions action;
  	char	   *action_string;
+ 	FILE	   *fd;
  	bool		no_readline;
  	bool		no_psqlrc;
  	bool		single_txn;
*************** main(int argc, char *argv[])
*** 184,189 ****
--- 185,218 ----
  		pset.popt.topt.recordSep.separator_zero = false;
  	}
  
+ 
+ 	/*
+ 	 * Before asking the user for a password, sanity check those options which can be
+ 	 * checked without information leakage.
+ 	 * --ouput has been checked during setQFout called from parse_psql_option
+ 	 */	
+ 	if (options.logfilename)
+ 	{
+ 		pset.logfile = fopen(options.logfilename, "a");
+ 		if (!pset.logfile)
+ 		{
+ 			fprintf(stderr, _("%s: could not open log file \"%s\": %s\n"),
+ 					pset.progname, options.logfilename, strerror(errno));
+ 			exit(EXIT_FAILURE);
+ 		}
+ 	}
+ 	if (options.action == ACT_FILE)
+ 	{
+ 		options.fd = get_fd_for_process(options.action_string, false);
+ 		if (!options.fd)
+ 		{
+ 			/*
+ 			 * Reporting the error has already been done in get_fd_for_process
+ 			 */
+ 			exit(EXIT_FAILURE);
+ 		}
+ 	}
+ 
  	if (options.username == NULL)
  		password_prompt = pg_strdup(_("Password: "));
  	else
*************** main(int argc, char *argv[])
*** 265,278 ****
  		exit(success ? EXIT_SUCCESS : EXIT_FAILURE);
  	}
  
- 	if (options.logfilename)
- 	{
- 		pset.logfile = fopen(options.logfilename, "a");
- 		if (!pset.logfile)
- 			fprintf(stderr, _("%s: could not open log file \"%s\": %s\n"),
- 					pset.progname, options.logfilename, strerror(errno));
- 	}
- 
  	/*
  	 * Now find something to do
  	 */
--- 294,299 ----
*************** main(int argc, char *argv[])
*** 285,291 ****
  		if (!options.no_psqlrc)
  			process_psqlrc(argv[0]);
  
! 		successResult = process_file(options.action_string, options.single_txn, false);
  	}
  
  	/*
--- 306,312 ----
  		if (!options.no_psqlrc)
  			process_psqlrc(argv[0]);
  
! 		successResult = process_fd(options.fd, options.action_string, options.single_txn);
  	}
  
  	/*
*************** parse_psql_options(int argc, char *argv[
*** 453,459 ****
  				options->no_readline = true;
  				break;
  			case 'o':
! 				setQFout(optarg);
  				break;
  			case 'p':
  				options->port = pg_strdup(optarg);
--- 474,481 ----
  				options->no_readline = true;
  				break;
  			case 'o':
! 				if (!(setQFout(optarg)))
! 					exit(EXIT_FAILURE);
  				break;
  			case 'p':
  				options->port = pg_strdup(optarg);
#2Robert Haas
robertmhaas@gmail.com
In reply to: Alastair Turner (#1)
Re: Patch for checking file parameters to psql before password prompt

On Sun, Dec 2, 2012 at 6:37 AM, Alastair Turner <bell@ctrlf5.co.za> wrote:

Patch for the changes discussed in
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
attached (eventually ...)

In summary: If the input file (-f) doesn't exist or the ouput or log
files (-o and -l) can't be created psql exits before prompting for a
password.

s/ouput/output/

Please add this patch here so we don't lose track of it:

https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Josh Kupershmidt
schmiddy@gmail.com
In reply to: Alastair Turner (#1)
Re: Patch for checking file parameters to psql before password prompt

On Sun, Dec 2, 2012 at 4:37 AM, Alastair Turner <bell@ctrlf5.co.za> wrote:

Patch for the changes discussed in
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
attached (eventually ...)

In summary: If the input file (-f) doesn't exist or the ouput or log
files (-o and -l) can't be created psql exits before prompting for a
password.

I assume you meant "-L" instead of "-l" here for specifying psql's log
file. I don't think that the inability to write to psql's log file
should be treated as a fatal error, especially since it is not treated
as such by the current code:

$ psql test -L /tmp/not_allowed
psql: could not open log file "/tmp/not_allowed": Permission denied
[... proceeds to psql prompt from here ...]

and the user (or script) may still usefully perform his work. Whereas
with your patch:

$ psql test -L /tmp/not_allowed
psql: could not open log file "/tmp/not_allowed": Permission denied
$

And IMO the same concern applies to the query results file, "-o".
Although +1 for the part about having psql exit early if the input
filename does not exist, since psql already bails out in this case,
and there is nothing else to be done in such case.

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Stephen Frost
sfrost@snowman.net
In reply to: Josh Kupershmidt (#3)
Re: Patch for checking file parameters to psql before password prompt

Josh,

* Josh Kupershmidt (schmiddy@gmail.com) wrote:

I assume you meant "-L" instead of "-l" here for specifying psql's log
file. I don't think that the inability to write to psql's log file
should be treated as a fatal error, especially since it is not treated
as such by the current code:

I disagree- if we're being asked to log or to send output somewhere, we
should error out if we're unable to do so. Consider doing that from the
shell:

sfrost@beorn:~$ psql > /bin/qq
bash: /bin/qq: Permission denied
sfrost@beorn:~$

Thanks,

Stephen

#5Stephen Frost
sfrost@snowman.net
In reply to: Alastair Turner (#1)
Re: Patch for checking file parameters to psql before password prompt

Alastair,

* Alastair Turner (bell@ctrlf5.co.za) wrote:

Patch for the changes discussed in
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
attached (eventually ...)

In summary: If the input file (-f) doesn't exist or the ouput or log
files (-o and -l) can't be created psql exits before prompting for a
password.

On a once-over, the patch looks reasonably good. A couple things
though:

Please be good with the whitespace:

Above 'if (options.logfilename)' you introduce an extra \n, while you
don't have one between the end of that if() { } block and the next. You
also have a whole diff block that's just adding a \n (between
"pset.inputfile = oldfilename;" and "return result;"). Reviewing your
patches before sending them is a good way to find these things. :)

Silly stuff, sure, but since it's your first patch, I figured I'd
mention it. :)

Also, if you're doing the error-reporting in get_fd_for_process and then
every time it's called and returns failure immediately exiting, why not
just error-exit from get_fd_for_process directly..?

Lastly, I'm not convinced that how you broke up process_file() and
process_fd() actually works. Inside the existing process_file(),
filename will be updated to '<stdin>' for error reporting when the input
in 'stdin', but that's now lost in the new process_file() and
process_fd() will always get whatever is in options.action_string, which
could be a '-' instead. In reviewing the patch, I was hoping that
process_fd() wouldn't actually need to have the filename passed in with
the fd, but it does because psql_error() depends on pset.inputfile being
set, which has to be done by the code which calls into MainLoop(), which
is process_fd() with your patch.

Perhaps there's a better way to handle that?

Thanks,

Stephen

#6Greg Smith
greg@2ndQuadrant.com
In reply to: Stephen Frost (#5)
Re: Patch for checking file parameters to psql before password prompt

On 12/29/12 3:36 PM, Stephen Frost wrote:

Perhaps there's a better way to handle that?

Responding to feedback would be a nice start. This submissions has been
dead at "Waiting on Author" for at least 3 months now. Time to give it
the "Returned with Feedback" boot and see if it comes around again
later. I'll do the kicking myself now.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers