Not saving whitelist icon paths that reference the parent.

Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.

Committed: https://crrev.com/6b03d171d4137691637129ebad5cab20f4babbbb
Cr-Commit-Position: refs/heads/master@{#381557}

[atanasova, 2016-03-09 14:39:28]

atanasova@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-03-09 14:50:54]

https://codereview.chromium.org/1770313004/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/supervised_user_whitelist_installer.cc
(right):

https://codereview.chromium.org/1770313004/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/supervised_user_whitelist_installer.cc:83:
return base::FilePath();
Probably the same should apply below for the raw whitelist path? Maybe worth
making a helper like "GetSafeFilePath(dict, key)"?

[atanasova, 2016-03-10 10:35:28]

https://codereview.chromium.org/1770313004/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/supervised_user_whitelist_installer.cc
(right):

https://codereview.chromium.org/1770313004/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/supervised_user_whitelist_installer.cc:83:
return base::FilePath();
On 2016/03/09 14:50:53, Marc Treib wrote:
> Probably the same should apply below for the raw whitelist path? Maybe worth
> making a helper like "GetSafeFilePath(dict, key)"?

Done.

[Marc Treib, 2016-03-10 10:45:45]

LGTM!

[atanasova, 2016-03-10 10:46:34]

Description was changed from

==========
Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.
==========

to

==========
Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.
==========

[atanasova, 2016-03-10 10:46:34]

atanasova@chromium.org changed reviewers:
+ sorin@chromium.org

[atanasova, 2016-03-10 10:47:04]

Adding Soring for OWNERS approval

[atanasova, 2016-03-14 09:20:14]

atanasova@chromium.org changed reviewers:
+ bauerb@chromium.org

[atanasova, 2016-03-15 08:44:01]

Friendly ping

[Bernhard Bauer, 2016-03-15 09:34:52]

https://codereview.chromium.org/1770313004/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/supervised_user_whitelist_installer.cc
(right):

https://codereview.chromium.org/1770313004/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/supervised_user_whitelist_installer.cc:83:
return base::FilePath();
On 2016/03/09 14:50:53, Marc Treib wrote:
> Probably the same should apply below for the raw whitelist path? Maybe worth
> making a helper like "GetSafeFilePath(dict, key)"?

(The raw whitelists are read with ReadFileToString, which already refuses to
load a file that references a parent directory, but it's good to fail early
here.)

https://codereview.chromium.org/1770313004/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/supervised_user_whitelist_installer.cc
(right):

https://codereview.chromium.org/1770313004/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/supervised_user_whitelist_installer.cc:72:
base::FilePath path;
Move this to where it's used.

[atanasova, 2016-03-15 11:22:06]

https://codereview.chromium.org/1770313004/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/supervised_user_whitelist_installer.cc
(right):

https://codereview.chromium.org/1770313004/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/supervised_user_whitelist_installer.cc:72:
base::FilePath path;
On 2016/03/15 09:34:52, bauerb catching up on reviews wrote:
> Move this to where it's used.

Done.

[Sorin Jianu, 2016-03-16 20:58:15]

lgtm

Thank you!

I apologize for the late review.

Please always IM me when I am not responding to reviews right away.

[atanasova, 2016-03-16 20:59:28]

The CQ bit was checked by atanasova@chromium.org

[atanasova, 2016-03-16 20:59:29]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1770313004/#ps40001
(title: "Addressing Bernhard's comment")

[commit-bot: I haz the power, 2016-03-16 21:00:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1770313004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1770313004/40001

[commit-bot: I haz the power, 2016-03-16 21:47:22]

Description was changed from

==========
Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.
==========

to

==========
Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.
==========

[commit-bot: I haz the power, 2016-03-16 21:47:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-16 21:49:10]

Description was changed from

==========
Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.
==========

to

==========
Not saving whitelist icon paths that reference the parent.

If the icon path that we get from the manifest contains a reference to
the parent directory ("..") we would not save it and associate with the
whitelist. This is done in order to avoid malicious access to parent
folders.

Committed: https://crrev.com/6b03d171d4137691637129ebad5cab20f4babbbb
Cr-Commit-Position: refs/heads/master@{#381557}
==========

[commit-bot: I haz the power, 2016-03-16 21:49:11]

Patchset 3 (id:??) landed as
https://crrev.com/6b03d171d4137691637129ebad5cab20f4babbbb
Cr-Commit-Position: refs/heads/master@{#381557}