Dealing with single quotes in text widget or text block widget

[TonyFMCMC]

I have discovered that users can enter a single quote, and possibly other dangerous characters into a text widget or a textblock widget. This is a serious security hole for sql injection attacks, let alone chaos caused by an innocent apostrophe.

It would be easier if there were an option to encode these dangerous characters as their HTML equivalents as roam saves them to the linked database.

My further processing uses a trigger sql script; if the next step were php it's easy to handle. I can't imagine there isn't already a solution to this... Anyone?

[NathanW2]

Where do you see the sql injection here? Roam is using QGIS which is using prepared statements to send the insert statements to the database.

On Tue, May 5, 2020 at 5:14 PM TonyFMCMC notifications@github.com wrote:

I have discovered that users can enter a single quote, and possibly other dangerous characters into a text widget or a textblock widget. This is a serious security hole for sql injection attacks, let alone chaos caused by an innocent apostrophe.

It would be easier if there were an option to encode these dangerous characters as their HTML equivalents as roam saves them to the linked database.

My further processing uses a trigger sql script; if the next step were php it's easy to handle. I can't imagine there isn't already a solution to this... Anyone?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/roam-qgis/Roam/issues/462, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC5FXG4BBME2BSMHIAZHI3RP64ETANCNFSM4MZKTVLQ .

[TonyFMCMC]

Hi Nathan, Roam 2.6. I used a textblock in a form linked to a gpkg file, and a user entered an apostrophe in the textbox. So then when there is connectivity I use an odbc script to upload the record to an sql server. The trigger script subsequently dealing with the insert tries to transfer some of the data to another table but this fails as the apostrophe/single quote stuffs up the sql of the insert statement. TSQL doesn't seem to have a good way of dealing with this, but really it would be better to prevent the quote getting into the gpkg file.

[TonyFMCMC]

specifically I use ogr2ogr to upload the record from the gpkg to sql server.

[TonyFMCMC]

See

[NathanW2]

What is failing though? The ogr script? or your script to transfer it to another table?

On Tue, May 5, 2020 at 5:38 PM TonyFMCMC notifications@github.com wrote:

specifically I use ogr2ogr to upload the record from the gpkg to sql server.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/roam-qgis/Roam/issues/462#issuecomment-623904094, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC5FXD3YKWAX73NNLZ23CTRP67ARANCNFSM4MZKTVLQ .

[TonyFMCMC]

the sql trigger which is trying to insert a subset of info from the reports table (including the FollowUpOrNextRoundWorks field into another table. This is the faulty sql that is generated: INSERT into dbo.ProjectJournal (JournalID,ProjectID,JDate,EmployeeID,Notes,Status,JType,HasLinkedFiles,EntryType,LastUpdated,LinkID) VALUES ('73309875-501F-4CA8-879E-942120C0A0DD','PStAlb20:Weed','2020.05.04','SarahBo', 'SarahBo, SophieL, TonyB spent 7.78 hours each at Biosite 3546 - St Albans in an area of 0.17ha doing Weed:Non-spray. Work completed: finished most handweeding of plantain in this area . Works for next visit: wait a week for previous spray to kick in, then spray what's sprayable and a final round of handweeding after that: 0.00 hours. Approved on 01/01/1900.',-1,11,0,9,'May 4 2020 11:26PM','97545CEC-5862-4CC1-9B1D-AA4331474836')

I would have expected that apostrophe ("...what's sprayable...") would never have got into the database without being converted somehow. But I'm a bit of a newb at all this.

[NathanW2]

How are you generating that SQL?

[NathanW2]

There is no issue with using an apostrophe in a string as long as you aren't handcrafting formatted strings in your statements....

On Tue, May 5, 2020 at 6:13 PM Nathan Woodrow madmanwoo@gmail.com wrote:

How are you generating that SQL?

[TonyFMCMC]

The problem is the apostrophe terminated the string early and made nonsense of the rest of the query, making the trigger fail, and stopping data transfer, but if there was an sql injection attack, could that not have been more serious?

[NathanW2]

Yeah but I am trying to work out what is making that query that is terminated early. What is making that insert statement? Is it your trigger? If so what does your trigger look like?

On Tue, 5 May 2020, 6:23 pm TonyFMCMC, notifications@github.com wrote:

The problem is the apostrophe terminated the string early and made nonsense of the rest of the query, making the trigger fail, and stopping data transfer, but if there was an sql injection attack, could that not have been more serious?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/roam-qgis/Roam/issues/462#issuecomment-623923074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC5FXE6V43VQZE6F2JSJWTRP7EGVANCNFSM4MZKTVLQ .

[TonyFMCMC]

Sorry, generating the sql within a trigger script in Azure SQL. There doesn't seem to be a function within sql to easily convert dangerous characters to HTML entities. I understand from the reading that I have done that it is better to prevent characters which could be used for sql injection from getting into the data in the first place.

[TonyFMCMC]

Oh the trigger is long. here is an excerpt: `BEGIN TRY set @sql=N'INSERT into dbo.ProjectJournal (JournalID,ProjectID,JDate,EmployeeID,Notes,Status,JType,HasLinkedFiles,EntryType,LastUpdated,LinkID) VALUES '; select @sql=isnull(@sql,'')+CONCAT('(''',[reportUUID] ,''',''' ,ProjectID,''',''' ,CONVERT(VARCHAR(10),[Datestamp],102),''',''' ,Isnull([Approvedby],'Draft') ,''',''' ,REPLACE(Isnull([Staff],''),';',', ')+' spent ' +ltrim(str(i.[Hours],6,2)) +Case when [Overtime]=-1 then ' overtime hours ' else ' hours ' end +Case when (len(i.Staff)-len(replace(i.Staff,';',''))+1)>1 then 'each ' else '' end +Case when [Site] is null then 'onsite' else 'at '+[Site] end +Case when [ogr_geometry] is null then '' else ' in an area of '+ltrim(str([ogr_geometry].STArea()/10000,8,2))+'ha' end +' doing '+Isnull([Activity],'[something]')+'. ' +Case when i.FeedbackOnWorksCompleted is null then '' else 'Work completed: '+i.FeedbackOnWorksCompleted+'. ' end
+Case when i.SupervisorsWorkRemainingAfter is null then 'No indication was given of work remaining' else 'Works for next visit: '+isnull(i.FollowUpOrNextRoundWorks,'unknown')+': '+ltrim(str(i.SupervisorsWorkRemainingAfter,6,2))+' hours. ' end +Case when i.Approved=5 then 'Approved on '+CONVERT(VARCHAR(10),isnull(i.Approveddatetime,''),103)+'.' else 'Not yet approved ('+i.Workflow_messages+')' end ,''',' ,-1 ,',' ,11,',' ,0 ,',' ,9 ,',''' ,CONVERT(datetime,GETDATE(),120),''',''' ,T.LineID,'''),') FROM inserted i left join dbo.TeamAllocationsAndBillquickAllocations T on i.TeamAllocationID=T.ID select @sql=substring(@sql, 1, len(@sql) - 1) EXEC sp_execute_remote N'xxxxx', @sql

                END TRY`

[NathanW2]

Yes, your issue is here. You shouldn't do this. Ever. (unless you fully trust the input and data, which you never ever should even in your own DB).

Any SQL statement should be using prepared statements and parameters. Never, ever, handcraft your SQL statements.

If ProjectJournal is in the same database there is no need to use sp_execute_remote here you can just insert into it with an INSERT INTO (...) SELECT .... FROM style query.

If you do need to use sp_execute_remote it can accept parameters: https://docs.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-execute-remote-azure-sql-database?view=azuresqldb-current

You should build the query with parameters: INSERT INTO ProjectJournal (JournalID) VALUES (@JournalID) and pass them in via sp_execute_remote with something like:

EXEC sp_execute_remote @data_source_name = 'xxxx', @stmt = N'INSERT INTO ProjectJournal (JournalID) VALUES (@JournalID) ', @params = N'@JournalID nvarchar(128)', @ JournalID = value from table here;

But again if it's int the same databas I don't see you needing sp_execute_remote here and it's making your life harder.

On Tue, May 5, 2020 at 6:31 PM TonyFMCMC notifications@github.com wrote:

Oh the trigger is long. here is an excerpt: `BEGIN TRY set @Sql https://github.com/Sql=N'INSERT into dbo.ProjectJournal (JournalID,ProjectID,JDate,EmployeeID,Notes,Status,JType,HasLinkedFiles,EntryType,LastUpdated,LinkID) VALUES '; select @Sql https://github.com/Sql=isnull(@Sql https://github.com/Sql,'')+CONCAT('(''',[reportUUID] ,''',''' ,ProjectID,''',''' ,CONVERT(VARCHAR(10),[Datestamp],102),''',''' ,Isnull([Approvedby],'Draft') ,''',''' ,REPLACE(Isnull([Staff],''),';',', ')+' spent ' +ltrim(str(i.[Hours],6,2)) +Case when [Overtime]=-1 then ' overtime hours ' else ' hours ' end +Case when (len(i.Staff)-len(replace(i.Staff,';',''))+1)>1 then 'each ' else '' end +Case when [Site] is null then 'onsite' else 'at '+[Site] end +Case when [ogr_geometry] is null then '' else ' in an area of '+ltrim(str([ogr_geometry].STArea()/10000,8,2))+'ha' end +' doing '+Isnull([Activity],'[something]')+'. ' +Case when i.FeedbackOnWorksCompleted is null then '' else 'Work completed: '+i.FeedbackOnWorksCompleted+'. ' end +Case when i.SupervisorsWorkRemainingAfter is null then 'No indication was given of work remaining' else 'Works for next visit: '+isnull(i.FollowUpOrNextRoundWorks,'unknown')+': '+ltrim(str(i.SupervisorsWorkRemainingAfter,6,2))+' hours. ' end +Case when i.Approved=5 then 'Approved on '+CONVERT(VARCHAR(10),isnull(i.Approveddatetime,''),103)+'.' else 'Not yet approved ('+i.Workflow_messages+')' end ,''',' ,-1 ,',' ,11,',' ,0 ,',' ,9 ,',''' ,CONVERT(datetime,GETDATE(),120),''',''' ,T.LineID,'''),') FROM inserted i left join dbo.TeamAllocationsAndBillquickAllocations T on i.TeamAllocationID=T.ID select @Sql https://github.com/Sql=substring(@Sql https://github.com/Sql, 1, len(@Sql https://github.com/Sql) - 1) EXEC sp_execute_remote N'xxxxx', @Sql https://github.com/Sql

          END TRY`

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/roam-qgis/Roam/issues/462#issuecomment-623926899, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC5FXB3567UJTTK5R3PQGLRP7FHJANCNFSM4MZKTVLQ .

[TonyFMCMC]

Its in another database alas. I'd much prefer to avoid sp_execute_remote but don't know another way.
I will have to do my homework on prepared statements. Thanks for your help!