Add server side validation for uploaded file types #173960

SiddharthMantri · 2023-12-26T12:18:19Z

Summary

Closes https://github.com/elastic/security/issues/1839

Fixes

Introduced a MIME type check for the server endpoint for image upload. The check runs against the same allowed file types for the UI and returns an error if not matched.

Tesing

Use the POST kbn://internal/security/user_profile/_data endpoint with a body as follows (substituting your own base64 image string):

{
  "avatar": {
    "imageUrl": "[image as base64 encoded string]"
   }
}

The beginning of the base64 string should look something like "data:image/png;base64,...", where 'png' is the image format. Verify that all supported image formats work and the API returns 200 for each.
In the base64 string, change the image format (e.g. 'png') to a non-supported format (e.g. 'gnp') and verify a 415 "Unsupported Media Type" error occurs.
In the base64 string, change the "data:image/png;base64" to "data:file/pdf;base64" and verify a 415 "Unsupported Media Type" error occurs.

Release notes

elasticmachine · 2023-12-28T12:06:05Z

Pinging @elastic/kibana-security (Team:Security)

jeramysoucy · 2023-12-28T16:15:54Z

/ci

jeramysoucy

Overall looks good! I just had couple of comments/questions.

x-pack/plugins/security/public/account_management/user_profile/utils.ts

jeramysoucy · 2023-12-28T18:20:45Z

x-pack/plugins/security/server/routes/user_profile/update.ts

