Skip to content
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

OpenEMR v5_0_1_4: SQL Injection in interface/logview/logview.php #1782

Closed
l00ph0le opened this issue Jul 25, 2018 · 11 comments
Closed

OpenEMR v5_0_1_4: SQL Injection in interface/logview/logview.php #1782

l00ph0le opened this issue Jul 25, 2018 · 11 comments

Comments

@l00ph0le
Copy link

l00ph0le commented Jul 25, 2018

I found an application security issue in interface/logview/logview.php of OpenEMR v5_0_1_4 and likely earlier versions.

The vulnerability exists due to a lack of sanitation of user-supplied input. The vulnerability could allow remote authenticated attackers to inject SQL commands via the 'sortby' parameter.

I think the issue is being triggered by the sql query in /library/log.inc on line 217.

$sql .= " ORDER BY ".$sortby." ".escape_sort_order($direction); // descending order

PoC:
http://192.168.246.144/openemr-5.0.1/interface/logview/logview.php?direction=asc&sortby=1,extractvalue(0x0a,concat(0x0a,(SELECT%20@@hostname)))--&csum=&start_date=2018-07-26+00%3A00&end_date=2018-07-26+23%3A59&form_patient=Click+To+Select&form_pid=&form_user=&eventname=&type_event=&event=

@bradymiller
Copy link
Sponsor Member

hi @l00ph0le ,

Thanks for the report! Fixes are in this PR:
#1783

For future specific security vulnerability reports, rec. emailing them to me at brady.g.miller@gmail.com rather than posting them publicly (and even better if email it encrypted via my public PGP key at: https://pgp.mit.edu/pks/lookup?op=get&search=0x27DEF05B1A8A6D4F).

Thanks!
-brady

@l00ph0le
Copy link
Author

Hi Brady,

Can you post the security vulnerability contact information on your main website or in the README of the project? I would have done this had I seen it somewhere. I opened the issues based on previous issues I found in the project, I thought it was the preferred reporting method.

Thank you

~Nick

@bradymiller
Copy link
Sponsor Member

bradymiller commented Jul 28, 2018

Hi @l00ph0le ,

Very good point. I just added this information to the README:
782dab2

thanks,
-brady

@kurtseifried
Copy link

Just a heads up this is already covered by CVE-2014-5462 https://www.portcullis-security.com/security-research-and-downloads/security-advisories/cve-2014-5462/ (pointed out to me by MITRE when I tried to assign a new CVE for this issue). Please note that there are additional SQL injection flaws you may need to fix (otherwise if you only fix part of CVE-2014-5462 we'll need to assign another CVE for the unfixed SQL injections), Thanks!

@bradymiller
Copy link
Sponsor Member

hi @kurtseifried ,
Thanks for the heads up. Will look at that report and make the needed fixes.
-brady

@bradymiller
Copy link
Sponsor Member

Good project for somebody related to security:
Look through the security issues in below link and ensure they are all addressed in OpenEMR's codebase:
https://www.portcullis-security.com/security-research-and-downloads/security-advisories/cve-2014-5462/

@bradymiller
Copy link
Sponsor Member

I confirmed that all above vulnerabilities in above link have been addressed, so closing this issue.

@NicoleG25
Copy link

@bradymiller Could you kindly point out where CVE-2014-5462 was addressed ? (in what commit)
Thanks in advance ! :)

@bradymiller
Copy link
Sponsor Member

hi @NicoleG25 ,
This was an old CVE, so I just checked to see if the vulnerabilities still existed back in 5/2019 and they didn't. The vulnerabilities were likely fixed over many commits, which would be time consuming to pinpoint and list for each fixed vulnerability.

@NicoleG25
Copy link

hi @NicoleG25 ,
This was an old CVE, so I just checked to see if the vulnerabilities still existed back in 5/2019 and they didn't. The vulnerabilities were likely fixed over many commits, which would be time consuming to pinpoint and list for each fixed vulnerability.

Ah I see. so what would be the 'safest' version in your opinion where the vulnerabilities no longer existed ?
@bradymiller

@bradymiller
Copy link
Sponsor Member

hi @NicoleG25 , Definitely should use most recent production version, OpenEMR 5.0.2, since that version had lots of security fixes/update/improvements/features (the systematic codebase refactoring for 5.0.2 likely was what fixed most of the vulnerabilities in that old CVE):
https://www.open-emr.org/blog/openemr-version-502-released/

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

No branches or pull requests

5 participants