Page MenuHomePhabricator
Differential D9723

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

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
View All Files
Subscribers
epriestley
Korvin
richardvanvelzen

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T5466: `ArcanistChmodLinter` does not raise an error on images that are executable
Commits
rARC5ab288b30c36: `ArcanistChmodLinter` should not allow certain MIME types to be executable
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
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence updated this revision to Diff 23341.Jun 25 2014, 5:56 AM
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.
joshuaspence added a task: T5466: `ArcanistChmodLinter` does not raise an error on images that are executable.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 25 2014, 5:56 AM
Herald added subscribers: Korvin, epriestley. · View Herald Transcript
Harbormaster completed remote builds in B1380: Diff 23341.Jun 25 2014, 5:57 AM
joshuaspence added inline comments.Jun 25 2014, 6:12 AM
src/lint/linter/ArcanistChmodLinter.php
49

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

joshuaspence updated this revision to Diff 23342.Jun 25 2014, 6:30 AM
joshuaspence edited edge metadata.
  • Return as soon as possible.
Harbormaster completed remote builds in B1381: Diff 23342.Jun 25 2014, 6:31 AM
joshuaspence updated this revision to Diff 23347.Jun 25 2014, 4:58 PM
  • Wrap in function_exists
Harbormaster completed remote builds in B1385: Diff 23347.Jun 25 2014, 4:59 PM
richardvanvelzen added a subscriber: richardvanvelzen.Jun 25 2014, 5:54 PM

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)

joshuaspence added a comment.Jun 25 2014, 5:59 PM
In D9723#14, @richardvanvelzen wrote:

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)

epriestley edited edge metadata.Jun 25 2014, 6:47 PM

Can we use Filesystem::getMimeType()?

epriestley added a comment.Jun 25 2014, 6:48 PM

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

epriestley added a comment.Jun 25 2014, 6:48 PM

oh maybe it can

"reading"

joshuaspence added a comment.Jun 25 2014, 6:50 PM

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.

epriestley added a comment.Jun 25 2014, 6:56 PM

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

joshuaspence updated this revision to Diff 23352.Jun 25 2014, 7:21 PM
joshuaspence edited edge metadata.
  • Use MIME type
Harbormaster completed remote builds in B1389: Diff 23352.Jun 25 2014, 7:22 PM
epriestley accepted this revision.Jun 25 2014, 7:26 PM
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 updated this revision to Diff 23353.Jun 25 2014, 7:28 PM
joshuaspence edited edge metadata.
  • Remove debugging code
Harbormaster completed remote builds in B1390: Diff 23353.Jun 25 2014, 7:29 PM
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.
joshuaspence closed this revision.Jun 25 2014, 7:30 PM
joshuaspence updated this revision to Diff 23355.

Closed by commit rARC5ab288b30c36 (authored by @joshuaspence).

Revision Contents
Changeset List

PathSize
src/
lint/
linter/
53 lines

Diff 23355

View Options

src/lint/linter/ArcanistChmodLinter.php

Loading...
Phabricator · Built by Phacility