Blind sql injection fixes rework (#3284)

[jekkos]

I have reinstated the fix from a month ago. Want to get this out to release the 3.3.6.

[odiea]

Applying this to Items the only column that sorts is the Item_id column. All other columns just sort the Item_id.

[jekkos]

Thanks for testing @odiea I have checked your comment. Which column were you trying to sort? If I sort on name or category it does seem to work

sorting on barcode

sorting on name

[odiea]

Here is what I see. This was sorting on Name or Wholesale Price.

[jekkos]

Ok weird, both cases seem to work fine on my end. I did force push a couple of times. Do you have the latest hash in use for the branch?

[odiea]

I downloaded this branch. © 2010 - 2021 · opensourcepos.org · 3.3.5 - 4b12b7.

[jekkos]

Ok thanks for the feedback, I have deployed the version to dev now and was checking, the name field does not sort correctly. I think the others do in fact.

[odiea]

Here is the code that I am testing out in Items.
$sort = $this->Item->sort_column($this->input->get('sort')); // controller
public function sort_column($field)
{
return array_key_exists($field, $this->db->list_fields('items')) ? $field : 'item_id';
} // model

[jekkos]

Think it should be cfe927

[jekkos]

Indeed there is still issue, I'll try to debug it, thanks for the feedback

[odiea]

I tried changing this and it appears to sort the columns correctly now.
From
return in_array($field, $this->db->list_fields('items')) ? $field : 'item_id';
To
return array_key_exists($field, $this->db->list_fields('items')) ? 'item_id' : $field;

[jekkos]

Did another push and seems better now on dev.

[odiea]

The only quirk I see now is on an older established db quantities does not sort? Looks ok on the Demo

[jekkos]

@odiea you mean the regular quantities field?

[odiea]

Yes the Quantity field in the Items table

[jekkos]

@odiea should be fixed now.

[odiea]

Yes it is. Works good now.

[odiea]

What is the issue with placing the id before the $field? I have had to do this to get some of the other tables to sort correctly. I guess the way I see it is the first table sort is the id and if that is not chosen then the fields are chosen.

[jekkos]

Well what the logic does it it checks if the supplied sorting column exists in the db, then applies it.

If some hacker tries to pass in some SQL for injection, it will be ignored as we only allow to sort on db column names.

[odiea]

But this does not appear to work correctly. I tried sorting in Customers, Suppliers and Employees and columns do not sort properly. The columns appear to sort to the id and not the column.

[jekkos]

Indeed well spotted, I think those three models need one more adapation

[odiea]

Try something on Gift cards and you may be done with this

[jekkos]

Ok the columns for giftcards are extended now. I've gotten two more security bug reports through this huntr.dev platform today. Not sure how this works but it seems that some researchers are looking for security bugs in ospos now.. which is good.

[jekkos]

The sale table might not work correctly yet.. The current approach will do some extra queries to fetch the database column names, which do not necessarily map to the column headers names.

I had a further look and there might be a better solution. The header names are already defined in the tabular_helper, but they are not in an accessible place. It might be better to extract those header arrays somewhere and then check for the field presence in each array separately.

[jekkos]

I should revisit this fix at some point..

[jekkos]

Links to #3965