New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SQL Issues in these files #1 #2095

Open
danehrlich1 opened this Issue Jan 2, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@danehrlich1
Copy link
Member

danehrlich1 commented Jan 2, 2019

$sequence_no = sqlQuery("SELECT IFNULL(MAX(sequence_no),0) + 1 AS increment FROM ar_activity WHERE pid = ? AND encounter = ?", array(trim(formData('hidden_patient_code')), trim(formData("HiddenEncounter$CountRow"))));

SQL Parameters need to be bound for security reasons in multiple places in this file.

Sample Problematic and Fixed Example:

  1. sqlQuery(SELECT aco_spec FROM categories WHERE name = $catname ORDER BY id LIMIT 1");
  2. sqlQuery(SELECT aco_spec FROM categories WHERE name = ? ORDER BY id LIMIT 1", array($catname));

This is a simple issue for first timers / those new to open source or PHP. Please feel free to contact myself or @bradymiller for more instructions.

Other Instructions:

@kristophesankar

This comment has been minimized.

Copy link
Contributor

kristophesankar commented Jan 3, 2019

I've installed OpenEMR and provide support for a current client of mine and I would love to get working on some of these. Is the payment.inc.php a good place to start ?

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 3, 2019

hi @kristophesankar ,
I'd stay away from payment.inc.php for now. It already prevents sql injection (albeit via a older method). Would take on something more straightforward for now like script at:
library/spreadsheet.inc.php
thanks, -brady

@kristophesankar

This comment has been minimized.

Copy link
Contributor

kristophesankar commented Jan 3, 2019

I'll take a look, thanks.

@kristophesankar

This comment has been minimized.

Copy link
Contributor

kristophesankar commented Jan 4, 2019

hi @bradymiller ,
I just got a chance to check it out. I have a question though. Is it safe to assume that the table name will not be bound via the array? or will I need to bind the table name as well? Am I on the right track with:

$trow = sqlQuery("SELECT value FROM form_$spreadsheet_form_name WHERE " .
    "id = ? AND rownbr = -1 AND colnbr = -1", array($tempid));

or

$trow = sqlQuery("SELECT value FROM form_?  WHERE id = ? AND rownbr = -1 AND colnbr = -1", array($spreadsheet_form_name, $tempid));

Thanks.

@danehrlich1

This comment has been minimized.

Copy link
Member

danehrlich1 commented Jan 4, 2019

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 4, 2019

hi @kristophesankar ,
As @danehrlich1 points out, tables names are treated differently, since we can't do the standard bind/escape like we usually do. We have a function that basically uses whitelisting technique to ensure the table actually does exist, effectively allowing us to escape table names:

$trow = sqlQuery("SELECT value FROM " . escape_table_name('form_' . $spreadsheet_form_name) . " WHERE id = ? AND rownbr = -1 AND colnbr = -1", array($tempid));

This function can be found here:
https://github.com/openemr/openemr/blob/master/library/formdata.inc.php#L132

@kristophesankar

This comment has been minimized.

Copy link
Contributor

kristophesankar commented Jan 4, 2019

That's an interesting way of doing it. I hadn't thought of that. Thanks.

kristophesankar pushed a commit to kristophesankar/openemr that referenced this issue Jan 5, 2019

kristophesankar pushed a commit to kristophesankar/openemr that referenced this issue Jan 5, 2019

kristophesankar pushed a commit to kristophesankar/openemr that referenced this issue Jan 5, 2019

kristophesankar pushed a commit to kristophesankar/openemr that referenced this issue Jan 5, 2019

bradymiller added a commit that referenced this issue Jan 5, 2019

@bradymiller bradymiller removed the help wanted label Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment