Authenticated Local File Disclosure in import_template.php

[prodigysml]

The Issue

Local file disclosure is a vulnerability which allows an attacker to disclose the contents of files on the server. An attacker can use this vulnerability to disclose the contents of sensitive files like /etc/passwd, config files, etc.

In lh-ehr, an attacker must be authenticated to perform this attack. Should the attacker know the path to a file and the web server user has sufficient access to read the file, the contents of the file will be echoed in the page.

Where the Issue Occurred

The following code snippet displays the usage of the file_get_contents function in PHP within the lh-ehr application: https://github.com/LibreHealthIO/lh-ehr/blob/cacaa71dca75c3bf53cdce506fbb62e8b0593f76/patient_portal/import_template.php#L24

[naveen17797]

does the authenicated user needed to be admin or normal user @ProDigySML

[prodigysml]

I'm unsure about that.

On Mon, 23 Jul 2018, 10:03 PM naveen [email protected] wrote:

does the authenicated user needed to be admin or normal user @ProDigySML https://github.com/ProDigySML

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LibreHealthIO/lh-ehr/issues/1210#issuecomment-407034213, or mute the thread https://github.com/notifications/unsubscribe-auth/AQNZ06gwPg-9_yPDFHVrAdC8fHxGy_Faks5uJbuOgaJpZM4Va1QQ .

[prodigysml]

A normal authenticated user should suffice as far as I know.

[aethelwulffe]

It is an easy test, aside from just looking at the code.

1. An Admin user in a setting like this often needs big data type tools (well, big data for a small clinic).

2. Admin superuser accounts have significant leeway to do things, though of course logging happens.

3. Many times someone at, say, the Washington Post has looked at the precursor project to this one and declared a sensational headline about injection attacks when that SAME superuser also had full access to a database tool, all the database auth strings etc...

Unauthorized access is one thing. Please help us smash those down.
Authorized access for lower levels of Access Control should either be fixed by limiting access to the feature itself, or by fixing issues like this. Authorized superuser access is a completely different beast. The basis of every one of these reports to my knowledge has been based on someone hacking their own machine after installing the software, not using any server configuration recommendations, then using the default admin account to do an injection attack.

It is an easy test, aside from just looking at the code.

1. An Admin user in a setting like this often needs big data type tools (well, big data for a small clinic).

2. Admin superuser accounts have significant leeway to do things, though of course logging happens.

3. Many times someone at, say, the Washington Post has looked at the precursor project to this one and declared a sensational headline about injection attacks when that SAME superuser also had full access to a database tool, all the database auth strings etc...

Unauthorized access is one thing. Please help us smash those down.
Authorized access for lower levels of Access Control should either be fixed by limiting access to the feature itself, or by fixing issues like this. Authorized superuser access is a completely different beast. The basis of every one of these reports to my knowledge has been based on someone hacking their own machine after installing the software, not using any server configuration recommendations, then using the default admin account to do an injection attack.

[aethelwulffe]

Don't get me wrong: THANK YOU THANK YOU THANK YOU for posting these vulnerability notices! Welcome to the team, and I hope to see more. When you have an authorized injection scenario, please take note of what level of user access can perform the attack, and just as importantly what the recommended server limitations are. There are no "unlimited" uploads in most server configs, so that would be one starting point. Often the issue is that the users hit a server config limitation that prevents them from doing their job, vs. the other way around.

[aethelwulffe]

I think this may be a patient uploading a file within the patient portal. I will also state that I have never done a security review of this particular module. That said, the best way to test what a patient logging into the patient portal system can do. The other theory is that this is to upload form template files for the patient to use, which is an admin feature. I would have to check more, as looking at the single file isn't doing it for me.

[naveen17797]

@aethelwulffe "Local file disclosure is a vulnerability which allows an attacker to disclose the contents of files on the server. An attacker can use this vulnerability to disclose the contents of sensitive files like /etc/passwd, config files, etc.", @aethelwulffe By editing those files the user can even remove the authenciation and execute all the queries he need, i will try to exploit in my own installation now

[teryhill]

@naveen17797 and @ProDigySML GSoC is finishing up and theses fixes would be good first commits for @ProDigySML If they would like to contribute and help us out.

[C-Sto]

@aethelwulffe I've confirmed this is possible when using a session token obtained with 'physician' privileges which as I understand, is not a 'super admin'.

I'm unable to test from the perspective of a patient, as the patient portal doesn't work in the 2.0.0 release, nor when cloning from the repo at this point in time.

[aethelwulffe]

I was just playing with the patient portal the other day @C-Sto . No, "Physician" isn't admin, though it is treated as a pretty high level access, often including the ability to do certain admin tasks...but there should be a specific ACL group that is allowed to modify the templates. If the templates are able to execute a wide range of code, yet must be capable of being modified, this should be an ACL config item at least. The main issue is if a patient can access the template functions, which I do not believe they can. -So, this is ultimately still a hole that must be plugged. It should at least check the user access level before doing anything. if (acl_check('admin','super'){}else{echo'nya-nya!';}

[C-Sto]

@aethelwulffe Patient portal registration is broken in the current repo version

https://github.com/LibreHealthIO/lh-ehr/blob/5b5f427c4742f901e426f17325fb0aaf8209e0bb/patient_portal/account/register.php#L492

'patientdata' page does not exist. What is the most recent release that has a working patient portal? Or what can I do to manually set up a patient that can interact with the portal?

[aethelwulffe]

You must configure the location in globals/patient portal

On 2018-08-13 13:33, C_Sto wrote:

@aethelwulffe https://github.com/aethelwulffe Patient portal registration is broken in the current repo version

https://github.com/LibreHealthIO/lh-ehr/blob/5b5f427c4742f901e426f17325fb0aaf8209e0bb/patient_portal/account/register.php#L492

'patientdata' page does not exist. What is the most recent release that has a working patient portal? Or what can I do to manually set up a patient that can interact with the portal?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LibreHealthIO/lh-ehr/issues/1210#issuecomment-412600104, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhzF2P4qamzbd6oS3bM7IdBzJybGJ2Lks5uQbhhgaJpZM4Va1QQ.

[NicoleG25]

Do you plan to address this vulnerability? Note that CVE-2018-1000645 was assigned.

[wisdommatt]

The Issue

Local file disclosure is a vulnerability which allows an attacker to disclose the contents of files on the server. An attacker can use this vulnerability to disclose the contents of sensitive files like /etc/passwd, config files, etc.

In lh-ehr, an attacker must be authenticated to perform this attack. Should the attacker know the path to a file and the web server user has sufficient access to read the file, the contents of the file will be echoed in the page.

Where the Issue Occurred

The following code snippet displays the usage of the file_get_contents function in PHP within the lh-ehr application: https://github.com/LibreHealthIO/lh-ehr/blob/cacaa71dca75c3bf53cdce506fbb62e8b0593f76/patient_portal/import_template.php#L24

This vulnerability has already been fixed --- the file for fetching local files is now manage_site_files.php --- check manage_site_files.php on line 46 // Sanity check to prevent evildoing. if (!in_array($form_filename, $my_files)) $form_filename = '';

[attritionorg]

@wisdommatt Can you link to the fixing commit please?

[wisdommatt]

@wisdommatt Can you link to the fixing commit please?

The issue has been opened since 2018 and if you cross check the code now you will notice it has already been fixed.

[attritionorg]

@wisdommatt There are no commits for import_template.php since the original commit (half a year before this issue). The latest version released is 2.0.0 in 2017. Searching for terms from this ticket that might appear in commit notes yields nothing.

There is no readily available way to figure out where this was fixed, that's why I asked for a pointer.

[NicoleG25]

@wisdommatt , following @attritionorg reply. there is indeed no way to pinpoint where the issue was fixed and we were hoping that you could steer us in the right direction.

Cheers and have a nice weekend !