Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1765993002:
Remove UnescapeRule::URL_SPECIAL_CHARS from components/ (Closed)
|
Created:
4 years, 9 months ago by mmenke Modified:
4 years, 9 months ago Reviewers:
Peter Kasting, sky, boliu, Thiemo Nagel, engedy, Use pkasting(at)chromium.org, Roger Tawa OOO till Jul 10th, kinaba CC:
chromium-reviews, extensions-reviews_chromium.org, vabr+watchlistpasswordmanager_chromium.org, tfarina, noyau+watch_chromium.org, chromium-apps-reviews_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove UnescapeRule::URL_SPECIAL_CHARS from components/
We're removing this, in favor of PATH_SEPARATORS and
URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the
risk of security issues when unescaping, going forward.
All the remaining uses of URL_SPECIAL_CHARS in components/
look safe, so this CL replaces URL_SPECIAL_CHARS with both the
new values, maintaining old behavior, except in
affiliation_utils.cc, where the behavior clearly serves no
purpose, and adding both flags would be confusing.
BUG=589257
Committed: https://crrev.com/fb798389802bd344ccd6594ec0b734c933e5a1d1
Cr-Commit-Position: refs/heads/master@{#382881}
Patch Set 1 #Patch Set 2 : Fix, merge #
Total comments: 11
Patch Set 3 : Response to comments #
Messages
Total messages: 28 (13 generated)
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
mmenke@chromium.org changed reviewers: + atwilson@chromium.org, boliu@chromium.org, engedy@chromium.org, kinaba@chromium.org, sky@chromium.org
[+boliu]: auto_login_parser/ [+sky]: bookmarks/ [+kinaba]: drive/ [+engedy]: password_manager/ [+atwilson]: policy/ and signin/ [+pkasting]: url_formatter/ https://codereview.chromium.org/1765993002/diff/80001/components/auto_login_p... File components/auto_login_parser/auto_login_parser.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/auto_login_p... components/auto_login_parser/auto_login_parser.cc:60: net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS)); Looks safe - not used as a path. https://codereview.chromium.org/1765993002/diff/80001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:531: net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, Looks safe - not used as a path. Note that unescaping URL_SPECIAL_CHARS does change the meaning of URLs, per https://code.google.com/p/chromium/codesearch#chromium/src/net/base/escape.h&.... https://codereview.chromium.org/1765993002/diff/80001/components/drive/servic... File components/drive/service/fake_drive_service.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/drive/servic... components/drive/service/fake_drive_service.cc:511: net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS); Just used for testing, so didn't dig into this one. https://codereview.chromium.org/1765993002/diff/80001/components/password_man... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:104: // unescape the padding ('='). Comment indicates that PATH_SEPARATORS isn't needed, but I'm erring on the side of keeping old behavior. Doesn't seem like a potential security issue. https://codereview.chromium.org/1765993002/diff/80001/components/policy/core/... File components/policy/core/common/cloud/device_management_service_unittest.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/policy/core/... components/policy/core/common/cloud/device_management_service_unittest.cc:351: net::UnescapeRule::PATH_SEPARATORS | Just a test, so didn't dig into whether this is needed here. https://codereview.chromium.org/1765993002/diff/80001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:83: net::UnescapeRule::PATH_SEPARATORS | I don't think we're getting anything that we use as a path here. If we are, we should remove this. https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... File components/url_formatter/url_fixer.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... components/url_formatter/url_fixer.cc:624: net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS)); Since this is only done on paths from the command line, it doesn't seem like this is a problem (At least not in Chrome, suppose if some external source did path validation before passing them in to us on the command line, could be a problem).
mmenke@chromium.org changed reviewers: + pkasting@google.com
[+pkasting]: Please review url_formatter/
On 2016/03/16 21:30:01, mmenke wrote: > [+boliu]: auto_login_parser/ rs lgtm > [+sky]: bookmarks/ > [+kinaba]: drive/ > [+engedy]: password_manager/ > [+atwilson]: policy/ and signin/ > [+pkasting]: url_formatter/ > > https://codereview.chromium.org/1765993002/diff/80001/components/auto_login_p... > File components/auto_login_parser/auto_login_parser.cc (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/auto_login_p... > components/auto_login_parser/auto_login_parser.cc:60: > net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS)); > Looks safe - not used as a path. > > https://codereview.chromium.org/1765993002/diff/80001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils.cc:531: > net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, > Looks safe - not used as a path. Note that unescaping URL_SPECIAL_CHARS does > change the meaning of URLs, per > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/escape.h&.... > > https://codereview.chromium.org/1765993002/diff/80001/components/drive/servic... > File components/drive/service/fake_drive_service.cc (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/drive/servic... > components/drive/service/fake_drive_service.cc:511: > net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS); > Just used for testing, so didn't dig into this one. > > https://codereview.chromium.org/1765993002/diff/80001/components/password_man... > File components/password_manager/core/browser/affiliation_utils.cc (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/password_man... > components/password_manager/core/browser/affiliation_utils.cc:104: // unescape > the padding ('='). > Comment indicates that PATH_SEPARATORS isn't needed, but I'm erring on the side > of keeping old behavior. Doesn't seem like a potential security issue. > > https://codereview.chromium.org/1765993002/diff/80001/components/policy/core/... > File components/policy/core/common/cloud/device_management_service_unittest.cc > (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/policy/core/... > components/policy/core/common/cloud/device_management_service_unittest.cc:351: > net::UnescapeRule::PATH_SEPARATORS | > Just a test, so didn't dig into whether this is needed here. > > https://codereview.chromium.org/1765993002/diff/80001/components/signin/core/... > File components/signin/core/browser/signin_header_helper.cc (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/signin/core/... > components/signin/core/browser/signin_header_helper.cc:83: > net::UnescapeRule::PATH_SEPARATORS | > I don't think we're getting anything that we use as a path here. If we are, we > should remove this. > > https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... > File components/url_formatter/url_fixer.cc (right): > > https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... > components/url_formatter/url_fixer.cc:624: > net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS)); > Since this is only done on paths from the command line, it doesn't seem like > this is a problem (At least not in Chrome, suppose if some external source did > path validation before passing them in to us on the command line, could be a > problem).
Description was changed from ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in these directories look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior. BUG=589257 ========== to ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior. BUG=589257 ==========
LGTM
drive/ LGTM
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
LGTM https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... File components/url_formatter/url_fixer.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... components/url_formatter/url_fixer.cc:624: net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS)); On 2016/03/16 21:30:01, mmenke wrote: > Since this is only done on paths from the command line, it doesn't seem like > this is a problem (At least not in Chrome, suppose if some external source did > path validation before passing them in to us on the command line, could be a > problem). I don't think I understand this comment: what does "this is a problem" mean?
https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... File components/url_formatter/url_fixer.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/url_formatte... components/url_formatter/url_fixer.cc:624: net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS)); On 2016/03/17 06:03:02, Peter Kasting wrote: > On 2016/03/16 21:30:01, mmenke wrote: > > Since this is only done on paths from the command line, it doesn't seem like > > this is a problem (At least not in Chrome, suppose if some external source did > > path validation before passing them in to us on the command line, could be a > > problem). > > I don't think I understand this comment: what does "this is a problem" mean? A security problem. Applying this to file URLs generated by other file URLs could violate whatever security model we have, and then this code changes the path, we could be in trouble. I am, admittedly, not familiar with the cross-path security model for file URLs, but if some code thinks file:///foo/..%2fbaz is inside foo (Which is what it really means, according to GURL and net/), this code would turn it into file:///foo/../baz/, which GURL would then turn into file:///baz/.
LGTM on components/password_manager % comment. https://codereview.chromium.org/1765993002/diff/80001/components/password_man... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:104: // unescape the padding ('='). On 2016/03/16 21:30:01, mmenke wrote: > Comment indicates that PATH_SEPARATORS isn't needed, but I'm erring on the side > of keeping old behavior. Doesn't seem like a potential security issue. Yes, we can safely remove net::UnescapeRule::PATH_SEPARATORS. In fact, we should remove it, in order to avoid confusing future readers of this code. In case of benevolent input, there will be no escaped path separators in it. In case of malicious input, the ContainsOnlyAlphanumericAnd(...) check below will still fail if escape sequences remain in |base64_encoded_hash|. So in either case, there is no change. Thank you for being so thorough!
Description was changed from ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior. BUG=589257 ========== to ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior, except in affiliation_utils.cc, where the behavior clearly serves no purpose, and adding both flags would be confusing. BUG=589257 ==========
https://codereview.chromium.org/1765993002/diff/80001/components/password_man... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/1765993002/diff/80001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:104: // unescape the padding ('='). On 2016/03/17 09:51:25, engedy wrote: > On 2016/03/16 21:30:01, mmenke wrote: > > Comment indicates that PATH_SEPARATORS isn't needed, but I'm erring on the > side > > of keeping old behavior. Doesn't seem like a potential security issue. > > Yes, we can safely remove net::UnescapeRule::PATH_SEPARATORS. In fact, we > should remove it, in order to avoid confusing future readers of this code. > > In case of benevolent input, there will be no escaped path separators in it. In > case of malicious input, the ContainsOnlyAlphanumericAnd(...) check below will > still fail if escape sequences remain in |base64_encoded_hash|. So in either > case, there is no change. > > Thank you for being so thorough! Done, updated CL description.
mmenke@chromium.org changed reviewers: + rogerta@chromium.org, tnagel@chromium.org - atwilson@chromium.org
[+tnagel]: Please review components/policy. [+rogerta]: Please review components/signin.
lgtm components/signin
lgtm policy
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sky@chromium.org, boliu@chromium.org, engedy@chromium.org, kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/1765993002/#ps100001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765993002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765993002/100001
Message was sent while issue was closed.
Description was changed from ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior, except in affiliation_utils.cc, where the behavior clearly serves no purpose, and adding both flags would be confusing. BUG=589257 ========== to ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior, except in affiliation_utils.cc, where the behavior clearly serves no purpose, and adding both flags would be confusing. BUG=589257 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior, except in affiliation_utils.cc, where the behavior clearly serves no purpose, and adding both flags would be confusing. BUG=589257 ========== to ========== Remove UnescapeRule::URL_SPECIAL_CHARS from components/ We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to reduce the risk of security issues when unescaping, going forward. All the remaining uses of URL_SPECIAL_CHARS in components/ look safe, so this CL replaces URL_SPECIAL_CHARS with both the new values, maintaining old behavior, except in affiliation_utils.cc, where the behavior clearly serves no purpose, and adding both flags would be confusing. BUG=589257 Committed: https://crrev.com/fb798389802bd344ccd6594ec0b734c933e5a1d1 Cr-Commit-Position: refs/heads/master@{#382881} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fb798389802bd344ccd6594ec0b734c933e5a1d1 Cr-Commit-Position: refs/heads/master@{#382881} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
