Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Enforce PNG check when posting images #2301

Closed
jvehent opened this issue Mar 7, 2017 · 6 comments · Fixed by #2547
Closed

Enforce PNG check when posting images #2301

jvehent opened this issue Mar 7, 2017 · 6 comments · Fixed by #2547
Assignees
Labels
security Security issue: can be an active issue, or related to security hygene

Comments

@jvehent
Copy link
Contributor

jvehent commented Mar 7, 2017

Only images in PNG format must be permitted through the pageshot app when a user posts a new shot. The app must check the magic number of the image.

PNG image files begin with an 8-byte signature which identifies the file as a PNG file and allows detection of common file transfer problems: \211 P N G \r \n \032 \n (89 50 4E 47 0D 0A 1A 0A).

@jvehent
Copy link
Contributor Author

jvehent commented Mar 7, 2017

Also, when uploading to S3, the content type should be forced to image/png.

@jvehent jvehent added the security Security issue: can be an active issue, or related to security hygene label Mar 7, 2017
@ianb ianb removed the beta blocker label Mar 7, 2017
@ianb ianb added this to the Page Shot in 54 milestone Mar 7, 2017
@ianb
Copy link
Contributor

ianb commented Mar 7, 2017

Note: moving to the 54 Milestone (to show we need it for 54), but removing the beta blocker label, as those are for tickets that are required for the add-on landing in 54 beta (while this is a server requirement).

@ghost
Copy link

ghost commented Mar 8, 2017

Is just checking the magic number enough? On a couple other sites we convert from an image to an image to strip out any weirdness (like that PHP injection thing buried in jpg comments).

@jvehent
Copy link
Contributor Author

jvehent commented Mar 8, 2017

That would be preferable indeed if the computing cost is within limits.
It does bring the issue of crashing the process with malformed images, and other vulnerabitilies in image libraries (yurk, imagemagick...).

@g-k
Copy link
Contributor

g-k commented Mar 20, 2017

ulfr pointed out this blog post on idat chunks too: https://www.idontplaydarts.com/2012/06/encoding-web-shells-in-png-idat-chunks/

@g-k
Copy link
Contributor

g-k commented Mar 31, 2017

The Firefox Accounts profile server uses reviewed code for handling PNG and JPEG uploads for avatars that would probably be helpful.

https://github.com/mozilla/fxa-profile-server/blob/05ae545aba1044700b1952e9916f9b817daf3e2b/lib/compute/index.js#L79

https://github.com/mozilla/fxa-profile-server/blob/b6a2816914b05c99c8c43b5be2b452d2966f3f93/lib/config.js#L90

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
security Security issue: can be an active issue, or related to security hygene
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants