Handle SQL injection vulnerabilities within ObjectToSQLString #3547
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,6 +208,7 @@ public virtual void Configure(IDictionary<string, string> settings) | |
| DefaultCastLength = PropertiesHelper.GetInt32(Environment.QueryDefaultCastLength, settings, 4000); | ||
| DefaultCastPrecision = PropertiesHelper.GetByte(Environment.QueryDefaultCastPrecision, settings, null) ?? 29; | ||
| DefaultCastScale = PropertiesHelper.GetByte(Environment.QueryDefaultCastScale, settings, null) ?? 10; | ||
| EscapeBackslashInStrings = PropertiesHelper.GetBoolean(Environment.EscapeBackslashInStrings, settings, EscapeBackslashInStrings); | ||
| } | ||
|
|
||
| #endregion | ||
|
|
@@ -1354,14 +1355,6 @@ public virtual CaseFragment CreateCaseFragment() | |
| return new ANSICaseFragment(this); | ||
| } | ||
|
|
||
| internal static void ExtractColumnOrAliasNames(SqlString select, out List<SqlString> columnsOrAliases, out Dictionary<SqlString, SqlString> aliasToColumn, out Dictionary<SqlString, SqlString> columnToAlias) | ||
| { | ||
| columnsOrAliases = new List<SqlString>(); | ||
|
|
@@ -2076,6 +2069,55 @@ public virtual string ConvertQuotesForCatalogName(string catalogName) | |
|
|
||
| #endregion | ||
|
|
||
| #region Literals support | ||
|
|
||
| /// <summary>The SQL literal value to which this database maps boolean values.</summary> | ||
| /// <param name="value">The boolean value.</param> | ||
| /// <returns>The appropriate SQL literal.</returns> | ||
| public virtual string ToBooleanValueString(bool value) | ||
| => value ? "1" : "0"; | ||
|
|
||
| /// <summary> | ||
| /// <see langword="true" /> if the database needs to have backslash escaped in string literals. | ||
| /// </summary> | ||
| /// <remarks><see langword="false" /> by default in the base dialect, to conform to SQL standard.</remarks> | ||
| protected virtual bool EscapeBackslashInStrings { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// <see langword="true" /> if the database needs to have Unicode literals prefixed by <c>N</c>. | ||
| /// </summary> | ||
| /// <remarks><see langword="false" /> by default in the base dialect.</remarks> | ||
| protected virtual bool UseNPrefixForUnicodeStrings => false; | ||
|
|
||
| /// <summary>The SQL string literal value to which this database maps string values.</summary> | ||
| /// <param name="value">The string value.</param> | ||
| /// <param name="type">The SQL type of the string value.</param> | ||
| /// <returns>The appropriate SQL string literal.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown if <paramref name="value"/> or | ||
| /// <paramref name="type"/> is <see langword="null" />.</exception> | ||
| public virtual string ToStringLiteral(string value, SqlType type) | ||
|
There was a problem hiding this comment. I'm not sold on necessity of passing the SqlType here. The primary purpose of this value is to determine whether or not the value to be escaped should result in a Unicode value. I would suggest one of the following:
Admittedly, I prefer the former over the latter as it allows dialects to have different implementations if needed without a conditional on the boolean value provided. |
||
| { | ||
| if (value == null) | ||
| throw new ArgumentNullException(nameof(value)); | ||
| if (type == null) | ||
| throw new ArgumentNullException(nameof(value)); | ||
hazzik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| var literal = new StringBuilder(value); | ||
| if (EscapeBackslashInStrings) | ||
| literal.Replace(@"\", @"\\"); | ||
|
|
||
| literal | ||
| .Replace("'", "''") | ||
| .Insert(0, '\'') | ||
| .Append('\''); | ||
|
There was a problem hiding this comment. There's no guarantee that the implementation of this method won't break an obscure dialect that does not support doubling the number of single quotes inside a single quoted string to escape a single quote. While this is the ANSI standard, not all dialects adhere to that standard. The only non-breaking thing to do here is to simply return the value passed. There was a problem hiding this comment. Fixing a bug of lack of escapism is anyway behavior breaking for those which were compensating it by escaping values themselves (if anyone). And from a security standpoint, better take the risk of breaking some dialects not natively supported by NHibernate rather than leaving the most common vulnerability (lack of doubling the quote) open for all not natively supported dialects. Fixing bug is anyway always somewhat behavior breaking. That is normal and accepted for patch releases. That is binary breaking changes or source breaking changes which are banned by SemVer for a patch or minor release. Mores specifically, from https://semver.org/ :
(So, a new throwing base method causing previously "working in most cases" features to cease working is neither a binary breaking change nor a source one. But undoubtedly it does not qualify as "an internal change that fixes incorrect behavior".) There was a problem hiding this comment. If I understand correctly, you are saying that in the case of a bug fix, it's okay to throw an exception for changes that correct behavior. Admittedly, I do not have a problem with that so long as the exception is thrown in the appropriate location. The changes here would cause an exception to be thrown once the statement is executed, not when the method is called. Any dialects broken by this change would therefore not know the root cause of the exception. Therefore, if you'd like to throw an exception in the base case instead of leaving external dialects vulnerable, that would be acceptable. Alternatively, as already discussed, NHibernate could use query parameters to avoid escaping any values. It may not be as performant as the solution here, but it would undoubtedly be more maintainable and secure. It would also not introduce any breaking changes. There was a problem hiding this comment. No, I am not saying that:
|
||
|
|
||
| if (UseNPrefixForUnicodeStrings && (type.DbType == DbType.String || type.DbType == DbType.StringFixedLength)) | ||
| literal.Insert(0, 'N'); | ||
| return literal.ToString(); | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| #region Union subclass support | ||
|
|
||
| /// <summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be a user configurable option in my opinion. The dialect should perform the all appropriate escaping where necessary. For example, your implementation for DB2 ignores this setting while always escaping back slashes for Unicode strings.