Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1059413004:
Add a validator for intent:// URLs. (Closed)
|
Created:
5 years, 8 months ago by Chris Palmer Modified:
5 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a validator for intent:// URLs.
BUG=462843, 482113
Committed: https://crrev.com/e8b8c72eb8eb462c52a707c556d53d350bbd408d
Cr-Commit-Position: refs/heads/master@{#327613}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Allow Extras; require that standard keys appear only once; use Matchers; more test cases. #
Total comments: 3
Patch Set 3 : Handle extras as blobs. Add some tests using extras. #Patch Set 4 : Defend against null dereferences. #Patch Set 5 : Call each assert directly, to help debug tests. #Patch Set 6 : Add logging to debug bot-only test failures. #Patch Set 7 : Document the intent of the positive tests, too. #Patch Set 8 : It seems URI does not like _ in hostnames? #Patch Set 9 : The _ test reliably passes locally and fails on the bot; reversing the assert reliably reverses the… #Patch Set 10 : Why are the presubmit checks different on the bots vs. local? import org.chromium.base.Log; #
Messages
Total messages: 37 (12 generated)
palmer@chromium.org changed reviewers: + jaekyun@chromium.org, palmer@chromium.org, tedchoc@chromium.org, yfriedman@chromium.org
PTAL. Part 1 of 2. Thanks!
https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java (right): https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java:238: || parts.length > 7 parts.length could be bigger than 7 because extra fields can be added.
https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java (right): https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java:229: if (!Pattern.matches("^[\\w\\.-]*$", parsed.getHost())) { I would build a Matcher and use that since it's re-used throughout the loop https://codereview.chromium.org/1059413004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/UrlUtilitiesTest.java (right): https://codereview.chromium.org/1059413004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/UrlUtilitiesTest.java:122: + "authenticator.AUTHENTICATE;end','*');" There's a lot of other validation that is currently untested. Are there plausible exploits for fields other than "action" that we can add tests for?
https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java (right): https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java:229: if (!Pattern.matches("^[\\w\\.-]*$", parsed.getHost())) { > I would build a Matcher and use that since it's re-used throughout the loop Not exactly; note the allowed "-" (which is legal in DNS names but not in e.g. Java package names, AFAIK). That said, I do re-use the same pattern several times in the loop, so I suppose I can pre-compile a Pattern for it. https://codereview.chromium.org/1059413004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java:238: || parts.length > 7 On 2015/04/24 02:05:52, Jaekyun Seok wrote: > parts.length could be bigger than 7 because extra fields can be added. Done. https://codereview.chromium.org/1059413004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/UrlUtilitiesTest.java (right): https://codereview.chromium.org/1059413004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/UrlUtilitiesTest.java:122: + "authenticator.AUTHENTICATE;end','*');" > There's a lot of other validation that is currently untested. > > Are there plausible exploits for fields other than "action" that we can add > tests for? Definitely. Adding more tests now. The above was written quickly before heading home last night, just to get something for you all to look at. :)
OK, see what you think now. Thanks!
https://codereview.chromium.org/1059413004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java (right): https://codereview.chromium.org/1059413004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java:261: if (2 != pair.length) return false; I think this breaks "extras". see below https://codereview.chromium.org/1059413004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java:286: // beware: Here Lurks Madness. I guess this is a deliberate "fail closed" but I worry that the madness is present and we could break legitimate functionality. I honestly don't know so doing some digging. Looks like extras can be specified as a: - single char - . - key - = - value both key/value are encoded as URIs. I'm not saying we need to fully parse extras but it seems that uri-escaped charactres + "[.=]" gives the full set of allowed values. https://codereview.chromium.org/1059413004/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/UrlUtilitiesTest.java (right): https://codereview.chromium.org/1059413004/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/UrlUtilitiesTest.java:116: "intent://wump_hey.example.com/#Intent;package=com.example.wump;" Please add a test with extras
OK, I think the new patchset handles extras about as well as we can. PTAL.
palmer@chromium.org changed reviewers: + jcivelli@chromium.org
Adding jcivelli FYI.
I think this now lgtm I guess one thing that it leaves a little open is when/where to apply this. I saw the downstream change but why is this not done for example in say IntentHandler. There are also other places where chrome intents out to another app. Can you elaborate on this here or in the bug thread?
I filed https://code.google.com/p/chromium/issues/detail?id=482113 to track adding calls to validateIntentUrl now.
I filed https://code.google.com/p/chromium/issues/detail?id=482113 to track adding calls to validateIntentUrl now.
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1059413004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1059413004/#ps60001 (title: "Defend against null dereferences.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1059413004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
> Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) Does anyone have a clue as to why this is failing (line 118)? It does not fail for me on 2 devices running 2 different versions of Android. All tests pass for me locally. assertTrue(UrlUtilities.validateIntentUrl( 119 "intent://wump_hey.example.com/#Intent;package=com.example.wump;" 120 + "scheme=eeek;action=frighten_children;end;"));
On 2015/04/29 17:52:21, palmer wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > Does anyone have a clue as to why this is failing (line 118)? It does not fail > for me on 2 devices running 2 different versions of Android. All tests pass for > me locally. > > assertTrue(UrlUtilities.validateIntentUrl( > 119 > "intent://wump_hey.example.com/#Intent;package=com.example.wump;" > 120 + "scheme=eeek;action=frighten_children;end;")); Can you add logs to each early return to see which assert is failing? The only other weirdness I've seen sometimes only on bots is to run build/android/provision_devices.py which makes some changes to your device config.
Is it the underscore in wump_hey (all others are wump-hey)? That doesn't look like it should match "^[\\w\\.-]*$" Granted, why that wouldn't fail locally is probably more frightening. Might be worth a brief comment above each test assert to clarify what exactly it is verifying (I can't tell if that test is to verify that _ works or whether it is a side effect). - Ted On Wed, Apr 29, 2015 at 10:52 AM, <palmer@chromium.org> wrote: > Try jobs failed on following builders: >> linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, >> > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r... > ) > > Does anyone have a clue as to why this is failing (line 118)? It does not > fail > for me on 2 devices running 2 different versions of Android. All tests > pass for > me locally. > > assertTrue(UrlUtilities.validateIntentUrl( > 119 > "intent://wump_hey.example.com/#Intent;package=com.example.wump;" > 120 + "scheme=eeek;action=frighten_children;end;")); > > https://codereview.chromium.org/1059413004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Can you add logs to each early return to see which assert is failing? OK, will try that. > The only other weirdness I've seen sometimes only on bots is to run > build/android/provision_devices.py which makes some changes to your device > config. Trying that...
On 2015/04/29 18:55:09, Ted C wrote: > Is it the underscore in wump_hey (all others are wump-hey)? That doesn't > look like it should match "^[\\w\\.-]*$" \w includes _. https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html: \w A word character: [a-zA-Z_0-9] > Might be worth a brief comment above each test assert to clarify what > exactly it is verifying (I can't tell if that test is to verify that _ > works or whether it is a side effect). OK.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1059413004/#ps160001 (title: "The _ test reliably passes locally and fails on the bot; reversing the assert reliably reverses the…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1059413004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1059413004/#ps180001 (title: "Why are the presubmit checks different on the bots vs. local? import org.chromium.base.Log;")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1059413004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e8b8c72eb8eb462c52a707c556d53d350bbd408d Cr-Commit-Position: refs/heads/master@{#327613} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
