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 #3 #2097

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

Comments

Projects
None yet
4 participants
@danehrlich1
Copy link
Member

danehrlich1 commented Jan 2, 2019

SQL Parameters need to be bound for security reasons in multiple places in these files: clinical_rules.php, create_ssl_certificate.php, formdata.inc.php.

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:

@Nelson-Chinedu

This comment has been minimized.

Copy link

Nelson-Chinedu commented Jan 3, 2019

hey @danehrlich1 will like to work on this issues but will need pointers on where to work in the files

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 3, 2019

hi @Nelson-Chinedu ,
Here's a good script that lots of sql-injection offenders:
https://github.com/openemr/openemr/blob/master/library/calendar.inc
-brady

@kkvvii84

This comment has been minimized.

Copy link
Contributor

kkvvii84 commented Jan 3, 2019

Hi,

Seen your post on reddit, just setting my machine up and will try have a look over the weekend. I am an Oracle middleware developer, been looking for a opensource project to learn new skills.

@danehrlich1

This comment has been minimized.

Copy link
Member

danehrlich1 commented Jan 3, 2019

@kkvvii84

This comment has been minimized.

Copy link
Contributor

kkvvii84 commented Jan 3, 2019

It is SOA Suite / PLSQL / Java || (Oracle Forms / Reports) I mainly work on :) - just doing the GitHub tutorial tonight, then bed. Never ever worked on any opensource projects - hope I can help you guys somehow :)

@danehrlich1

This comment has been minimized.

Copy link
Member

danehrlich1 commented Jan 3, 2019

@kkvvii84

This comment has been minimized.

Copy link
Contributor

kkvvii84 commented Jan 7, 2019

Hi again,

Sorry for the delay, I only get so much spare time.

Basically if I'm understanding this correctly

$query = "select * from calendar where pid=$pid and time>now() order by time limit 1";

Will become

$query = "select * from calendar where pid = ? and time>now() order by time limit 1", array($pid);

So will

$query = "select * from calendar where time like '$datetime%' and owner like '$owner' and groupname like '$groupname' order by time";

becomes

$query = "select * from calendar where time like ? and owner like ? and groupname like ? order by time", array($datetime%, $owner, $groupname);

Is this correct?, even with the like condition ?

Thanks

kkvvii84 added a commit to kkvvii84/openemr that referenced this issue Jan 7, 2019

Fixing issue openemr#2097
Removing non bind variables in SQL calls
@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 8, 2019

hi @kkvvii84 ,
very, very close. it will become:

$query = "select * from calendar where time like ? and owner like ? and groupname like ? order by time", array($datetime.'%', $owner, $groupname);
@kkvvii84

This comment has been minimized.

Copy link
Contributor

kkvvii84 commented Jan 8, 2019

@bradymiller thanks I have applied the suggested fixes.

Will I just continue with -

  • clinical_rules.php
  • create_ssl_certificate.php
  • formdata.inc.php

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

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 9, 2019

hi @kkvvii84 ,
Those scripts should be fine. Try these, though:
library/transaction.inc
library/lists.inc

@kkvvii84

This comment has been minimized.

Copy link
Contributor

kkvvii84 commented Jan 11, 2019

@bradymiller - Is there any other files you'd like me to look at?

Thanks

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 12, 2019

hi @kkvvii84 ,
Check out the last query in library/transaction.inc :)

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