Skip to content

Commit

Permalink
stop creating parameter prefixed names unless we really need the stri…
Browse files Browse the repository at this point in the history 
…ng (dotnet#1829)
  • Loading branch information
@Wraith2 @kant2002
Wraith2 authored and kant2002 committed Jun 29, 2023
1 parent 8e233bd commit 33f48ad
Show file tree
Hide file tree
Showing 6 changed files with 175 additions and 62 deletions.
50 changes: 33 additions & 17 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Expand Up @@ -4363,7 +4363,7 @@ private void ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDi
SqlParameter sqlParameter = rpc.userParams[index];
Debug.Assert(sqlParameter != null, "sqlParameter should not be null.");

if (sqlParameter.ParameterNameFixed.Equals(parameterName, StringComparison.Ordinal))
if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName,parameterName,StringComparison.Ordinal))
{
Debug.Assert(sqlParameter.CipherMetadata == null, "param.CipherMetadata should be null.");
sqlParameter.HasReceivedMetadata = true;
Expand Down Expand Up @@ -5457,7 +5457,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
{
if (rec.tdsType != TdsEnums.SQLBIGVARBINARY)
{
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.ParameterNameFixed, rec.tdsType, TdsEnums.SQLBIGVARBINARY);
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.GetPrefixedParameterName(), rec.tdsType, TdsEnums.SQLBIGVARBINARY);
}

