scrappy · scrappy · commit bd029bcb4a11 · 1998-07-09T03:32:10.000Z
From: Tom Lane <tgl@sss.pgh.pa.us> The attached patches respond to discussion that was on pgsql-hackers around the beginning of June (see thread "libpgtcl bug (and symptomatic treatment)"). The changes are: 1. Remove code in connectDB that throws away the password after making a connection. This doesn't really add much security IMHO --- a bad guy with access to your client's address space can likely extract the password anyway, to say nothing of what he might do directly. And there's the serious shortcoming that it prevents PQreset() from working if the database requires a password. 2. Fix coredump problem: fe_sendauth did not guard against being handed a NULL password pointer. (This is the proximate cause of the coredump- during-PQreset problem that Magosanyi Arpad complained of last month.) 3. Remove highly questionable "error recovery" logic in libpgtcl's pg_exec statement. I believe the consensus of the discussion last month was in favor of #1 and #3, but I'm just now getting around to making the change. I realized that #2 was a bug in process of looking at the change.
diff --git a/src/interfaces/libpgtcl/pgtclCmds.c b/src/interfaces/libpgtcl/pgtclCmds.c
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/interfaces/libpgtcl/Attic/pgtclCmds.c,v 1.27 1998/06/16 06:53:27 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/interfaces/libpgtcl/Attic/pgtclCmds.c,v 1.28 1998/07/09 03:32:09 scrappy Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -441,24 +441,7 @@ Pg_exec(ClientData cData, Tcl_Interp *interp, int argc, char* argv[])
     }
     else {
 	/* error occurred during the query */
-	Tcl_SetResult(interp, conn->errorMessage, TCL_STATIC);
-	if (connStatus != CONNECTION_OK) {
-	    /* Is this REALLY a good idea?  I don't think so! */
-	    PQreset(conn);
-	    if (conn->status == CONNECTION_OK) {
-		result = PQexec(conn, argv[2]);
-		PgNotifyTransferEvents(connid);
-		if (result) {
-		    int rId = PgSetResultId(interp, argv[1], result);
-		    if (result->resultStatus == PGRES_COPY_IN ||
-			result->resultStatus == PGRES_COPY_OUT) {
-			    connid->res_copyStatus = RES_COPY_INPROGRESS;
-			    connid->res_copy = rId;
-		    }
-		    return TCL_OK;
-		}
-	    }
-	}
+	Tcl_SetResult(interp, conn->errorMessage, TCL_VOLATILE);
 	return TCL_ERROR;
     }
 }
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v 1.18 1998/07/03 04:24:11 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v 1.19 1998/07/09 03:32:09 scrappy Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -522,6 +522,12 @@ fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname,
 
 		case AUTH_REQ_PASSWORD:
 		case AUTH_REQ_CRYPT:
+			if (password == NULL || *password == '\0')
+			{
+				(void) sprintf(PQerrormsg,
+				 "fe_sendauth: no password supplied\n");
+				return (STATUS_ERROR);
+			}
 			if (pg_password_sendauth(conn, password, areq) != STATUS_OK)
 			{
 				(void) sprintf(PQerrormsg,
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v 1.73 1998/07/09 03:29:07 scrappy Exp $
+ *	  $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v 1.74 1998/07/09 03:32:10 scrappy Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -768,17 +768,6 @@ connectDB(PGconn *conn)
 
 	PQsetenv(conn);
 
-	/* Free the password so it's not hanging out in memory forever */
-	/* XXX Is this *really* a good idea?  The security gain is marginal
-	 * if not totally illusory, and it breaks PQreset() for databases
-	 * that use passwords.
-	 */
-	if (conn->pgpass != NULL)
-	{
-		free(conn->pgpass);
-		conn->pgpass = NULL;
-	}
-
 	return CONNECTION_OK;
 
 connect_errReturn:

Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`*`
`8`	`8`	`*`
`9`	`9`	`* IDENTIFICATION`
`10`		`- * $Header: /cvsroot/pgsql/src/interfaces/libpgtcl/Attic/pgtclCmds.c,v 1.27 1998/06/16 06:53:27 momjian Exp $`
	`10`	`+ * $Header: /cvsroot/pgsql/src/interfaces/libpgtcl/Attic/pgtclCmds.c,v 1.28 1998/07/09 03:32:09 scrappy Exp $`
`11`	`11`	`*`
`12`	`12`	`*-------------------------------------------------------------------------`
`13`	`13`	`*/`
`@@ -441,24 +441,7 @@ Pg_exec(ClientData cData, Tcl_Interp interp, int argc, char argv[])`
`441`	`441`	`}`
`442`	`442`	`else {`
`443`	`443`	`/* error occurred during the query */`
`444`		`- Tcl_SetResult(interp, conn->errorMessage, TCL_STATIC);`
`445`		`- if (connStatus != CONNECTION_OK) {`
`446`		`- /* Is this REALLY a good idea? I don't think so! */`
`447`		`- PQreset(conn);`
`448`		`- if (conn->status == CONNECTION_OK) {`
`449`		`- result = PQexec(conn, argv[2]);`
`450`		`- PgNotifyTransferEvents(connid);`
`451`		`- if (result) {`
`452`		`- int rId = PgSetResultId(interp, argv[1], result);`
`453`		`- if (result->resultStatus == PGRES_COPY_IN \|\|`
`454`		`- result->resultStatus == PGRES_COPY_OUT) {`
`455`		`- connid->res_copyStatus = RES_COPY_INPROGRESS;`
`456`		`- connid->res_copy = rId;`
`457`		`- }`
`458`		`- return TCL_OK;`
`459`		`- }`
`460`		`- }`
`461`		`- }`
	`444`	`+ Tcl_SetResult(interp, conn->errorMessage, TCL_VOLATILE);`
`462`	`445`	`return TCL_ERROR;`
`463`	`446`	`}`
`464`	`447`	`}`