Page MenuHomePhabricator

`ArcanistChmodLinter` should not allow certain MIME types to be executable
ClosedPublic

Authored by joshuaspence on Jun 25 2014, 5:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 12:53 AM
Unknown Object (File)
Tue, Dec 17, 10:51 PM
Unknown Object (File)
Mon, Dec 9, 2:48 PM
Unknown Object (File)
Mon, Dec 9, 2:30 PM
Unknown Object (File)
Sat, Nov 30, 4:27 AM
Unknown Object (File)
Wed, Nov 27, 12:01 PM
Unknown Object (File)
Sat, Nov 23, 2:43 PM
Unknown Object (File)
Sat, Nov 23, 4:48 AM

Details

Summary

Fixes T5466. An image is an example of a binary which should not be executable. Modify the ArcanistChmodLinter to disallow certain blacklisted MIME types from being executable.

Test Plan

Created an executable image file and ran arc lint over this file.

Diff Detail

Repository
rARC Arcanist
Branch
chmod-images
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1390
Build 1390: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to `ArcanistChmodLinter` should not allow images to be executable.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
src/lint/linter/ArcanistChmodLinter.php
49

Actually, this function requires the Exif extension, so we should probably wrap in function_exists.

joshuaspence edited edge metadata.
  • Return as soon as possible.

You could presumably also use identify from ImageMagick. Probably overkill though.

(other thought: it should be clear to users that exif is needed to lint image+executable. I wouldn't expect users to find out on their own)

You could presumably also use identify from ImageMagick. Probably overkill though.

(other thought: it should be clear to users that exif is needed to lint image+executable. I wouldn't expect users to find out on their own)

Yeah, I suppose we could mention in the help text. Although I'd probably prefer to have a few alternatives that could be used without the Exif extension (I don't expect users to want to install an extension just for a linter)

Can we use Filesystem::getMimeType()?

(Particularly, exif_imagetype can't detect PNG, GIF, etc., right?)

I suppose that we could, I thought that you were against using the MIME type (D9187#8) but, upon a second reading, I think that you were referring to non-binary files. FWIW, exif_imagetype seems to work on most image types AFAICT.

Yeah, I'm fine with / supportive of MIME on binaries.

joshuaspence edited edge metadata.
  • Use MIME type
epriestley edited edge metadata.

See inline!

src/lint/linter/ArcanistChmodLinter.php
47

debugging

This revision is now accepted and ready to land.Jun 25 2014, 7:26 PM
joshuaspence edited edge metadata.
  • Remove debugging code
joshuaspence retitled this revision from `ArcanistChmodLinter` should not allow images to be executable to `ArcanistChmodLinter` should not allow certain MIME types to be executable.Jun 25 2014, 7:30 PM
joshuaspence updated this object.