+
+      const [, mimeType] = matches;
+
+      if (!IMAGE_FILE_TYPES.includes(mimeType)) {


Should we also constrain the image size (automatically resize) as we do via the UI?
https://github.com/elastic/kibana/blob/079db3e7ebcbed934c39d0efe67821317fe9eaec/x-pack/plugins/security/public/account_management/user_profile/utils.ts#L63C8-L63C8

I did notice that at some point the payload max size kicks in, which would limit the incoming image size, but not to same degree that the UI resizes.

{
"statusCode": 413,
"error": "Request Entity Too Large",
"message": "Payload content length greater than maximum allowed: 1048576"
}

You can see that if I upload via API, the image is much more clear because it is not resized:

But if I upload via the UI, it gets resized:

Good idea! As image resizing is done via the Image and Canvas APIs on the browser, we'll need to use libraries to do the same for the node server which means adding a new dependency. Where does this dependency go? Top level package.json?

Adding a separate issue for this. https://github.com/elastic/security/issues/1868

jeramysoucy · 2023-12-28T18:51:09Z

In testing this PR I noticed that the Avatar image upload on the Edit User Profile page behaves not as I would expect. So long as you've uploaded an image file, any subsequent non-supported file dragged to the image input does not provide any feedback. I will create an issue for this.

SiddharthMantri · 2024-01-13T12:49:49Z

/ci

SiddharthMantri · 2024-01-17T10:51:44Z

/ci

jeramysoucy

LGTM! I tested with various good and bad inputs, including large images which get rejected due to the payload size.

SiddharthMantri · 2024-01-29T08:47:48Z

@elasticmachine merge upstream

kibana-ci · 2024-01-29T09:58:00Z

💚 Build Succeeded

Buildkite Build
Commit: c8578d6

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`security`	580.3KB	580.2KB	-35.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`security`	64.6KB	64.7KB	+91.0B

History

💚 Build #187856 succeeded 43cc864
💚 Build #187855 succeeded 0f64429
💛 Build #187346 was flaky 0f64429
💔 Build #187338 failed 0f64429
💔 Build #187269 failed d52c6cf
💔 Build #187242 failed 63cb71b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

## Summary Closes elastic/security#1839 ## Fixes - Introduced a MIME type check for the server endpoint for image upload. The check runs against the same allowed file types for the UI and returns an error if not matched. ### Tesing 1. Use the `POST kbn://internal/security/user_profile/_data` endpoint with a body as follows (substituting your own base64 image string): ``` { "avatar": { "imageUrl": "[image as base64 encoded string]" } } ``` 2. The beginning of the base64 string should look something like "data:image/png;base64,...", where 'png' is the image format. Verify that all supported image formats work and the API returns 200 for each. 3. In the base64 string, change the image format (e.g. 'png') to a non-supported format (e.g. 'gnp') and verify a 415 "Unsupported Media Type" error occurs. 4. In the base64 string, change the "data:image/png;base64" to "data:file/pdf;base64" and verify a 415 "Unsupported Media Type" error occurs. ## Release notes --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary Closes https://github.com/elastic/security/issues/1839 ## Fixes - Introduced a MIME type check for the server endpoint for image upload. The check runs against the same allowed file types for the UI and returns an error if not matched. ### Tesing 1. Use the `POST kbn://internal/security/user_profile/_data` endpoint with a body as follows (substituting your own base64 image string): ``` { "avatar": { "imageUrl": "[image as base64 encoded string]" } } ``` 2. The beginning of the base64 string should look something like "data:image/png;base64,...", where 'png' is the image format. Verify that all supported image formats work and the API returns 200 for each. 3. In the base64 string, change the image format (e.g. 'png') to a non-supported format (e.g. 'gnp') and verify a 415 "Unsupported Media Type" error occurs. 4. In the base64 string, change the "data:image/png;base64" to "data:file/pdf;base64" and verify a 415 "Unsupported Media Type" error occurs. ## Release notes --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary Closes elastic/security#1839 ## Fixes - Introduced a MIME type check for the server endpoint for image upload. The check runs against the same allowed file types for the UI and returns an error if not matched. ### Tesing 1. Use the `POST kbn://internal/security/user_profile/_data` endpoint with a body as follows (substituting your own base64 image string): ``` { "avatar": { "imageUrl": "[image as base64 encoded string]" } } ``` 2. The beginning of the base64 string should look something like "data:image/png;base64,...", where 'png' is the image format. Verify that all supported image formats work and the API returns 200 for each. 3. In the base64 string, change the image format (e.g. 'png') to a non-supported format (e.g. 'gnp') and verify a 415 "Unsupported Media Type" error occurs. 4. In the base64 string, change the "data:image/png;base64" to "data:file/pdf;base64" and verify a 415 "Unsupported Media Type" error occurs. ## Release notes --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

SiddharthMantri added 2 commits December 26, 2023 13:17

Add server side validation for uploaded file types to image url

82e4a88

use correct response code for unsupported media type

fca7724

SiddharthMantri added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Users/Roles/API Keys release_note:fix labels Dec 28, 2023

Merge branch 'main' into serverside-file-validation

59fee3d

SiddharthMantri marked this pull request as ready for review December 28, 2023 12:06

SiddharthMantri requested a review from a team as a code owner December 28, 2023 12:06

jeramysoucy self-requested a review December 28, 2023 16:24

jeramysoucy requested changes Dec 28, 2023

View reviewed changes

jeramysoucy mentioned this pull request Dec 28, 2023

User profile avatar upload control does not provide feedback on drag & drop #174044

Open

SiddharthMantri and others added 5 commits December 29, 2023 10:10

update imports and remove export for image file type constant

ef42f09

Merge branch 'main' into serverside-file-validation

e60693e

Merge branch 'main' into serverside-file-validation

a7f3f10

Merge branch 'main' into serverside-file-validation

8125ead

Use valid base64 string in tests and update endpoint check

a02b323

SiddharthMantri requested a review from a team as a code owner January 12, 2024 12:41

SiddharthMantri and others added 3 commits January 12, 2024 16:05

Use test image data url for trial and basic tests

63cb71b

Merge branch 'main' into serverside-file-validation

d52c6cf

Update tests and use test results to fix code

0f64429

SiddharthMantri requested a review from jeramysoucy January 13, 2024 14:51

Merge branch 'main' into serverside-file-validation

43cc864

jeramysoucy approved these changes Jan 22, 2024

View reviewed changes

Merge branch 'main' into serverside-file-validation

c8578d6

cnasikas approved these changes Jan 30, 2024

View reviewed changes

SiddharthMantri merged commit 9ab8b1d into elastic:main Jan 30, 2024
17 checks passed

kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add server side validation for uploaded file types #173960

Add server side validation for uploaded file types #173960

SiddharthMantri commented Dec 26, 2023 •

edited by jeramysoucy

elasticmachine commented Dec 28, 2023

jeramysoucy commented Dec 28, 2023

jeramysoucy left a comment

jeramysoucy Dec 28, 2023

jeramysoucy Dec 28, 2023

SiddharthMantri Dec 29, 2023

SiddharthMantri Jan 11, 2024

jeramysoucy commented Dec 28, 2023

SiddharthMantri commented Jan 13, 2024

SiddharthMantri commented Jan 17, 2024

jeramysoucy left a comment

SiddharthMantri commented Jan 29, 2024

kibana-ci commented Jan 29, 2024


		const [, mimeType] = matches;

		if (!IMAGE_FILE_TYPES.includes(mimeType)) {

Add server side validation for uploaded file types #173960

Add server side validation for uploaded file types #173960

Conversation

SiddharthMantri commented Dec 26, 2023 • edited by jeramysoucy

Summary

Fixes

Tesing

Release notes

elasticmachine commented Dec 28, 2023

jeramysoucy commented Dec 28, 2023

jeramysoucy left a comment

Choose a reason for hiding this comment

jeramysoucy Dec 28, 2023

Choose a reason for hiding this comment

jeramysoucy Dec 28, 2023

Choose a reason for hiding this comment

SiddharthMantri Dec 29, 2023

Choose a reason for hiding this comment

SiddharthMantri Jan 11, 2024

Choose a reason for hiding this comment

jeramysoucy commented Dec 28, 2023

SiddharthMantri commented Jan 13, 2024

SiddharthMantri commented Jan 17, 2024

jeramysoucy left a comment

Choose a reason for hiding this comment

SiddharthMantri commented Jan 29, 2024

kibana-ci commented Jan 29, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

SiddharthMantri commented Dec 26, 2023 •

edited by jeramysoucy