Skip to content

Commit 4702420

Browse files
Wraith2Wraith2
authored
stop creating parameter prefixed names unless we really need the string (#1829)
1 parent a5ad838 commit 4702420

File tree

6 files changed

+175
-62
lines changed

6 files changed

+175
-62
lines changed

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4363,7 +4363,7 @@ private void ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDi
43634363
SqlParameter sqlParameter = rpc.userParams[index];
43644364
Debug.Assert(sqlParameter != null, "sqlParameter should not be null.");
43654365

4366-
if (sqlParameter.ParameterNameFixed.Equals(parameterName, StringComparison.Ordinal))
4366+
if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName,parameterName,StringComparison.Ordinal))
43674367
{
43684368
Debug.Assert(sqlParameter.CipherMetadata == null, "param.CipherMetadata should be null.");
43694369
sqlParameter.HasReceivedMetadata = true;
@@ -5457,7 +5457,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
54575457
{
54585458
if (rec.tdsType != TdsEnums.SQLBIGVARBINARY)
54595459
{
5460-
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.ParameterNameFixed, rec.tdsType, TdsEnums.SQLBIGVARBINARY);
5460+
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.GetPrefixedParameterName(), rec.tdsType, TdsEnums.SQLBIGVARBINARY);
54615461
}
54625462

