Rajeev rastogi <rajeev.rast...@huawei.com> writes:
> One suggestion:
> Instead of using sizeof(cmdLine),
>       a. Can't we use strlen  (hence small 'for' loop).
>       b. Or use memmove to move one byte. 

I looked at this patch a bit.  I agree that we need to fix
pgwin32_CommandLine to double-quote the executable name, but it needs a
great deal more work than that :-(.  Whoever wrote this code was
apparently unacquainted with the concept of buffer overrun.  It's not
going to be hard at all to crash pg_ctl with overlength arguments.  I'm
not sure that that amounts to a security bug, but it's certainly bad.

After some thought it seems like the most future-proof fix is to not
use a fixed-length buffer for the command string at all.  The attached
revised patch switches it over to using a PQExpBuffer instead, which is
pretty much free since we're relying on libpq anyway in this program.
(We still use a fixed-length buffer for the program path, which is OK
because that's what find_my_exec and find_other_exec expect.)

In addition, I fixed it to append .exe in both cases not just the one.

I'm not in a position to actually test this, but it does compile
without warnings.

                        regards, tom lane

diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 8399cdd..dd80719 100644
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 18,24 ****
--- 18,26 ----
  #endif
  
  #include "postgres_fe.h"
+ 
  #include "libpq-fe.h"
+ #include "pqexpbuffer.h"
  
  #include <fcntl.h>
  #include <locale.h>
*************** pgwin32_IsInstalled(SC_HANDLE hSCM)
*** 1238,1253 ****
  static char *
  pgwin32_CommandLine(bool registration)
  {
! 	static char cmdLine[MAXPGPATH];
  	int			ret;
  
- #ifdef __CYGWIN__
- 	char		buf[MAXPGPATH];
- #endif
- 
  	if (registration)
  	{
! 		ret = find_my_exec(argv0, cmdLine);
  		if (ret != 0)
  		{
  			write_stderr(_("%s: could not find own program executable\n"), progname);
--- 1240,1252 ----
  static char *
  pgwin32_CommandLine(bool registration)
  {
! 	PQExpBuffer cmdLine = createPQExpBuffer();
! 	char		cmdPath[MAXPGPATH];
  	int			ret;
  
  	if (registration)
  	{
! 		ret = find_my_exec(argv0, cmdPath);
  		if (ret != 0)
  		{
  			write_stderr(_("%s: could not find own program executable\n"), progname);
*************** pgwin32_CommandLine(bool registration)
*** 1257,1263 ****
  	else
  	{
  		ret = find_other_exec(argv0, "postgres", PG_BACKEND_VERSIONSTR,
! 							  cmdLine);
  		if (ret != 0)
  		{
  			write_stderr(_("%s: could not find postgres program executable\n"), progname);
--- 1256,1262 ----
  	else
  	{
  		ret = find_other_exec(argv0, "postgres", PG_BACKEND_VERSIONSTR,
! 							  cmdPath);
  		if (ret != 0)
  		{
  			write_stderr(_("%s: could not find postgres program executable\n"), progname);
*************** pgwin32_CommandLine(bool registration)
*** 1267,1320 ****
  
  #ifdef __CYGWIN__
  	/* need to convert to windows path */
  #if CYGWIN_VERSION_DLL_MAJOR >= 1007
! 	cygwin_conv_path(CCP_POSIX_TO_WIN_A, cmdLine, buf, sizeof(buf));
  #else
! 	cygwin_conv_to_full_win32_path(cmdLine, buf);
  #endif
! 	strcpy(cmdLine, buf);
  #endif
  
  	if (registration)
! 	{
! 		if (pg_strcasecmp(cmdLine + strlen(cmdLine) - 4, ".exe") != 0)
! 		{
! 			/* If commandline does not end in .exe, append it */
! 			strcat(cmdLine, ".exe");
! 		}
! 		strcat(cmdLine, " runservice -N \"");
! 		strcat(cmdLine, register_servicename);
! 		strcat(cmdLine, "\"");
! 	}
  
  	if (pg_config)
! 	{
! 		strcat(cmdLine, " -D \"");
! 		strcat(cmdLine, pg_config);
! 		strcat(cmdLine, "\"");
! 	}
  
  	if (registration && do_wait)
! 		strcat(cmdLine, " -w");
  
  	if (registration && wait_seconds != DEFAULT_WAIT)
! 		/* concatenate */
! 		sprintf(cmdLine + strlen(cmdLine), " -t %d", wait_seconds);
  
  	if (registration && silent_mode)
! 		strcat(cmdLine, " -s");
  
  	if (post_opts)
  	{
- 		strcat(cmdLine, " ");
- 		if (registration)
- 			strcat(cmdLine, " -o \"");
- 		strcat(cmdLine, post_opts);
  		if (registration)
! 			strcat(cmdLine, "\"");
  	}
  
! 	return cmdLine;
  }
  
  static void
--- 1266,1319 ----
  
  #ifdef __CYGWIN__
  	/* need to convert to windows path */
+ 	{
+ 		char		buf[MAXPGPATH];
+ 
  #if CYGWIN_VERSION_DLL_MAJOR >= 1007
! 		cygwin_conv_path(CCP_POSIX_TO_WIN_A, cmdPath, buf, sizeof(buf));
  #else
! 		cygwin_conv_to_full_win32_path(cmdPath, buf);
  #endif
! 		strcpy(cmdPath, buf);
! 	}
  #endif
  
+ 	/* if path does not end in .exe, append it */
+ 	if (strlen(cmdPath) < 4 ||
+ 		pg_strcasecmp(cmdPath + strlen(cmdPath) - 4, ".exe") != 0)
+ 		snprintf(cmdPath + strlen(cmdPath), sizeof(cmdPath) - strlen(cmdPath),
+ 				 ".exe");
+ 
+ 	/* be sure to double-quote the executable's name in the command */
+ 	appendPQExpBuffer(cmdLine, "\"%s\"", cmdPath);
+ 
+ 	/* append assorted switches to the command line, as needed */
+ 
  	if (registration)
! 		appendPQExpBuffer(cmdLine, " runservice -N \"%s\"",
! 						  register_servicename);
  
  	if (pg_config)
! 		appendPQExpBuffer(cmdLine, " -D \"%s\"", pg_config);
  
  	if (registration && do_wait)
! 		appendPQExpBuffer(cmdLine, " -w");
  
  	if (registration && wait_seconds != DEFAULT_WAIT)
! 		appendPQExpBuffer(cmdLine, " -t %d", wait_seconds);
  
  	if (registration && silent_mode)
! 		appendPQExpBuffer(cmdLine, " -s");
  
  	if (post_opts)
  	{
  		if (registration)
! 			appendPQExpBuffer(cmdLine, " -o \"%s\"", post_opts);
! 		else
! 			appendPQExpBuffer(cmdLine, " %s", post_opts);
  	}
  
! 	return cmdLine->data;
  }
  
  static void
*************** CreateRestrictedProcess(char *cmd, PROCE
*** 1745,1751 ****
  	 */
  	return r;
  }
! #endif
  
  static void
  do_advice(void)
--- 1744,1750 ----
  	 */
  	return r;
  }
! #endif	/* defined(WIN32) || defined(__CYGWIN__) */
  
  static void
  do_advice(void)
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to