ObjectToSQLString implementations are problematic

[gliljas]

While working with DateOnly and TimeOnly support I realized that the ObjectToSQLString method of ILiteralType is quite problematic.

AbstractStringType:
"'" + (string)value + "'"

• SQL injection
• No N-prefix for unicode (required in SQL server when the string is outside ANSI)

AbstractCharType:
'\'' + value.ToString() + '\''

• SQL injection
• No N-prefix for unicode (required in SQL server when the string is outside ANSI)

AbstractDateTimeType:
"'" + (DateTime) value + "'"

• No guarantee that the stringified DateTime will be a supported datetime syntax

DateTimeOffsetType:
"'" + ((DateTimeOffset) value) + "'"

• No guarantee that the stringified DateTimeOffset will be a supported datetimeoffset syntax

DateType:
"\'" + ((DateTime)value).ToShortDateString() + "\'"

• No guarantee that the stringified DateTime will be a supported datetime syntax

DecimalType:
value.ToString()

• Could very well be a decimal with the wrong kind of decimal separator

And there are more.

Since the method has access to the dialect, maybe more formatting should be moved there.

[fredericDelaporte]

From my viewpoint, this method should be obsoleted and no more used. Building string fragments of literal values is a bad practice. Parameters should be used instead.

I can find three usages:

• To handle discriminators. I have neither investigated why does it not use parameters, nor how much work would it involve.
• A seemingly unused AddColumn overload on SqlInsertBuilder and SqlUpdateBuilder: why not obsoleting these methods?

[gliljas]

Using literal values in SQL does have some value. E.g. filtered indexes rely on the filtered columns not being supplied as parameters. And filtered indexes are really useful, e.g in scenarios where discriminators are being used. It makes it easy to create indexes tailor made for the sub classes in a class hierarchy. We use that functionality, and I'm certain others do as well.

We could probably "re-enable" such things with query rewriting if the literals were removed, but it's something to take into account. Making them into parameters is likely to cause performance degradation for some users.

[fredericDelaporte]

At least with discriminators, the injection risk is very slim, their values being not dynamic but hard-coded in the mappings. Since NHibernate does not actually use those ObjectToSQLString for anything else (AddColumn being not used), the security vulnerabilities of their implementation is currently not practical to exploit.

Still, we should not let there security vulnerabilities there. If we cannot simply obsolete these methods and cease using them, they would have to be fixed. But I am not sure the dialects currently allow for robust literal SQL fragment generation. It may even be bound to the server or connection locales, like mdy vs dmy troubles on strings representing dates with SQL Server. (Which can be dodged by using an ISO format, but will we have this kind of solution with every vendor?)

[hazzik]

Would someone want to write this up? https://github.com/nhibernate/nhibernate-core/security/advisories/new

[fredericDelaporte]

I now see it is used in HQL too, for JavaConstantNode, BooleanLiteralNode and BinaryLogicOperatorNode. As I understand it, the JavaConstantNode may be used in case of a reference to a static field of a class in the HQL. BooleanLiteralNode should not have issues. And BinaryLogicOperatorNode uses it in case it has to resolve a discriminator, so, we are back to the discriminator case for this one.

[fredericDelaporte]

It is hard to evaluate the impact of the flaw, since the vulnerable methods take an object as a value and do not, in most case, check it is of the expected type. So, with DecimalType (and most number types), if the value happen to be a string like ; drop database nhibernate;, it will be yielded identically by the method current implementation.

The question is then, is it possible to get DecimalType.ObjectToSQLString called with such a value? Definitely yes with SqlInsertBuilder and SqlUpdateBuilder, since they expect both the value and the type as arguments. But that would most likely be a programming error.
With JavaConstantNode, on principle yes too, since the class is public and can be constructed with a "wicked token". Otherwise that depends on TypeFactory.HeuristicType(value.GetType().Name); yielding the adequate type.
With the discriminator case, that would be a very explicit programming error.

On Hibernate side, these ObjectToSQLString methods do not look to exist. I find instead JdbcLiteralFormatter<T> getJdbcLiteralFormatter.

[deAtog]

SQLiteDialect: FormatException due to string conversion of DateTime fields seems to be related to this issue which demonstrates what happens when you carelessly convert to/from a string without using the appropriate locale for the conversion.

[fredericDelaporte]

No it is not related to ObjectToSQLString. SQLite lax typing causes issues of its own, without involving that method. #3530 does not involve it.

[fredericDelaporte]

The advisory was drafted a few weeks ago but I have not seen reactions to it. https://github.com/nhibernate/nhibernate-core/security/advisories
I am not used to the security framework for estimating the severity. I have used the classification to estimate it but many things were not making much sense for the case.

[fredericDelaporte]

The security concerns raised here have now been addressed. Bugs remain.