Have media gallery (through native media file util) use MIME sniffer

webkit/fileapi/media/native_media_file_util.cc

Index: webkit/fileapi/media/native_media_file_util.cc
diff --git a/webkit/fileapi/media/native_media_file_util.cc b/webkit/fileapi/media/native_media_file_util.cc
index 115278c5b7c617b72ef58d9751b86eb7ed17dfde..0cd77b17cb48db9865c0e2482d545b5ef1b78117 100644
--- a/webkit/fileapi/media/native_media_file_util.cc
+++ b/webkit/fileapi/media/native_media_file_util.cc
@@ -4,7 +4,12 @@
 
 #include "webkit/fileapi/media/native_media_file_util.h"
 
+#include "base/string_util.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/mime_sniffer.h"
+#include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_operation_context.h"
+#include "webkit/fileapi/file_system_task_runners.h"
 #include "webkit/fileapi/media/media_path_filter.h"
 #include "webkit/fileapi/media/filtering_file_enumerator.h"
 #include "webkit/fileapi/native_file_util.h"
@@ -13,6 +18,27 @@ using base::PlatformFile;
 using base::PlatformFileError;
 using base::PlatformFileInfo;
 
+namespace {
+
+// Modelled after ScopedFILEClose.
+class ScopedPlatformFileClose {
+ public:
+  void operator()(base::PlatformFile* ppf) const {
+    base::ClosePlatformFile(*ppf);
+  }
+};
+
+typedef scoped_ptr<base::PlatformFile,
+                   ScopedPlatformFileClose> ScopedPlatformFile;
+
+// Returns true if the current thread is IO thread.
+bool IsOnIOThread(fileapi::FileSystemOperationContext* context) {

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

nit: There's a thread called the IO thread, so this isn't a good name.  Maybe
OnTaskRunnerThread ?

[Kevin Bailey, 2013/03/20 13:53:23]

And that's what I get for plagiarizing. This was copied from
device_media_async_file_util. I meant to ask if you thought a common location
would be better, but I couldn't spot one. It needs about 5 different class
definitions, so there are lots of possibilities, from
file_system_operation_context to file_system_task_runners. Or I could just leave
it here.

[vandebo (ex-Chrome), 2013/03/20 20:03:48]

Hmm, I'm not completely familiar with the different task runners.  The one in
device_media_async_file_util.cc just does
context->task_runner()->RunsTasksOnCurrentThread().  Is there a reason to
specifically pull out the io_task_runner?

I think it's fine as a stand alone function, or even fine inline.

[Kevin Bailey, 2013/03/21 15:53:19]

It's identical to what's in device_media_async_file_util.cc

+  return context->file_system_context()->task_runners()->
+      io_task_runner()->RunsTasksOnCurrentThread();
+}
+
+}
+
 namespace fileapi {
 
 NativeMediaFileUtil::NativeMediaFileUtil() {
@@ -214,4 +240,56 @@ NativeMediaFileUtil::GetFilteredLocalFilePathForExistingFileOrDirectory(
   return base::PLATFORM_FILE_OK;
 }
 
+bool NativeMediaFileUtil::IsMediaFile(const base::FilePath& path) {
+  base::PlatformFile file_handle;
+  bool created;
+  if (NativeFileUtil::CreateOrOpen(path,
+                                   base::PLATFORM_FILE_OPEN |
+                                   base::PLATFORM_FILE_READ,
+                                   &file_handle, &created)
+      != base::PLATFORM_FILE_OK) {

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

To improve formatting, assign the result of CreateOrOpen to a variable and check
that.

[Kevin Bailey, 2013/03/20 13:53:23]

Done.

+    return false;
+  }
+  ScopedPlatformFile scoped_platform_file(
+      new base::PlatformFile(file_handle));

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

nit: will fit on the previous line.

[Kevin Bailey, 2013/03/20 13:53:23]

Ok, but one stray rename...

+  char buffer[net::kMaxBytesToSniff];
+  int64 len;
+  // Read as much as SniffMimeType() will bother looking at.
+  len = base::ReadPlatformFile(file_handle, 0, buffer, net::kMaxBytesToSniff);
+  // Let the SniffMimeType() function decide what to do

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

Actually, I think we can pretty safely say that a zero byte file is not a media
file.

[Kevin Bailey, 2013/03/20 13:53:23]

Fair enough, 1) I wanted the policy decisions in one place (there may very well
be valid zero-byte media files, however I will grant the user probably isn't
hurt by not opening them) and 2) I can't imagine how one could implement an
attack through a zero-byte file.

[vandebo (ex-Chrome), 2013/03/20 20:03:48]

I don't feel strongly, if you'd prefer to leave it as only < 0, that's fine as
well.  In practice I don't think it'll make a difference because a 0 byte media
file simply can't be validated with mime type sniffing, there's nothing to
validate.

[Kevin Bailey, 2013/03/21 15:53:19]

Ack.

+  // with a 0 length file.
+  if (len < 0) {
+    return false;
+  }
+  std::string result;

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

nit: result -> mime_type ?

[Kevin Bailey, 2013/03/20 13:53:23]

Done.

+  if (!net::SniffMimeType(buffer, net::kMaxBytesToSniff,

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

I think you should pass len here.

[Kevin Bailey, 2013/03/20 13:53:23]

Good catch.

+                          GURL(path.value()),

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

nit: This will fit on the previous line.

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

Should we prefix "file://" to the path first ?

[Kevin Bailey, 2013/03/20 13:53:23]

It doesn't seem to need it, but I'm all for sanitizing input.

[Kevin Bailey, 2013/03/20 13:53:23]

Done.

+                          "text/plain", &result)) {
+    return false;
+  }
+  return StartsWithASCII(result, "image/", true) ||
+         StartsWithASCII(result, "audio/", true) ||
+         StartsWithASCII(result, "video/", true);
+}
+
+base::PlatformFileError NativeMediaFileUtil::CreateSnapshotFile(
+    FileSystemOperationContext* context,
+    const FileSystemURL& url,
+    base::PlatformFileInfo* file_info,
+    base::FilePath* platform_path,
+    SnapshotFilePolicy* policy) {
+  DCHECK(policy);

[vandebo (ex-Chrome), 2013/03/20 00:18:27]

Can we change lines 281-288 to just IsolatedFileUtil::CreateSnapshotFile(...);
if (error != base::PLATFORM_FILE_OK)
  return error;
?

[Kevin Bailey, 2013/03/20 13:53:23]

Done. Moved the DCHECK too, since I imagine we wanted to test before the
snapshot, not so much before the media check.

+  DCHECK(file_info);
+  // We're just returning the local file information.
+  *policy = kSnapshotFileLocal;
+  base::PlatformFileError error =
+      GetFileInfo(context, url, file_info, platform_path);
+  if (error == base::PLATFORM_FILE_OK && file_info->is_directory)
+    return base::PLATFORM_FILE_ERROR_NOT_A_FILE;
+  DCHECK(IsOnIOThread(context));
+  if (!IsMediaFile(*platform_path))
+    return base::PLATFORM_FILE_ERROR_SECURITY;
+  return error;
+}
+
 }  // namespace fileapi