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 #2053

Open
danehrlich1 opened this Issue Dec 21, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@danehrlich1
Copy link
Member

danehrlich1 commented Dec 21, 2018

sql-injection fixes in library/classes directory

SQL queries in the application need to use binded parameters.

Note, this application uses the ADODB library (basically an ORM): http://adodb.org/dokuwiki/doku.php

Please contact dan@ehrlichserver.com for more information on getting started.

@danehrlich1

This comment has been minimized.

Copy link
Member

danehrlich1 commented Dec 21, 2018

Please see this commit for changes that were made just like this to certain parts of the app: 35b4096

Your task will be to find other parts of the app that need this update. Some of the SQL queries you'll find will be bizarre and not worth redoing (Brady Miller will probably need to closely look at the query to decide what to be done), but there should be at least a dozen, if not a few dozen quick wins to find in the code.

@bradymiller I had a request to create an issue for someone to get started on. They saw some of the JS issues I posted for first timers, and asked for PHP first timer issues, which is why I created this.

@danehrlich1

This comment has been minimized.

Copy link
Member

danehrlich1 commented Dec 21, 2018

NOTE: for the first pull request, start in the portal/ folder. If you go elsewhere you might end up doing work that has already been done by someone else in another pull request that hasn't been merged. I've checked and there are queries waiting to be fixed: https://github.com/openemr/openemr/tree/master/portal

@kacyw

This comment has been minimized.

Copy link
Contributor

kacyw commented Jan 8, 2019

@danehrlich1 I might be missing something, but I've combed through the whole portal directory and can't find any sql queries that aren't using parameter binding, where appropriate. Perhaps this issue was already resolved by someone else?

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 9, 2019

hi @kacyw,

Check out:
library/classes/Address.class.php
And then just start looking through all the scripts in that directory.

-brady

@kacy-weakley

This comment has been minimized.

Copy link

kacy-weakley commented Jan 9, 2019

@bradymiller Yeah, I definitely found missing parameter binding in other directories. I saw other sql parm binding tickets and didn't want to step on others' work. @danehrlich1 specified portal/ in this ticket, so that was where I looked for this issue. If there isn't another ticket to comb through the library directory, I am happy take on that instead.

@kacy-weakley

This comment has been minimized.

Copy link

kacy-weakley commented Jan 9, 2019

@bradymiller Actually, it does look like #2096 links to the library directory and has been assigned already. Is there another directory I should take on instead?

@bradymiller bradymiller changed the title Help restructure SQL queries Fix SQL Issues in these files Jan 9, 2019

@bradymiller

This comment has been minimized.

Copy link
Member

bradymiller commented Jan 9, 2019

hi @kacy-weakley (@kacyw),
At this point, the library directory is where most of the sql-injection action is(and to much for 1 dev). I changed the 2096 ticket to not include the library/classes/, so it is all yours. When (and if) if you get sick of doing sql-injection just let me know, and I can find some other cool things to work on.
-brady

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

@kacy-weakley

This comment has been minimized.

Copy link

kacy-weakley commented Jan 9, 2019

Sounds good. Thanks!

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