Avoiding SQL Injection

Databases
1

I feel like I went down the wrong path here, and maybe I need to rework some methods before I get too much farther and end up adding a lot of work.

I’ve been updating my values this way:

var rs as rowset

rs=Session.db.SelectSQL("SELECT * from Inventory where InternalInventoryID = '" + InventoryPage.pCurrentInternalInventoryID.ToString + "' AND DistrictID = " + Session.pCurrentDistrictID.tostring + "")

if rs = nil or rs.RowCount= 0 then
  MessageBox "Database Error" + EndOfLine + EndOfLine + "Could not find the selected inventory item in the database."
  return
else
  //Update information in Inventory
  Try
    rs.EditRow
    //
    //Update Check Boxes
    rs.Column("CheckedOut").BooleanValue=self.CheckedOutCheckbox.Value
    rs.Column("Missing").BooleanValue=self.MissingCheckbox.Value
    rs.Column("Disposition").BooleanValue=self.DispositionCheckbox.Value
    rs.Column("StudentOwned").BooleanValue=self.StudentOwnedCheckbox.Value
   
    //
    //Update Basic Information
    //
    rs.Column("SerialNumber").Value=self.InventorySerialNumberField.text
    rs.Column("CaseNumber").Value=self.InventoryCaseNumberField.text
    rs.Column("LockerNumber").Value=self.InventoryLockerNumberField.text
    rs.Column("LockerCombo").Value=self.InventoryLockerComboField.Text
    
    //
    //Update 'Other Data'
    rs.Column("InternalID").Value=self.InventoryInternalIDField.text
    rs.Column("OtherData1").Value=self.InventoryOtherData1Field.text
    rs.Column("OtherData2").Value=self.InventoryOtherData2Field.text
    rs.Column("OtherData3").Value=self.InventoryOtherData3Field.text
    rs.Column("OtherData4").Value=self.InventoryOtherData4Field.Text
    
    //ETC.... There are plenty more, but that gives you an idea.
    
    //Save and close record
    rs.SaveRow
    rs.Close
    
  Catch error As DatabaseException
    MessageBox ("DB Error: " + error.Message)
  end try
end if

Is this an okay way to be doing this, or should I be going back and modifying my methods to eliminate the whateverfield.value entries to something like the Database.ExecuteSQL examples?

2

Doing if this way will avoid SQL injection.

Your SELECT statement is fine too, but only because you know you are working with integers. However, this might be easier to read:

rs=Session.db.SelectSQL("SELECT * from Inventory where InternalInventoryID = ? AND DistrictID = ?", _
    InventoryPage.pCurrentInternalInventoryID.ToString, _
    Session.pCurrentDistrictID)
2 Likes
3

Thank you!

4

your code is fine but playing a bit with ExecuteSQL and prepare statements is worth the efforts IMHO. There are lot of interesting things you can do directly on the database with an update statement. For instance an UPSERT in postgres (https://www.postgresqltutorial.com/postgresql-upsert/). The only “disadvantage” with prepare statements (which prevents SQL injection) is that it sometimes a bit tricks to debug them. But with a good database manager, it is not difficult either.

5

Oh, I’m absolutely certain it is! Moving forward I plan on using it. In fact, I WAS doing it at some point and then stopped (and I have no idea why).

My immediate concern was whether needed to go back and redo everything right now, or whether I could take my time on it. (Probably as a variant array as per the example, as that seems a lot easier to read.)

1 Like
6

No rush needed ;-). There are anyways so many ways to do it and it is debatable what is the best approach. I’m for instance a big SQL fan and to let the database do the heavy load.

That’s often the fastest way, but granted such SQL statements are sometimes not only difficult to read but especially difficult to debug - specifically after a days/weeks, if you forgot to clearly document what your SQL is expected to do :wink: .

7

Just to be clear about injection, when using API 1 would this potentially be an issue:

Dim RS as RecordSet
RS = DB.SQLSelect(SOMETHING)
RS.Edit
RS.Field("SomeColumn").StringValue = UserEnteredString
RS.Update

Or this:

Dim Row As NEW DatabaseRecord
Row.Column("SomeColumn") = UserEnteredString
DB.InsertRecord("SomeTable", Row)

IIRC I think those both should be safe from injection attacks, but I don’t recall where/when I heard that so since the subject has come up I thought I should ask.

  • Karen
8

Technically, you can get SQL injection with any call to the database if the statement takes a parameter, even calling a stored procedure. That’s why Prepared statements, in either API, are important. In API2, much of the work is done for you. In API, you need to create the prepared statements and do the binding.

1 Like
9

yup, therefore protection is a must and you have to do it right for every login screen, especially on Web. A simple select to check the credentials of the user, could otherwise cause a lot of harm.

10

My question was basically is does using the Xojo DB API as shown above provides the same protection as an Prepared statement for adding data to the DB…

The Xojo Insert and update APIs requires that you specify which column the data goes into to and also to specify the datatype…Which is basically what you are doing with a prepared statement.

With those Xojo APIs you are NOT doing the edit or insert in raw SQL, so it depends on what Xojo is doing under the hood.

-Karen