Anytime you introduce data the user could have entered directly into a SQL query, you must be aware of the possibility of attack.  For example, if you use a user's City profile property in a query

SELECT * FROM mystore_Locations WHERE City = '[User:City]'

If the "City" profile property is not correctly sanitized, a user could enter the following as their City:

'; UPDATE dnn_Users SET IsSuperUser = 1 WHERE UserId = 1234; SELECT '

Which would change the script above to:

SELECT * FROM mystore_Locations WHERE City = ''; UPDATE dnn_Users SET IsSuperUser = 1 WHERE UserId = 1234; SELECT ''

(I'm working somewhat from memory, so the exact database fields may not be correct, but I hope you get the idea).

The point is not that TokenReplace is accessing the database, it's that you could be introducing user editable fields directly into the SQL query used by the Reports Module.  Even if TokenReplace or the Reports Module itself did some checking to remove SQL commands from those strings, I would not feel comfortable using it as there are just so many different SQL Injection attack vectors.  Especially not when SQL Server has a parameter model designed explicitly to avoid this problem.

As I said, it all depends on what data you are retrieving from TokenReplace, which is why I don't have a problem with people making the change themselves, but they must be aware of the potential problems if they use data entered by regular users.

Andrew Nurse
DotNetNuke Core Team Member and Reports Module Project Lead
Microsoft Certified Professional Developer

I absolutely understand the need for parameterizing queries based on data from DotNetNuke objects, and I intend to implement such a feature.  Tokens aren't the secure way to do that, but I have designed a system that has the same power as Tokens but with full security against SQL Injection (using SQL Parameterized Queries).  As I said, I just haven't had time to implement it.  It will be part of the next major release of the module, I just can't guarantee when that will be.

Andrew Nurse
DotNetNuke Core Team Member and Reports Module Project Lead
Microsoft Certified Professional Developer

The difference is that parameter values are passed separately to SQL Server and are sanitized by ADO.Net and SQL Server to escape SQL characters.  For example, the first ' character in the malicious string we've been using would be escaped, if we used parameters.  Token Replace, on the other hand, uses .Net String Replacement techniques.  Even if Token Replace is aware of SQL Injection concerns and performs the correctly sanitization, I'd much rather trust SQL Server to do it for me.

Using parameters, the query you wrote above would simply search for entries in mystore_Locations where the User = "'; UPDATE dnn_Users SET IsSuperUser = 1 WHERE UserId = 1234; SELECT '", the value in @Portal_user_name cannot find its way into the query.  So it's not a matter of what's more comfortable, its a matter of security.  Could the Reports Module implement Token Replace in a secure way by santizing the Token values?  Probably, but I would rather trust Microsoft to do it right than risk accidentally missing out some edge case.

On the topic of the trade off between security and functionality, I don't believe it is that much of a concern.  In this case, adding parameters would provide all the same features as Token Replace and be much more secure.  Trading security for functionality is asking for trouble and is not a trade off that the Reports Module will make under my leadership.  However, I think that when implemented correctly, almost all functionality can be implemented securely.

Andrew Nurse
DotNetNuke Core Team Member and Reports Module Project Lead
Microsoft Certified Professional Developer