Patch applied for SQL Injection vulnerability for setObject(int,Object,int)

[Barry Lind <blind(at)xythos(dot)com>]

I have applied a fix for the reported SQL injection vulnerability in 
setObject for both 7.3 and 7.4.  I have also places new builds with this 
bugfix on the jdbc.postgresql.org website.

For the record, the vulnerability was limited to the 
setObject(int,Object,int) method where Object is a String and the type 
is a numeric type (i.e. SqlTypes.INTEGER, SqlTypes.LONG, etc)

Given the ongoing discussion that this SQL injection vulnerability has 
caused, I decided not to apply the below patch from Kim and instead 
fixed the problem in a different way.  The fix essentially applies the 
regular escaping done for setString to appropriate values passed to 
setObject.  It does not however add quotes to the value.  Thus existing 
uses of setObject for in clause and array type values will still 
continue to work.

I didn't want to break backward compatibility in a patch to 7.3 and 
didn't want to change the functionality in 7.4 until the current 
discussions come to a conclusion.

thanks,
--Barry

PS.  I have not yet uploaded new jdbc1 builds to the website.  As I have 
recently upgraded to RH9, my jdk1.1 environment no longer works and I 
will need to do this build on a different machine tomorrow.


Kim Ho wrote:
> To speed things up a bit, since the regoutParam patch is not likely to
> be approved anytime soon.
>
> This patch 
> - adds single quotes for numbers in setObject and also setInt/Byte/etc.
> - Improves getInt/Long when you may have parser errors if you're too
> close to Integer.MIN_VALUE or Integer.MAX_VALUE. Thanks to Fujitsu.
> - Improves radix point handling when using setObject to an integer
> parameter while passing in a float. This is especially important in
> callable statements.
>
> Cheers,
>
> Kim
>
> On Fri, 2003-07-18 at 12:51, Fernando Nasser wrote:
>
> > Barry Lind wrote:
> >
> > > Dmitry,
> > >
> > > That is a bug.  Thanks for pointing it out.  Anyone care to submit a patch?
> >
> > Kim's patch fixes this.  It is pending approval.
> >
> >
> >
> > --
> > Fernando Nasser
> > Red Hat Canada Ltd.                     E-Mail:  fnasser(at)redhat(dot)com
> > 2323 Yonge Street, Suite #300
> > Toronto, Ontario   M4P 2C9
> >
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
>
>
>
> ------------------------------------------------------------------------
>
> ? temp.diff
> Index: org/postgresql/jdbc1/AbstractJdbc1ResultSet.java
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/jdbc/org/postgresql/jdbc1/AbstractJdbc1ResultSet.java,v
> retrieving revision 1.13
> diff -c -p -r1.13 AbstractJdbc1ResultSet.java
> *** org/postgresql/jdbc1/AbstractJdbc1ResultSet.java	30 Jun 2003 21:10:55 -0000	1.13
> --- org/postgresql/jdbc1/AbstractJdbc1ResultSet.java	18 Jul 2003 17:02:20 -0000
> *************** public abstract class AbstractJdbc1Resul
> *** 805,811 ****
>   			try
>   			{
>   				s = s.trim();
> ! 				return Integer.parseInt(s);
>   			}
>   			catch (NumberFormatException e)
>   			{
> --- 805,811 ----
>   			try
>   			{
>   				s = s.trim();
> ! 				return Float.valueOf(s).intValue();
>   			}
>   			catch (NumberFormatException e)
>   			{
> *************** public abstract class AbstractJdbc1Resul
> *** 822,828 ****
>   			try
>   			{
>   				s = s.trim();		
> ! 				return Long.parseLong(s);
>   			}
>   			catch (NumberFormatException e)
>   			{
> --- 822,828 ----
>   			try
>   			{
>   				s = s.trim();		
> ! 				return Double.valueOf(s).longValue();
>   			}
>   			catch (NumberFormatException e)
>   			{
> Index: org/postgresql/jdbc1/AbstractJdbc1Statement.java
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/jdbc/org/postgresql/jdbc1/AbstractJdbc1Statement.java,v
> retrieving revision 1.27
> diff -c -p -r1.27 AbstractJdbc1Statement.java
> *** org/postgresql/jdbc1/AbstractJdbc1Statement.java	9 Jul 2003 05:12:04 -0000	1.27
> --- org/postgresql/jdbc1/AbstractJdbc1Statement.java	18 Jul 2003 17:02:22 -0000
> *************** public abstract class AbstractJdbc1State
> *** 920,926 ****
>   	 */
>   	public void setByte(int parameterIndex, byte x) throws SQLException
>   	{
> ! 		bind(parameterIndex, Integer.toString(x), PG_TEXT);
>   	}
>   
>   	/*
> --- 920,926 ----
>   	 */
>   	public void setByte(int parameterIndex, byte x) throws SQLException
>   	{
> ! 		bind(parameterIndex, "'" + Integer.toString(x) + "'", PG_TEXT);
>   	}
>   
>   	/*
> *************** public abstract class AbstractJdbc1State
> *** 933,939 ****
>   	 */
>   	public void setShort(int parameterIndex, short x) throws SQLException
>   	{
> ! 		bind(parameterIndex, Integer.toString(x), PG_INT2);
>   	}
>   
>   	/*
> --- 933,939 ----
>   	 */
>   	public void setShort(int parameterIndex, short x) throws SQLException
>   	{
> ! 		bind(parameterIndex, "'" + Integer.toString(x) + "'" , PG_INT2);
>   	}
>   
>   	/*
> *************** public abstract class AbstractJdbc1State
> *** 946,952 ****
>   	 */
>   	public void setInt(int parameterIndex, int x) throws SQLException
>   	{
> ! 		bind(parameterIndex, Integer.toString(x), PG_INTEGER);
>   	}
>   
>   	/*
> --- 946,952 ----
>   	 */
>   	public void setInt(int parameterIndex, int x) throws SQLException
>   	{
> ! 		bind(parameterIndex, "'" + Integer.toString(x) + "'", PG_INTEGER);
>   	}
>   
>   	/*
> *************** public abstract class AbstractJdbc1State
> *** 959,965 ****
>   	 */
>   	public void setLong(int parameterIndex, long x) throws SQLException
>   	{
> ! 		bind(parameterIndex, Long.toString(x), PG_INT8);
>   	}
>   
>   	/*
> --- 959,965 ----
>   	 */
>   	public void setLong(int parameterIndex, long x) throws SQLException
>   	{
> ! 		bind(parameterIndex, "'" + Long.toString(x) + "'", PG_INT8);
>   	}
>   
>   	/*
> *************** public abstract class AbstractJdbc1State
> *** 972,978 ****
>   	 */
>   	public void setFloat(int parameterIndex, float x) throws SQLException
>   	{
> ! 		bind(parameterIndex, Float.toString(x), PG_FLOAT);
>   	}
>   
>   	/*
> --- 972,978 ----
>   	 */
>   	public void setFloat(int parameterIndex, float x) throws SQLException
>   	{
> ! 		bind(parameterIndex, "'" + Float.toString(x) + "'", PG_FLOAT);
>   	}
>   
>   	/*
> *************** public abstract class AbstractJdbc1State
> *** 985,991 ****
>   	 */
>   	public void setDouble(int parameterIndex, double x) throws SQLException
>   	{
> ! 		bind(parameterIndex, Double.toString(x), PG_DOUBLE);
>   	}
>   
>   	/*
> --- 985,991 ----
>   	 */
>   	public void setDouble(int parameterIndex, double x) throws SQLException
>   	{
> ! 		bind(parameterIndex, "'" + Double.toString(x) + "'", PG_DOUBLE);
>   	}
>   
>   	/*
> *************** public abstract class AbstractJdbc1State
> *** 1003,1009 ****
>   			setNull(parameterIndex, Types.DECIMAL);
>   		else
>   		{
> ! 			bind(parameterIndex, x.toString(), PG_NUMERIC);
>   		}
>   	}
>   
> --- 1003,1009 ----
>   			setNull(parameterIndex, Types.DECIMAL);
>   		else
>   		{
> ! 			bind(parameterIndex, "'" + x.toString() + "'", PG_NUMERIC);
>   		}
>   	}
>   
> *************** public abstract class AbstractJdbc1State
> *** 1464,1486 ****
>   		switch (targetSqlType)
>   		{
>   			case Types.INTEGER:
> - 				if (x instanceof Boolean)
> - 					bind(parameterIndex,((Boolean)x).booleanValue() ? "1" :"0", PG_BOOLEAN);
> - 				else
> - 					bind(parameterIndex, x.toString(), PG_INTEGER);
> - 				break;
>   			case Types.TINYINT:
>   			case Types.SMALLINT:
>   			case Types.BIGINT:
>   			case Types.REAL:
>   			case Types.FLOAT:
>   			case Types.DOUBLE:
>   			case Types.DECIMAL:
>   			case Types.NUMERIC:
> ! 				if (x instanceof Boolean)
> ! 					bind(parameterIndex, ((Boolean)x).booleanValue() ? "1" : "0", PG_BOOLEAN);
> ! 				else
> ! 					bind(parameterIndex, x.toString(), PG_NUMERIC);
>   				break;
>   			case Types.CHAR:
>   			case Types.VARCHAR:
> --- 1464,1484 ----
>   		switch (targetSqlType)
>   		{
>   			case Types.INTEGER:
>   			case Types.TINYINT:
>   			case Types.SMALLINT:
> + 				x = removeRadix(x,Types.INTEGER);
> + 				bindNumber(parameterIndex,x,PG_INTEGER);
> + 				break;
>   			case Types.BIGINT:
> + 				x = removeRadix(x,Types.BIGINT);
> + 				bindNumber(parameterIndex,x,PG_INT8);
> + 				break;
>   			case Types.REAL:
>   			case Types.FLOAT:
>   			case Types.DOUBLE:
>   			case Types.DECIMAL:
>   			case Types.NUMERIC:
> ! 				bindNumber(parameterIndex,x,PG_NUMERIC);
>   				break;
>   			case Types.CHAR:
>   			case Types.VARCHAR:
> *************** public abstract class AbstractJdbc1State
> *** 2026,2031 ****
> --- 2024,2056 ----
>   		if (parameterIndex != 1)
>   			throw new PSQLException("postgresql.call.noinout");
>   	}
> + 	
> + 	private void bindNumber(int parameterIndex, Object x, String pgtype) throws SQLException
> + 	{
> + 		if (x instanceof Boolean)
> + 			bind(parameterIndex,((Boolean)x).booleanValue() ? "'1'" :"'0'", pgtype);
> + 		else
> + 			bind(parameterIndex, "'"+x.toString()+"'", pgtype);
> + 	}
> + 	
> + 
> + 	private Object removeRadix(Object x, int sqlType)
> + 	{
> + 		if (x.toString().indexOf(".")>0)
> + 		{
> + 			switch (sqlType)
> + 			{
> + 				case Types.BIGINT:
> + 					x = String.valueOf(Double.valueOf(x.toString()).longValue());
> + 					break;
> + 				default:
> + 					x = String.valueOf(Float.valueOf(x.toString()).intValue());
> + 					break;
> + 			}
> + 		}
> + 		return x;
> + 	}
> + 	
>   
>   
>   
>
>
> ------------------------------------------------------------------------
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
>       subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
>       message can get through to the mailing list cleanly