Generated FIlename different from Content Disposition string

net/base/filename_util_internal.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/filename_util.h"
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
@@ skipping 211 matching lines @@
     ReplaceIllegalCharactersCallback replace_illegal_characters_callback) {
   // TODO: this function to be updated to match the httpbis recommendations.
   // Talk to abarth for the latest news.
 
   // We don't translate this fallback string, "download". If localization is
   // needed, the caller should provide localized fallback in |default_name|.
   static const base::FilePath::CharType kFinalFallbackName[] =
       FILE_PATH_LITERAL("download");
   std::string filename;  // In UTF-8
   bool overwrite_extension = false;
-
+  bool is_name_from_content_disposition = false;
   // Try to extract a filename from content-disposition first.
   if (!content_disposition.empty()) {
     HttpContentDisposition header(content_disposition, referrer_charset);
     filename = header.filename();
+    if (!filename.empty())
+      is_name_from_content_disposition = true;
   }
 
   // Then try to use the suggested name.
   if (filename.empty() && !suggested_name.empty())
     filename = suggested_name;
 
   // Now try extracting the filename from the URL.  GetFileNameFromURL() only
   // looks at the last component of the URL and doesn't return the hostname as a
   // failover.
   if (filename.empty())
@@ skipping 21 matching lines @@
   SanitizeGeneratedFileName(&result_str, replace_trailing);
   if (result_str.find_last_not_of(FILE_PATH_LITERAL("-_")) ==
       base::FilePath::StringType::npos) {
     result_str = !default_name_str.empty()
                      ? default_name_str
                      : base::FilePath::StringType(kFinalFallbackName);
     overwrite_extension = false;
   }
   replace_illegal_characters_callback.Run(&result_str, '-');
   base::FilePath result(result_str);
-  GenerateSafeFileName(mime_type, overwrite_extension, &result);
+  // extension is not appended to filename derived from content-disposition,
+  // if it doesnot have one
+  if (!is_name_from_content_disposition)
+    GenerateSafeFileName(mime_type, overwrite_extension, &result);

[asanka, 2014/10/08 15:58:29]

This step isn't optional. The fact that a filename was specified in a
Content-Disposition header doesn't mean it should be used as is, and it could
potentially be attacker controlled.

Perhaps the correct fix is to honor the |overwrite_extension| flag even if the
extension is empty or avoid using an extension for a common blacklist of mime
types?

 
   base::string16 result16;
   if (!FilePathToString16(result, &result16)) {
     result = base::FilePath(default_name_str);
     if (!FilePathToString16(result, &result16)) {
       result = base::FilePath(kFinalFallbackName);
       FilePathToString16(result, &result16);
     }
   }
   return result16;
@@ skipping 22 matching lines @@
   base::FilePath generated_name(
       base::SysWideToNativeMB(base::UTF16ToWide(file_name)));
 #endif
 
   DCHECK(!generated_name.empty());
 
   return generated_name;
 }
 
 }  // namespace net