// Decrypt the ciphertext
Expand Down Expand Up @@ -5487,7 +5487,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
}
catch (Exception e)
{
throw SQL.ParamDecryptionFailed(thisParam.ParameterNameFixed, null, e);
throw SQL.ParamDecryptionFailed(thisParam.GetPrefixedParameterName(), null, e);
}
}
else
Expand Down Expand Up @@ -5628,7 +5628,11 @@ private SqlParameterCollection GetCurrentParameterCollection()
{
thisParam = parameters[i];
// searching for Output or InputOutput or ReturnValue with matching name
if (thisParam.Direction != ParameterDirection.Input && thisParam.Direction != ParameterDirection.ReturnValue && paramName == thisParam.ParameterNameFixed)
if (
thisParam.Direction != ParameterDirection.Input &&
thisParam.Direction != ParameterDirection.ReturnValue &&
SqlParameter.ParameterNamesEqual(paramName, thisParam.ParameterName,StringComparison.Ordinal)
)
{
foundParam = true;
break; // found it
Expand Down Expand Up @@ -5999,11 +6003,11 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

// Find the return value parameter (if any).
SqlParameter returnValueParameter = null;
foreach (SqlParameter parameter in parameters)
foreach (SqlParameter param in parameters)
{
if (parameter.Direction == ParameterDirection.ReturnValue)
if (param.Direction == ParameterDirection.ReturnValue)
{
returnValueParameter = parameter;
returnValueParameter = param;
break;
}
}
Expand All @@ -6012,7 +6016,8 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// EXEC @returnValue = moduleName [parameters]
if (returnValueParameter != null)
{
execStatement.AppendFormat(@"{0}=", returnValueParameter.ParameterNameFixed);
SqlParameter.AppendPrefixedParameterName(execStatement, returnValueParameter.ParameterName);
execStatement.Append('=');
}

execStatement.Append(ParseAndQuoteIdentifier(storedProcedureName, false));
Expand All @@ -6023,6 +6028,7 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the first parameter
int index = 0;
int count = parameters.Count;
SqlParameter parameter;
if (count > 0)
{
// Skip the return value parameters.
Expand All @@ -6033,15 +6039,19 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

if (index < count)
{
parameter = parameters[index];
// Possibility of a SQL Injection issue through parameter names and how to construct valid identifier for parameters.
// Since the parameters comes from application itself, there should not be a security vulnerability.
// Also since the query is not executed, but only analyzed there is no possibility for elevation of privilege, but only for
// incorrect results which would only affect the user that attempts the injection.
execStatement.AppendFormat(@" {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(' ');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput)
if (parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput)
{
execStatement.AppendFormat(@" OUTPUT");
}
Expand All @@ -6054,14 +6064,18 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the rest of parameters
for (; index < count; index++)
{
if (parameters[index].Direction != ParameterDirection.ReturnValue)
parameter = parameters[index];
if (parameter.Direction != ParameterDirection.ReturnValue)
{
execStatement.AppendFormat(@", {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(", ");
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (
parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput
parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput
)
{
execStatement.AppendFormat(@" OUTPUT");
Expand Down Expand Up @@ -6095,9 +6109,11 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete

// add our separator for the ith parameter
if (fAddSeparator)
{
paramList.Append(',');
}

paramList.Append(sqlParam.ParameterNameFixed);
SqlParameter.AppendPrefixedParameterName(paramList, sqlParam.ParameterName);

MetaType mt = sqlParam.InternalMetaType;

Expand All @@ -6120,7 +6136,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
string typeName = sqlParam.TypeName;
if (string.IsNullOrEmpty(typeName))
{
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.ParameterNameFixed);
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.GetPrefixedParameterName());
}
paramList.Append(ParseAndQuoteIdentifier(typeName, false /* is not UdtTypeName*/));

Expand Down
33 changes: 25 additions & 8 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Expand Up @@ -9312,7 +9312,7 @@ private Task TDSExecuteRPCAddParameter(TdsParserStateObject stateObj, SqlParamet
}
}

WriteParameterName(param.ParameterNameFixed, stateObj, isAnonymous);
WriteParameterName(param.ParameterName, stateObj, isAnonymous);

// Write parameter status
stateObj.WriteByte(options);
Expand Down Expand Up @@ -9833,17 +9833,34 @@ private void ExecuteFlushTaskCallback(Task tsk, TdsParserStateObject stateObj, T
}
}


private void WriteParameterName(string parameterName, TdsParserStateObject stateObj, bool isAnonymous)
/// <summary>
/// Will check the parameter name for the required @ prefix and then write the correct prefixed
/// form and correct character length to the output buffer
/// </summary>
private void WriteParameterName(string rawParameterName, TdsParserStateObject stateObj, bool isAnonymous)
{
// paramLen
// paramName
if (!isAnonymous && !string.IsNullOrEmpty(parameterName))
if (!isAnonymous && !string.IsNullOrEmpty(rawParameterName))
{
Debug.Assert(parameterName.Length <= 0xff, "parameter name can only be 255 bytes, shouldn't get to TdsParser!");
int tempLen = parameterName.Length & 0xff;
stateObj.WriteByte((byte)tempLen);
WriteString(parameterName, tempLen, 0, stateObj);
int nameLength = rawParameterName.Length;
int totalLength = nameLength;
bool writePrefix = false;
if (nameLength > 0)
{
if (rawParameterName[0] != '@')
{
writePrefix = true;
totalLength += 1;
}
}
Debug.Assert(totalLength <= 0xff, "parameter name can only be 255 bytes, shouldn't get to TdsParser!");
stateObj.WriteByte((byte)(totalLength & 0xFF));
if (writePrefix)
{
WriteString("@", 1, 0, stateObj);
}
WriteString(rawParameterName, nameLength, 0, stateObj);
}
else
{
Expand Down
53 changes: 34 additions & 19 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Expand Up @@ -4881,7 +4881,7 @@ private void ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDi
SqlParameter sqlParameter = rpc.userParams[index];
Debug.Assert(sqlParameter != null, "sqlParameter should not be null.");

if (sqlParameter.ParameterNameFixed.Equals(parameterName, StringComparison.Ordinal))
if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName, parameterName, StringComparison.Ordinal))
{
Debug.Assert(sqlParameter.CipherMetadata == null, "param.CipherMetadata should be null.");
sqlParameter.HasReceivedMetadata = true;
Expand Down Expand Up @@ -6239,7 +6239,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
{
if (rec.tdsType != TdsEnums.SQLBIGVARBINARY)
{
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.ParameterNameFixed, rec.tdsType, TdsEnums.SQLBIGVARBINARY);
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.GetPrefixedParameterName(), rec.tdsType, TdsEnums.SQLBIGVARBINARY);
}

// Decrypt the ciphertext
Expand Down Expand Up @@ -6269,7 +6269,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
}
catch (Exception e)
{
throw SQL.ParamDecryptionFailed(thisParam.ParameterNameFixed, null, e);
throw SQL.ParamDecryptionFailed(thisParam.GetPrefixedParameterName(), null, e);
}
}
else
Expand Down Expand Up @@ -6462,7 +6462,11 @@ private SqlParameterCollection GetCurrentParameterCollection()
{
thisParam = parameters[i];
// searching for Output or InputOutput or ReturnValue with matching name
if (thisParam.Direction != ParameterDirection.Input && thisParam.Direction != ParameterDirection.ReturnValue && paramName == thisParam.ParameterNameFixed)
if (
thisParam.Direction != ParameterDirection.Input &&
thisParam.Direction != ParameterDirection.ReturnValue &&
SqlParameter.ParameterNamesEqual(paramName, thisParam.ParameterName,StringComparison.Ordinal)
)
{
foundParam = true;
break; // found it
Expand Down Expand Up @@ -6850,11 +6854,11 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

// Find the return value parameter (if any).
SqlParameter returnValueParameter = null;
foreach (SqlParameter parameter in parameters)
foreach (SqlParameter param in parameters)
{
if (parameter.Direction == ParameterDirection.ReturnValue)
if (param.Direction == ParameterDirection.ReturnValue)
{
returnValueParameter = parameter;
returnValueParameter = param;
break;
}
}
Expand All @@ -6863,7 +6867,8 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// EXEC @returnValue = moduleName [parameters]
if (returnValueParameter != null)
{
execStatement.AppendFormat(@"{0}=", returnValueParameter.ParameterNameFixed);
SqlParameter.AppendPrefixedParameterName(execStatement, returnValueParameter.ParameterName);
execStatement.Append('=');
}

execStatement.Append(ParseAndQuoteIdentifier(storedProcedureName, false));
Expand All @@ -6874,6 +6879,7 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the first parameter
int index = 0;
int count = parameters.Count;
SqlParameter parameter;
if (count > 0)
{
// Skip the return value parameters.
Expand All @@ -6884,16 +6890,20 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

if (index < count)
{
parameter = parameters[index];
// Possibility of a SQL Injection issue through parameter names and how to construct valid identifier for parameters.
// Since the parameters comes from application itself, there should not be a security vulnerability.
// Also since the query is not executed, but only analyzed there is no possibility for elevation of priviledge, but only for
// incorrect results which would only affect the user that attempts the injection.
execStatement.AppendFormat(@" {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(' ');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (
parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput
parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput
)
{
execStatement.AppendFormat(@" OUTPUT");
Expand All @@ -6907,14 +6917,18 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the rest of parameters
for (; index < count; index++)
{
if (parameters[index].Direction != ParameterDirection.ReturnValue)
parameter = parameters[index];
if (parameter.Direction != ParameterDirection.ReturnValue)
{
execStatement.AppendFormat(@", {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(", ");
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (
parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput
parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput
)
{
execStatement.AppendFormat(@" OUTPUT");
Expand Down Expand Up @@ -6946,9 +6960,10 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete

// add our separator for the ith parameter
if (fAddSeparator)
{
paramList.Append(',');

paramList.Append(sqlParam.ParameterNameFixed);
}
SqlParameter.AppendPrefixedParameterName(paramList, sqlParam.ParameterName);

MetaType mt = sqlParam.InternalMetaType;

Expand All @@ -6957,7 +6972,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete

// paragraph above doesn't seem to be correct. Server won't find the type
// if we don't provide a fully qualified name
paramList.Append(" ");
paramList.Append(' ');
if (mt.SqlDbType == SqlDbType.Udt)
{
string fullTypeName = sqlParam.UdtTypeName;
Expand All @@ -6971,7 +6986,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
string typeName = sqlParam.TypeName;
if (ADP.IsEmpty(typeName))
{
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.ParameterNameFixed);
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.GetPrefixedParameterName());
}
paramList.Append(ParseAndQuoteIdentifier(typeName, false /* is not UdtTypeName*/));

Expand Down

0 comments on commit 33f48ad

Please sign in to comment.