Generated FIlename different from Content Disposition string

This patch prevents appending extension to filename derived from content disposition if it does not have one.

For example, in below case,
 Content-Type:text/plain;charset=UTF-8
 Content-Disposition:attachment; fileName = "ABC"

File is saved as "ABC" instead of "ABC.txt"

BUG=411200
Committed: https://crrev.com/7252e6eb8e6e88cf7ac6f5181498d2b3fc2c3874
Cr-Commit-Position: refs/heads/master@{#301794}

[arun87.kumar, 2014-10-08 11:39:46]

arun87.kumar@samsung.com changed reviewers:
+ abarth@chromium.org, asanka@chromium.org, rvargas@chromium.org,
ttuttle@chromium.org

[arun87.kumar, 2014-10-08 12:02:15]

asanka@

I have made a patch for Issue=411200. 

PTAL






Regards,
Arun

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

https://codereview.chromium.org/640713002/diff/1/net/base/filename_util_inter...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/640713002/diff/1/net/base/filename_util_inter...
net/base/filename_util_internal.cc:283: GenerateSafeFileName(mime_type,
overwrite_extension, &result);
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?

[arun87.kumar, 2014-10-09 05:12:01]

On 2014/10/08 15:58:29, asanka wrote:
>
https://codereview.chromium.org/640713002/diff/1/net/base/filename_util_inter...
> File net/base/filename_util_internal.cc (right):
> 
>
https://codereview.chromium.org/640713002/diff/1/net/base/filename_util_inter...
> net/base/filename_util_internal.cc:283: GenerateSafeFileName(mime_type,
> overwrite_extension, &result);
> 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?

asanka@

* With the present logic, we are appending new extension If
|overwrite_extension| is true or 
extension is empty and mimetype is not empty.

* In case of content disposition, |overwrite_extension| is always false. So then
it depends only on extension and mimetype

* My suggestion is, in case of content disposition, pass mimetype as empty
string to GenerateSafeFileName()
so that new extension construction is skipped.

* Rest all checks like IsShellIntegratedExtension() will still be executed.

Is this approach fine?



Regards,
Arun

[arun87.kumar, 2014-10-10 12:04:36]

asanka@

I have made changes according to above approach.

PTAL

[asanka, 2014-10-10 19:52:25]

https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
net/base/filename_util_internal.cc:283: GenerateSafeFileName("",
overwrite_extension, &result);
Let's not hack around our APIs and be explicit about whether we want the
extension to be overridden or not.

[arun87.kumar, 2014-10-11 14:45:20]

On 2014/10/10 19:52:25, asanka wrote:
>
https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
> File net/base/filename_util_internal.cc (right):
> 
>
https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
> net/base/filename_util_internal.cc:283: GenerateSafeFileName("",
> overwrite_extension, &result);
> Let's not hack around our APIs and be explicit about whether we want the
> extension to be overridden or not.

Ok. How about overloading GenerateSafeFileName() by defining another method
which takes only filepath as parameter? 

This api will be used when we explicitly donot want the extension to be
overriden.

[arun87.kumar, 2014-10-16 17:46:06]

asanka@ - <ping!>


> Ok. How about overloading GenerateSafeFileName() by defining another method
> which takes only filepath as parameter? 
> 
> This api will be used when we explicitly donot want the extension to be
> overriden.

[asanka, 2014-10-23 08:21:49]

https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
net/base/filename_util_internal.cc:283: GenerateSafeFileName("",
overwrite_extension, &result);
After looking at alternatives, it seems this isn't so bad. :-/

overwrite_extension should always be false in this case.

[arun87.kumar, 2014-10-23 09:37:24]

Updated to explicitly pass overwrite_extension as false.

PTAL




Regards,
Arun

https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/640713002/diff/20001/net/base/filename_util_i...
net/base/filename_util_internal.cc:283: GenerateSafeFileName("",
overwrite_extension, &result);
On 2014/10/23 08:21:49, asanka wrote:

Done.

[asanka, 2014-10-23 17:58:44]

Could you add tests to filename_util_unittest?

https://codereview.chromium.org/640713002/diff/80001/net/base/filename_util_i...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/640713002/diff/80001/net/base/filename_util_i...
net/base/filename_util_internal.cc:281: // content-disposition, if it doesnot
have one.
Nit: does not

[asanka, 2014-10-23 19:24:23]

On 2014/10/23 17:58:44, asanka wrote:
> Could you add tests to filename_util_unittest?
> 
>
https://codereview.chromium.org/640713002/diff/80001/net/base/filename_util_i...
> File net/base/filename_util_internal.cc (right):
> 
>
https://codereview.chromium.org/640713002/diff/80001/net/base/filename_util_i...
> net/base/filename_util_internal.cc:281: // content-disposition, if it doesnot
> have one.
> Nit: does not

lso update the change description to mention what the patch is doing. It
currently describes the issue it's trying to fix.

[arun87.kumar, 2014-10-28 11:43:55]

Have added unittests and updated description


PTAL

https://codereview.chromium.org/640713002/diff/80001/net/base/filename_util_i...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/640713002/diff/80001/net/base/filename_util_i...
net/base/filename_util_internal.cc:281: // content-disposition, if it doesnot
have one.
On 2014/10/23 17:58:44, asanka wrote:

Done.

[asanka, 2014-10-28 21:18:02]

lgtm

[arun87.kumar, 2014-10-29 04:57:37]

The CQ bit was checked by arun87.kumar@samsung.com

[commit-bot: I haz the power, 2014-10-29 04:58:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640713002/100001

[commit-bot: I haz the power, 2014-10-29 06:19:10]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-10-29 06:19:56]

Patchset 4 (id:??) landed as
https://crrev.com/7252e6eb8e6e88cf7ac6f5181498d2b3fc2c3874
Cr-Commit-Position: refs/heads/master@{#301794}