Attachment functionality of OpenMRS Reference Application is vulnerable

Openmrs RA provides functionality for a user to add an attachment(document or file) to a patient as a Complex Obs. However, this functionality is vulnerable. An attacker can submit a malicious script. For example, a user can upload a .php file solong as its weight is below the maximum set size.

Actually, i even uploaded a .js file and the process executed successfully.

A possible solution is to sanitize the input from the client size by being strict on what you want. I guess this is responsible for file upload(correct me if i am wrong) in this form

Another requirement is to make sure that the user has file upload permissions. But I guess this feature is being worked upon BU @sharif

How can I restrict file upload basing their type using the client side. Or this will be better if it is implemented from the server side.

cc @isears @mksd @sharif @ibacher and anyone willing to help

Thanks

you can find how this things are saved ie attachments from openmrs-module-attachments/ComplexObsSaver.java at master · openmrs/openmrs-module-attachments · GitHub For restricting uploads,i know we have a couple of doc formats that can be saved,which formats are you looking at to be only saved while uploading attachments

1 Like

I think we would upload a document(lile pdf), image and zipped files only

i have looked through the file but i don’t see any sieve to get the most relevant file types.

If i am to use this saver class to sieve file types that means i am validating the uploaded file further from the client side.

I think there is a way i can implement this using just the html files or the js files such that the file upload window shows only the relevant files.

since we are using dropzone, i want to dod it like like

Dropzone.options.visit-documents-dropzone= {
   acceptedFiles: 'image/*' //image as an example.
};

Maybe we should start by just disallowing the upload of a Window or *nix executable file. It’s difficult to predict all the different filetypes implementers might need and we should try not to break existing workflows.

Be careful about implementing filtering client-side. It’s likely that most client-side filetype upload prevention methods would be trivially circumventable by manually tampering with the POST request.

2 Likes

Yes this is a better option.

Am then going to implement this in https://github.com/openmrs/openmrs-module-attachments/blob/master/api/src/main/java/org/openmrs/module/attachments/ComplexObsSaver.java because it seams to be a better option.

Thank you.

@jnsereko right at this point file gives you the MIME type: file.getContentType().

You could have a validation method that would take the MIME type as an argument and would decide if the upload is accepted or refused based on the MIME type.

1 Like

FWIW, we would expect a wide variety of potential file types across images, documents (text, PDF, Word, etc.), and videos to support a wide variety of use cases (documentation, clinical images, scanned files, data from ancillary systems, patients uploading documentation or images for their provider, etc.).

Blocking obvious bad players (e.g., executables and scripts) would be a good place to start.

1 Like

Thanks @burke @mksd @isears and @herbert24 I have been able to create a PR
please kindly review.

i think that i could use application/octet-stream (MIME type for executable files) according to MIME Types, Their File Extensions, and Applications but i think it isn’t right basing on the iana media types

Is there any MIME type that wraps all scripts together.

1 Like

why not simply check the type you wana exclude here and return the appropriate message openmrs-module-attachments/AttachmentResource1_10.java at bd8f77ba063203a6898b33ef4a856c64444b9738 · openmrs/openmrs-module-attachments · GitHub

this is a great idea @herbert24 thanks