54635463
// Decrypt the ciphertext
@@ -5487,7 +5487,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
54875487
}
54885488
catch (Exception e)
54895489
{
5490-
throw SQL.ParamDecryptionFailed(thisParam.ParameterNameFixed, null, e);
5490+
throw SQL.ParamDecryptionFailed(thisParam.GetPrefixedParameterName(), null, e);
54915491
}
54925492
}
54935493
else
@@ -5628,7 +5628,11 @@ private SqlParameter GetParameterForOutputValueExtraction(SqlParameterCollection
56285628
{
56295629
thisParam = parameters[i];
56305630
// searching for Output or InputOutput or ReturnValue with matching name
5631-
if (thisParam.Direction != ParameterDirection.Input && thisParam.Direction != ParameterDirection.ReturnValue && paramName == thisParam.ParameterNameFixed)
5631+
if (
5632+
thisParam.Direction != ParameterDirection.Input &&
5633+
thisParam.Direction != ParameterDirection.ReturnValue &&
5634+
SqlParameter.ParameterNamesEqual(paramName, thisParam.ParameterName,StringComparison.Ordinal)
5635+
)
56325636
{
56335637
foundParam = true;
56345638
break; // found it
@@ -5999,11 +6003,11 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
59996003

60006004
// Find the return value parameter (if any).
60016005
SqlParameter returnValueParameter = null;
6002-
foreach (SqlParameter parameter in parameters)
6006+
foreach (SqlParameter param in parameters)
60036007
{
6004-
if (parameter.Direction == ParameterDirection.ReturnValue)
6008+
if (param.Direction == ParameterDirection.ReturnValue)
60056009
{
6006-
returnValueParameter = parameter;
6010+
returnValueParameter = param;
60076011
break;
60086012
}
60096013
}
@@ -6012,7 +6016,8 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
60126016
// EXEC @returnValue = moduleName [parameters]
60136017
if (returnValueParameter != null)
60146018
{
6015-
execStatement.AppendFormat(@"{0}=", returnValueParameter.ParameterNameFixed);
6019+
SqlParameter.AppendPrefixedParameterName(execStatement, returnValueParameter.ParameterName);
6020+
execStatement.Append('=');
60166021
}
60176022

60186023
execStatement.Append(ParseAndQuoteIdentifier(storedProcedureName, false));
@@ -6023,6 +6028,7 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
60236028
// Append the first parameter
60246029
int index = 0;
60256030
int count = parameters.Count;
6031+
SqlParameter parameter;
60266032
if (count > 0)
60276033
{
60286034
// Skip the return value parameters.
@@ -6033,15 +6039,19 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
60336039

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

60426052
// InputOutput and Output parameters need to be marked as such.
6043-
if (parameters[index].Direction == ParameterDirection.Output ||
6044-
parameters[index].Direction == ParameterDirection.InputOutput)
6053+
if (parameter.Direction == ParameterDirection.Output ||
6054+
parameter.Direction == ParameterDirection.InputOutput)
60456055
{
60466056
execStatement.AppendFormat(@" OUTPUT");
60476057
}
@@ -6054,14 +6064,18 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
60546064
// Append the rest of parameters
60556065
for (; index < count; index++)
60566066
{
6057-
if (parameters[index].Direction != ParameterDirection.ReturnValue)
6067+
parameter = parameters[index];
6068+
if (parameter.Direction != ParameterDirection.ReturnValue)
60586069
{
6059-
execStatement.AppendFormat(@", {0}={0}", parameters[index].ParameterNameFixed);
6070+
execStatement.Append(", ");
6071+
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
6072+
execStatement.Append('=');
6073+
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
60606074

60616075
// InputOutput and Output parameters need to be marked as such.
60626076
if (
6063-
parameters[index].Direction == ParameterDirection.Output ||
6064-
parameters[index].Direction == ParameterDirection.InputOutput
6077+
parameter.Direction == ParameterDirection.Output ||
6078+
parameter.Direction == ParameterDirection.InputOutput
60656079
)
60666080
{
60676081
execStatement.AppendFormat(@" OUTPUT");
@@ -6095,9 +6109,11 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
60956109

60966110
// add our separator for the ith parameter
60976111
if (fAddSeparator)
6112+
{
60986113
paramList.Append(',');
6114+
}
60996115

6100-
paramList.Append(sqlParam.ParameterNameFixed);
6116+
SqlParameter.AppendPrefixedParameterName(paramList, sqlParam.ParameterName);
61016117

61026118
MetaType mt = sqlParam.InternalMetaType;
61036119

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

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9312,7 +9312,7 @@ private Task TDSExecuteRPCAddParameter(TdsParserStateObject stateObj, SqlParamet
93129312
}
93139313
}
93149314

9315-
WriteParameterName(param.ParameterNameFixed, stateObj, isAnonymous);
9315+
WriteParameterName(param.ParameterName, stateObj, isAnonymous);
93169316

93179317
// Write parameter status
93189318
stateObj.WriteByte(options);
@@ -9833,17 +9833,34 @@ private void ExecuteFlushTaskCallback(Task tsk, TdsParserStateObject stateObj, T
98339833
}
98349834
}
98359835

9836-
9837-
private void WriteParameterName(string parameterName, TdsParserStateObject stateObj, bool isAnonymous)
9836+
/// <summary>
9837+
/// Will check the parameter name for the required @ prefix and then write the correct prefixed
9838+
/// form and correct character length to the output buffer
9839+
/// </summary>
9840+
private void WriteParameterName(string rawParameterName, TdsParserStateObject stateObj, bool isAnonymous)
98389841
{
98399842
// paramLen
98409843
// paramName
9841-
if (!isAnonymous && !string.IsNullOrEmpty(parameterName))
9844+
if (!isAnonymous && !string.IsNullOrEmpty(rawParameterName))
98429845
{
9843-
Debug.Assert(parameterName.Length <= 0xff, "parameter name can only be 255 bytes, shouldn't get to TdsParser!");
9844-
int tempLen = parameterName.Length & 0xff;
9845-
stateObj.WriteByte((byte)tempLen);
9846-
WriteString(parameterName, tempLen, 0, stateObj);
9846+
int nameLength = rawParameterName.Length;
9847+
int totalLength = nameLength;
9848+
bool writePrefix = false;
9849+
if (nameLength > 0)
9850+
{
9851+
if (rawParameterName[0] != '@')
9852+
{
9853+
writePrefix = true;
9854+
totalLength += 1;
9855+
}
9856+
}
9857+
Debug.Assert(totalLength <= 0xff, "parameter name can only be 255 bytes, shouldn't get to TdsParser!");
9858+
stateObj.WriteByte((byte)(totalLength & 0xFF));
9859+
if (writePrefix)
9860+
{
9861+
WriteString("@", 1, 0, stateObj);
9862+
}
9863+
WriteString(rawParameterName, nameLength, 0, stateObj);
98479864
}
98489865
else
98499866
{

‎src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs‎

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4881,7 +4881,7 @@ private void ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDi
48814881
SqlParameter sqlParameter = rpc.userParams[index];
48824882
Debug.Assert(sqlParameter != null, "sqlParameter should not be null.");
48834883

4884-
if (sqlParameter.ParameterNameFixed.Equals(parameterName, StringComparison.Ordinal))
4884+
if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName, parameterName, StringComparison.Ordinal))
48854885
{
48864886
Debug.Assert(sqlParameter.CipherMetadata == null, "param.CipherMetadata should be null.");
48874887
sqlParameter.HasReceivedMetadata = true;
@@ -6239,7 +6239,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
62396239
{
62406240
if (rec.tdsType != TdsEnums.SQLBIGVARBINARY)
62416241
{
6242-
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.ParameterNameFixed, rec.tdsType, TdsEnums.SQLBIGVARBINARY);
6242+
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.GetPrefixedParameterName(), rec.tdsType, TdsEnums.SQLBIGVARBINARY);
62436243
}
62446244

62456245
// Decrypt the ciphertext
@@ -6269,7 +6269,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
62696269
}
62706270
catch (Exception e)
62716271
{
6272-
throw SQL.ParamDecryptionFailed(thisParam.ParameterNameFixed, null, e);
6272+
throw SQL.ParamDecryptionFailed(thisParam.GetPrefixedParameterName(), null, e);
62736273
}
62746274
}
62756275
else
@@ -6462,7 +6462,11 @@ private SqlParameter GetParameterForOutputValueExtraction(SqlParameterCollection
64626462
{
64636463
thisParam = parameters[i];
64646464
// searching for Output or InputOutput or ReturnValue with matching name
6465-
if (thisParam.Direction != ParameterDirection.Input && thisParam.Direction != ParameterDirection.ReturnValue && paramName == thisParam.ParameterNameFixed)
6465+
if (
6466+
thisParam.Direction != ParameterDirection.Input &&
6467+
thisParam.Direction != ParameterDirection.ReturnValue &&
6468+
SqlParameter.ParameterNamesEqual(paramName, thisParam.ParameterName,StringComparison.Ordinal)
6469+
)
64666470
{
64676471
foundParam = true;
64686472
break; // found it
@@ -6850,11 +6854,11 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
68506854

68516855
// Find the return value parameter (if any).
68526856
SqlParameter returnValueParameter = null;
6853-
foreach (SqlParameter parameter in parameters)
6857+
foreach (SqlParameter param in parameters)
68546858
{
6855-
if (parameter.Direction == ParameterDirection.ReturnValue)
6859+
if (param.Direction == ParameterDirection.ReturnValue)
68566860
{
6857-
returnValueParameter = parameter;
6861+
returnValueParameter = param;
68586862
break;
68596863
}
68606864
}
@@ -6863,7 +6867,8 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
68636867
// EXEC @returnValue = moduleName [parameters]
68646868
if (returnValueParameter != null)
68656869
{
6866-
execStatement.AppendFormat(@"{0}=", returnValueParameter.ParameterNameFixed);
6870+
SqlParameter.AppendPrefixedParameterName(execStatement, returnValueParameter.ParameterName);
6871+
execStatement.Append('=');
68676872
}
68686873

68696874
execStatement.Append(ParseAndQuoteIdentifier(storedProcedureName, false));
@@ -6874,6 +6879,7 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
68746879
// Append the first parameter
68756880
int index = 0;
68766881
int count = parameters.Count;
6882+
SqlParameter parameter;
68776883
if (count > 0)
68786884
{
68796885
// Skip the return value parameters.
@@ -6884,16 +6890,20 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
68846890

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

68936903
// InputOutput and Output parameters need to be marked as such.
68946904
if (
6895-
parameters[index].Direction == ParameterDirection.Output ||
6896-
parameters[index].Direction == ParameterDirection.InputOutput
6905+
parameter.Direction == ParameterDirection.Output ||
6906+
parameter.Direction == ParameterDirection.InputOutput
68976907
)
68986908
{
68996909
execStatement.AppendFormat(@" OUTPUT");
@@ -6907,14 +6917,18 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
69076917
// Append the rest of parameters
69086918
for (; index < count; index++)
69096919
{
6910-
if (parameters[index].Direction != ParameterDirection.ReturnValue)
6920+
parameter = parameters[index];
6921+
if (parameter.Direction != ParameterDirection.ReturnValue)
69116922
{
6912-
execStatement.AppendFormat(@", {0}={0}", parameters[index].ParameterNameFixed);
6923+
execStatement.Append(", ");
6924+
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
6925+
execStatement.Append('=');
6926+
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
69136927

69146928
// InputOutput and Output parameters need to be marked as such.
69156929
if (
6916-
parameters[index].Direction == ParameterDirection.Output ||
6917-
parameters[index].Direction == ParameterDirection.InputOutput
6930+
parameter.Direction == ParameterDirection.Output ||
6931+
parameter.Direction == ParameterDirection.InputOutput
69186932
)
69196933
{
69206934
execStatement.AppendFormat(@" OUTPUT");
@@ -6946,9 +6960,10 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
69466960

69476961
// add our separator for the ith parameter
69486962
if (fAddSeparator)
6963+
{
69496964
paramList.Append(',');
6950-
6951-
paramList.Append(sqlParam.ParameterNameFixed);
6965+
}
6966+
SqlParameter.AppendPrefixedParameterName(paramList, sqlParam.ParameterName);
69526967

69536968
MetaType mt = sqlParam.InternalMetaType;
69546969

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

69586973
// paragraph above doesn't seem to be correct. Server won't find the type
69596974
// if we don't provide a fully qualified name
6960-
paramList.Append(" ");
6975+
paramList.Append(' ');
69616976
if (mt.SqlDbType == SqlDbType.Udt)
69626977
{
69636978
string fullTypeName = sqlParam.UdtTypeName;
@@ -6971,7 +6986,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
69716986
string typeName = sqlParam.TypeName;
69726987
if (ADP.IsEmpty(typeName))
69736988
{
6974-
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.ParameterNameFixed);
6989+
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.GetPrefixedParameterName());
69756990
}
69766991
paramList.Append(ParseAndQuoteIdentifier(typeName, false /* is not UdtTypeName*/));
69776992

0 commit comments

Comments
 (0)