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

EMPT-41: Prevent upload of executable and script files. #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jnsereko
Copy link

Basing on the discussion we had at https://talk.openmrs.org/t/attachment-functionality-of-openmrs-reference-application-is-vulnerable/33208/7.

this functionality is to prevent upload of an executable or a script in order to prevent vulnerabilities in this module.
I haven't created any ticket for this feature. That is the reason why i haven't attached its link here.

cc @isears @HerbertYiga @sherrif10

@@ -345,6 +345,12 @@ public static double getCompressionRatio(double fileByteSize, double maxByteSize
*/
public static ContentFamily getContentFamily(String mimeType) {
ContentFamily contentFamily = ContentFamily.OTHER;
if (StringUtils.equals(mimeType, "some type")) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately i haven't been able to get the correct MIME type for all executables.
i think that i could use application/octet-stream according to https://www.lifewire.com/file-extensions-and-mime-types-3469109 but i think it isn't right basing on the iana media types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnsereko maybe take it the other way around. Think of a couple of obviously insecure file types (.js, .exe, .sh, .sql, ... etc) and see what the MIME types are for those guys, and start building an exclusion list that way.
You could probably start with:

  • application/octet-stream
  • text/javascript (and its cousins, see here).

@isears @ibacher?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just denying the upload of aplication/octet-stream and text/javascript would be a big improvement over what we have now (which is no filtering).

Building that initial functionality is the difficult part. Once we have it for one or two mime-types we can always extend it to include others if we need to.

if (StringUtils.equals(mimeType, "some type")) {
contentFamily = ContentFamily.EXECUTABLE;
}
if (StringUtils.equals(mimeType, "some type")) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately i haven't been able to get the correct MIME type for all scripts.

Copy link
Contributor

@HerbertYiga HerbertYiga Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply check the type you wana exclude here and return the appropriate message

public Object upload(MultipartFile file, RequestContext context) throws ResponseException, IOException {

@@ -12,6 +12,8 @@
public class AttachmentsConstants {

public static enum ContentFamily {
EXECUTABLE,
SCRIPT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnsereko you basically treat everything as executables in this PR, there is no reason to distinguish scripts from executables.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oohh.. great suggestion @mks-d. Thanks

@@ -345,6 +345,12 @@ public static double getCompressionRatio(double fileByteSize, double maxByteSize
*/
public static ContentFamily getContentFamily(String mimeType) {
ContentFamily contentFamily = ContentFamily.OTHER;
if (StringUtils.equals(mimeType, "some type")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnsereko maybe take it the other way around. Think of a couple of obviously insecure file types (.js, .exe, .sh, .sql, ... etc) and see what the MIME types are for those guys, and start building an exclusion list that way.
You could probably start with:

  • application/octet-stream
  • text/javascript (and its cousins, see here).

@isears @ibacher?

@mks-d mks-d changed the title EMPT-41: Prevent upload of executable and script files EMPT-41: Prevent upload of executable and script files. Apr 26, 2021
@sherrif10
Copy link
Member

sherrif10 commented Apr 28, 2021

Thanks @jnsereko for the awesome work, Since this ticket is worth doing, i think its ok to create a ticket in jira for it and update it here with the ticket id, Also the branch name should reflect ATTACHMENT tag cc @isears @mks-d

Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests as well.

@@ -70,7 +69,9 @@
public static final String INSTRUCTIONS_PREFIX = "instructions";

public static final String UNKNOWN_MIME_TYPE = "application/octet-stream";


public static final String[] EXECUTABLE_MIME_TYPE = {"application/octet-stream", "type/javascript"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static final List<String> EXECUTABLE_MIME_TYPES = Arrays.asList("application/octet-stream", "type/javascript");

(TYPES, plural)

Comment on lines 349 to 350
if (getMimeType(mimeType)) {
contentFamily = ContentFamily.EXECUTABLE;
}
Copy link
Member

@mks-d mks-d Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (EXECUTABLE_MIME_TYPES.contains(mimeType)) {
  contentFamily = ContentFamily.EXECUTABLE;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented suggestions and added tests. Thanks

cc @mks-d @isears @sherrif10

@jnsereko jnsereko requested a review from mks-d April 29, 2021 19:25
@jnsereko jnsereko requested a review from mks-d April 30, 2021 14:54
@jnsereko
Copy link
Author

Thanks @mks-d @isears @sherrif10 @HerbertYiga for reviewing this. I have added mime types for .py files, .jar files, .php files, .sh files, .exe files, .sql files, and visual basic files

@@ -69,6 +73,9 @@

public static final String UNKNOWN_MIME_TYPE = "application/octet-stream";

public static final List<String> EXECUTABLE_MIME_TYPES = Arrays.asList("application/vnd.microsoft.portable-executable",
"text/javascript", "application/javascript", "application/sql", "application/x-sh", "application/java-archive" "application/x-httpd-php","application/x-vbs", "application/x-python-code", "text/vbscript", "text/x-python");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isears @ibacher, does it look like a decent shortlist of bad guys?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like mimetypes can get pretty crazy, but maybe we should add in application/x-ms-installer and application/x-elf as well @jnsereko

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably drop application/sql from the list. Also, a few additions:

  • application/x-applescript
  • application/x-ruby
  • application/x-perl
  • application/wasm

That should cover many of the more common scripting languages (I can't find any even semi-standard MIME Types for batch and ps1 files).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @ibacher i have removed the application/sql mime type added added more as you suggested. I guess this catch is good for the start.
@mks-d @isears @sherrif10 @dkayiwa

@jnsereko jnsereko force-pushed the EMPT-41 branch 4 times, most recently from 49d452c to 39033f9 Compare May 5, 2021 10:03
@jnsereko jnsereko requested a review from mks-d May 6, 2021 13:34
@@ -439,6 +439,27 @@ public void postAttachment_shouldNotUploadFileAboveSizeLimit() throws Exception
SimpleObject response = deserialize(handle(request));
}

@Test(expected = IllegalRequestException.class)
public void postAttachment_shouldNotUplodExecutbleFile() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo here by maybe rename this to postAttachment_shouldThrowOnExecutableFile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mks-d I have changed the test method name..

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

Successfully merging this pull request may close these issues.

6 participants