Thanks for the report! It seems to work for me.

tedchoc: Can you handle this, or delegate it to someone on your team? Please feel free to CC anyone who needs to know or can help. Thanks!

meacer: I thought we had a thing of not letting web contents navigate to javascript: URLs? Am I mis-remembering?

For reference, the link in the POC is:

<a href="o.o:@javascript:://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337" target="win" onclick="redirect();">Click here to go to https://google.com</a>

(`redirect()` sets a timeout to call `window.open("https://google.com/load/", "win")`)

On desktop Chrome, this gets treated as:

https://lbherrera.github.io/lab/script/o.o:@javascript::/google.com/error/%0Aalert(document.domain)/.https:/google.com/load/webpage/133337

But on mobile it gets executed when the explicit omnibox navigation happens. It seems like the javascript isn't executed _until_ then, however, so the link just manages to get the javascript URL into the omnibox (which may be why it is bypassing the navigation restrictions?).

We intentionally do not defocus the omnibox when a navigation occurs on the underlying page as that can happen at arbitrary times and inputting on mobile is hard enough without it being cleared at random points.

Would it make more sense to simply clear the omnibox when that particular dialog shows up?

> meacer: I thought we had a thing of not letting web contents navigate to javascript: URLs? Am I mis-remembering?

No we don't block javascript: URLs, only data: and filesytem: are blocked at the moment. javascript: navigations should work as executing the script inside the page.

I think I'll need more input from security/UX on how to solve this.  Defocusing the omnibox on navigation isn't a good solution for mobile.

In this particular case, putting a javascript:// URL into the omnibox is never going to be something a user should see on mobile.

The main issue seems like a navigation bug to me. The desktop behavior in comment #2 sounds the correct one: the page should never be able to put a javascript: URL in the omnibox.

Also not clear why a URL that starts with "o.o:@javascript" is treated as a javascript URL. We should drop username/password pairs before checking for javascript: URLs.

Sorry, looks like I mostly rephrased comment #5. Is it possible that the "o.o:@javascript" is what's bypassing the javascript scheme check?

tedchoc: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

@meacer, who was the comment in #7 too?  As I mentioned from #5, defocusing the omnibox on navigation won't be a good option for mobile.

I don't have a lot of input on what is being handled here, so maybe someone that knows how that dialog works in the first place would be a good candidate to look at this for Android?

meacer: Uh oh! This issue still open and hasn't been updated in the last 28 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

> @meacer, who was the comment in #7 too?  As I mentioned from #5, defocusing the omnibox on navigation won't be a good option for mobile.

Just a general observation about where the bug might be. I wasn't suggesting defocusing the omnibox, but pointing that the code that determines whether a URL is a javascript: or not could be buggy. I did some investigation back then and found a potentially problematic spot, but can't recall where it was.

That said,

I'm trying to repro on Chrome 69 and can't get the PoC to work. When I click the link, google.com opens but there is no modal dialog. Only when I switch back to the attacker's page and tap the omnibox the modal dialog shows, and it keeps showing up every time I tap the omnibox. No keyboard opens and I can't type.

This is on a Nexus 5X on Android 8.1.

luan.herrera: Are you still able to repro with Chrome 69? I wonder if avi's modal dialog suppression is kicking in when I try it.

@meacer: I just tested on Chrome 70.0.3503.0 and was able to reproduce it. I am using Android 6.0.1 on a SM-J500M.

I will try to get my hands on other phones to test on them as well.

meacer@, friendly ping from the security sheriff. This is a high severity vulnerability affecting Stable.

@luan.herrera: Have you had a chance to test with a newer Android version?

In any case, I don't think I'm the right owner. As I mentioned above, it's possible that Android Chrome doesn't detect that the URL is a javascript URL and lets it remain in the omnibox. Then when the user refreshes the page, the JS is executed.

Ted, could you please take a look / help to find an owner?

What exactly is the proposed fix?  Are we not supposed to show javascript URLs in the omnibox if they weren't typed in by the user?

I was under the impression that we share the same logic as desktop in this regard, so I'm wondering if there is additional logic further down in desktop land is doing whatever is the desired behavior here.

I'm adding a couple desktop omnibox folk that might be able to add anything.  But if you can tell me exactly what the correct behavior is with javascript URLs, then we could likely make a change in Android to solve this.

meacer@: Unfortunately I haven't.

By the way, I was testing more and I think the core issue is in the logic that handles the opening of new links through anchor/form tags.

For example:

1. window.open("o.o:@javascript:://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337", "_blank");
2. <a href="o.o:@javascript:://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337" target="_blank">Click here</a>
3. <form action="o.o:@javascript:://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337" target="_blank"><input type="submit"></form>

When [1] is executed, a new tab opens with the omnibox displaying "about:blank" as well as an "Open xdg-open" dialog. I think this happens because Chrome thinks that "o.o" is an external protocol and it tries to open it.
In [2] and [3], instead of "about:blank" in the omnibox, the "javascript://google.com..." appears together with the "Open xdg-open" dialog.

Note that I tested this on Linux, so this behavior is not Android specific.
What I think Android does differently is that it probably doesn't support the opening of external protocols and because of that the "xdg-open" dialog doesn't show up.

So from a superficial view it seems that setting the omnibox to about:blank when Chrome detects that an external protocol is trying to be opened would fix the issue.
But I think the underlying issue that turns "o.o:@javascript:://" into "javascript://" should also be looked, because it might have implications elsewhere.

We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue?

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Hi! Friendly ping on this issue given there wasn't much movement since my last post.
Was anyone able to confirm the behavior in #18?

+creis for help routing of this

So the original <a> tag is this:
<a href="o.o:@javascript:://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337">

When the navigation entry is created, it has become this:
http://o.o@javascript//google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337

I don't know if the o.o: should have been rejected earlier or if that is a somehow being considered as valid.

At this point, spec() on the GURL fails because of:
DCHECK(false) << "Trying to get the spec of an invalid URL!";

After commenting out that DCHECK as Android fails all over the place, the omnibox problem comes down to ToolbarModelImpl::GetFormattedURL using url_formatter::FormatUrl (internally that uses url.possibly_invalid_spec() so it doesn't operate on the empty string).

FormatUrl converts:
http://o.o@javascript://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337

into:

javascript://google.com/error/%0Aalert(document.domain)//.https://google.com/load/webpage/133337

The gotcha on Android is that if the URL matches the text of the omnibox, we just reload the page on hitting enter.  In this case, getVisibleUrl() is returning the empty string because we use .spec() and thus it appears as if the user typed the javascript URL.

Before I try to fix this in the Android layers, I agree with the original author that this seems like something that should be fixed at a lower level and someone with more knowledge of the whole URL handling should probably take that on instead of me trying to stumble my way through it.

Removing myself from the owner for now (again).

creis - can you find an owner for this rapidly-aging issue?  Thanks heaps.

Hi again! Another friendly ping on this issue given m70 is coming "soon" and it would be cool if this bug could be fixed in time for it.

Sorry, I didn't realize this had been assigned to me.  I'll try to take a look this morning to see what it's about.

Just starting to look and I want to look into why we leave the javascript: URL in the address bar, but the first question I have is why the modal dialog is allowed (or doesn't switch you to the tab showing it) on Android.  avi@, do you know what's going on there?  See also  issue 606104 .

Becky does JavaScript dialogs on Android. I pass the question to her.

Re #27: For  issue 606104 , that should have been fixed after the auto dismissing dialog using Avi's dialog suppression (see Issue 799334).

And for this issue, I couldn't reproduce as is in the OP's video that shows an alert on google.com on 71.0.3561.0 (video attached).

Deleted: device-2018-09-26-104956.mp4 12.2 MB

	device-2018-09-26-104956.mp4 12.2 MB View Download

Comment 29: Thanks!  That solves part of the problem, where the attacker can show a dialog to convince the user to hit enter.  Looks like it launched in Chrome 68 around August 10 or so, so users have been protected from the dialog part since then.

However, there's still a problem with the javascript URL showing up in the address bar, which means it's still possible to go through the steps (i.e., select the omnibox before redirect, then hit enter after redirect) and run script on the new page.  We should not be showing the javascript URL in that case, and it looks like we don't do it for window.open or for javascript URLs without the o.o:@ prefix.  It seems to affect desktop as well-- I'll take a look after lunch as I'm catching up on the rest of the comments on this bug.

#29, The reason you couldn't reproduce is that the page was redirected to google.com before you had the chance to click in the omnibox. I guess now that the dialogs are being suppressed, 1500ms is not enough.

Could you try again using the PoC below (you can adjust the delay of the redirect using the delay parameter):
https://lbherrera.github.io/lab/script/delay.html?delay=3000

Also, this seems reproducible in Desktop (but not as convincing) by clicking in the link, focusing the omnibox, pressing the up or down arrow key, waiting for the redirect and then pressing enter.

Re #30, 31: Thanks for the new PoC. I can reproduce it with the delay (both 1000ms and 3000ms), so this is still a problem even with the launch mentioned in comment #29. I think the problem is still javascript URL showing up in the address bar.

Deleted: device-2018-09-26-125500.mp4 8.2 MB

	device-2018-09-26-125500.mp4 8.2 MB View Download

Update:

Comments 18 and 21 seem key to the navigation part of this.  Apparently, using a prefix like "o.o:@" (or "a.:@", or "anything.:@") triggers external protocol handling.  I can confirm the xdg-open dialog shows up on Linux but not on Android or Windows.

The navigation appears to fail, and we do get to NavigatorImpl::DiscardPendingEntryIfNeeded.  However, we don't end up clearing the pending entry for two reasons.  First, we've deleted the NavigationHandle already so we can't find a matching unique ID, so we return early to avoid clearing the wrong entry (which is a bug in this case).  Second, even if we did have a matching unique ID, we don't end up clearing it because we consider it an unmodified blank tab.  That seems like a bad result in this case, and something we may want to change.  (The catch is that we intentionally leave a failed link in the omnibox to let the user see what failed, but maybe we should revisit that, e.g., if there's no error page.  I think pkasting@ may be familiar with some parts of this heuristic, which I helped write a long time ago.)

arthursonzogni@: Do you know why we aren't getting here with the right unique ID?  I might be able to help with clearing the entry if we can get past that early return in DiscardPendingEntryIfNeeded.


Beyond that, clearing the entry would be less important if it weren't for the rewriting of the URL in the omnibox which strips the unexpected prefix and leaves the rest.  It's particularly bad for javascript: URLs, which go from being non-executable to being executable as a result.  I think there's a check somewhere that tries to strip out javascript: when you copy/paste a javascript: URL, but it must be kicking in at the wrong time for this case.

pkasting@ or mpearson@, do you know more about the ToolbarModelImpl::GetFormattedURL logic that tedchoc@ mentioned in comment 21, and what it should be doing in this case?

Note that the rewriting is bad even for non-javascript: URLs as well.  It will rewrite "a.:@https:://google.com" to "https://google.com", which would be pretty bad for a URL spoof.  Thankfully, the defense for  issue 9682  kicks in and resets the omnibox to about:blank if any other document tries to access the initial empty document to put spoof text below it.  Still, it seems like we shouldn't be stripping the arbitrary prefix?

On a meta note, I'm not sure about the High severity rating, given the mitigating factors that the user has to both focus the omnibox before the redirect and hit enter after the redirect.  That feels pretty like a pretty suspicious situation, with or without the dialog.  I still think it's important to fix this to avoid leaving a dangerous URL in the omnibox, but it feels more like Medium to me.  Anyway, we can discuss that more after we come up with a fix.

I think if we're trying to format an invalid URL, all bets are off, because the formatter relies on the URL canonicalization process to tell it what parts of the input mean what.  We'll best-effort invalid URLs, but to me the issue here is that something that doesn't canonicalize makes it this far at all.

Comment 34: Thanks-- I'll check to see if there's something that can catch and deal with invalid URLs earlier.

On Linux, I tried:
 1) <a href="o.o:@javascript:alert('test')" target="_blank">click</a>
 2) <a href="ooo:@javascript:alert('test')" target="_blank">click</a>

They are both handled as "external protocol", but in the omnibox they are rewritten as:
 1) javascript:alert('test')
 2) ooo:@javascript:alert('test')

Do we know why the "." in o.o causes it to be handled differently?

-----------------------------------------------------------------------------

Re comment 33:

> Apparently, using a prefix like "o.o:@" (or "a.:@", or "anything.:@") triggers external protocol handling.  I can confirm the xdg-open dialog shows up on Linux but not on Android or Windows.

Yes, this URL looks like one with an external protocol which needs to be handled by the system instead of the browser.
From a navigation point of view, it will be canceled and won't display an error page. The current document will still be there.

In the case of the new window, the current document is about:blank.

> The navigation appears to fail, and we do get to NavigatorImpl::DiscardPendingEntryIfNeeded.  However, we don't end up clearing the pending entry for two reasons.  First, we've deleted the NavigationHandle already so we can't find a matching unique ID, so we return early to avoid clearing the wrong entry (which is a bug in this case).  Second, even if we did have a matching unique ID, we don't end up clearing it because we consider it an unmodified blank tab.  That seems like a bad result in this case, and something we may want to change.  (The catch is that we intentionally leave a failed link in the omnibox to let the user see what failed, but maybe we should revisit that, e.g., if there's no error page.  I think pkasting@ may be familiar with some parts of this heuristic, which I helped write a long time ago.)
>
> arthursonzogni@: Do you know why we aren't getting here with the right unique ID?  I might be able to help with clearing the entry if we can get past that early return in DiscardPendingEntryIfNeeded.

On the contrary, I think we are getting here with the right unique ID, but because the controller->IsUnmodifiedBlankTab() is true, DiscardPendingEntryIfNeeded decides not to delete the entry.

Details:
========

  I tried this to navigate to this link:
  <a href="o.o:@javascript:alert('test')" target="_blank">click</a>

  I got:

  #1 0x559813ca9717 content::NavigatorImpl::DiscardPendingEntryIfNeeded()
  #2 0x559813ca3b20 content::NavigationRequest::OnRequestFailedInternal()
  #3 0x559813ca6916 content::NavigationRequest::OnRequestFailed()
  #4 0x559813d532bb content::NavigationURLLoaderImpl::OnComplete()

  * expected_pending_entry_id == 5
  * controller_->GetPendingEntry()->GetUniqueID() == 5
  * controller_->IsUnmodifiedBlankTab() == true
  * should_preserve_entry == true

  results: The entry is not deleted. (should_preserve_entry == true)

  #1 0x559813ca9717 content::NavigatorImpl::DiscardPendingEntryIfNeeded()
  #2 0x559813ca7fbc content::NavigatorImpl::DidFailProvisionalLoadWithError()
  #3 0x559813cb0631 content::RenderFrameHostImpl::OnDidFailProvisionalLoadWithError()

  * expected_pending_entry_id == 0
  * controller_->GetPendingEntry()->GetUniqueID() == 5
  * controller_->IsUnmodifiedBlankTab() == true

  results: Nothing happens (0 != 5)

Note: I answered your question, but I am not exactly sure to understand why it matters.

arthursonzogni@: Thanks for the details.  That's weird that we're getting the matching unique ID now-- that might have changed recently. I was seeing a mismatched ID (e.g., 9 vs 10) in a build from a week or two ago, but now I agree that I see matching IDs after updating my build.  That means a fix to that function might not work in M70 if we were to merge it, but we can cross that bridge if we come to it.

pkasting@: Do you happen to know where the code lives for stripping javascript: from a URL when we paste it in?  I'm wondering if that might be something we could use in GetFormattedURL in the case that the result starts with javascript:.  (I'm somewhat skeptical that we'll be able to either prevent navigations to invalid URLs or skip formatting for invalid URLs, given the high risk of regression if we haven't done it already.  But maybe it's possible?)

Anyway, I didn't get much time to work on this today, but hopefully I can revisit tomorrow.

@37: Code search for "StripJavascripSchemas".

I'm a bit uncomfortable with the idea of this living in GetFormattedURL().  I think this should be a function of caller code in response to specific actions, as it is today.  Perhaps it could be called from more places.  I dunno.

Comment 38: Yes, I'm not thrilled with the StripJavascriptSchemas approach either.  That may take away the worst outcome (i.e., leaving an "executable" URL in the address bar) if we can find where to put it, but it wouldn't help with other ways that a confusing URL is left behind after the formatting.

I'm sort of curious to understand more about why the multiple layers of rewriting put us in this state.  It's weird to have that middle step mentioned in comment 21, where http:// is prepended and the first colon is removed, which becomes the input to GetFormattedURL.  I wonder how much that matters to ending up with javascript: as the prefix.

Also, I'm curious about this comment in FormatUrlWithAdjustments, which seems to suggest that it was meant to handle invalid URLs as well (and thus maybe we should consider trying to fix its output in this case):
https://cs.chromium.org/chromium/src/components/url_formatter/url_formatter.cc?gsn=FormatUrl&l=485
  // We handle both valid and invalid URLs (this will give us the spec
  // regardless of validity).
  const std::string& spec = url.possibly_invalid_spec();
  const url::Parsed& parsed = url.parsed_for_possibly_invalid_spec();

Still, I can agree that it's weird to get this far with an invalid URL.

One quick option might be to just stop preserving the URL in DiscardPendingEntryIfNeeded if it's invalid.  That seems to apply to the javascript: case and avoids getting into the bad state.  I'm sending that for some try jobs in 
https://chromium-review.googlesource.com/c/chromium/src/+/1252942 just to see what impact it has, though it may not solve all the invalid URL messiness.

I spent some time thinking about this yesterday and looking into the various URL transformations.  TL;DR: I think the StripJavascriptSchemas approach is probably the safest way to handle the underlying concern here, if we can find the right place to put it.

Now that the modal dialog problem is gone, the main problem is that we're letting a page put a javascript: URL in the omnibox, which can run script if the user hits enter.  (It's not great that pages can also use this approach to put other arbitrary URLs like https://www.google.com or chrome://settings in the omnibox.  However, those would mostly be concerning for a URL spoof, and a spoof isn't possible here-- the URL resets to about:blank as soon as the initial document is accessed or modified.)  Thus, for now, let's focus on making sure a URL starting with javascript: doesn't end up in the address bar.

[URL Transformations]

There's several stages the URL goes through to make this attack possible:
* Initial URL: What the page specifies.
   - o.:javascript:foo()
   - a.a:@chrome:://settings
* NavEntry URL: What gets returned by NavigationEntry::GetURL().
   - http://o.:@javascript:foo()/
   - http://a.a@chrome//settings
* Virtual URL: What gets returned by NavigationEntry::GetVirtualURL().
   - http://o.:@javascript:foo()/
   - http://a.a@chrome://settings
* Displayed URL: What the omnibox shows after formatting.
   - javascript:foo()
   - chrome://settings

When we're creating the NavigationEntry in NavigationControllerImpl::CreateNavigationEntry, we pass the initial URL to BrowserURLHandlerImpl::GetInstance()->FixupURLBeforeRewrite.  That uses SegmentURLInternal and GetValidScheme to try to figure out the scheme.  It guesses "o.:" or "a.a:" but rejects it because it contains a dot, which is intentional to handle cases like "www.example.com:/".  Since it can't find a scheme, it gives up and prepends http://, giving us the virtual URL.

The NavEntry URL also goes through BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary, which strips the colons out of chrome:// and http://, but not javascript:.  Even if it applied to javascript: as well, a second colon is enough to bypass it.

I don't see an easy option for changing these rewriting rules to avoid ending up with http:// at the start.  That turns the "o.:@" prefix into a username/password to be removed later during GetFormattedURL.

I also don't see a great way to modify GetFormattedURL to make sure that we don't output something that starts with javascript: (which pkasting@ was also hesitant about in comment 38).

[Options for Fixes]

Let's consider all the places we might try to add a fix for this.

1) Clear the pending entry after failure if it's going to be "dangerous."
If we could tell when the output is going to start with javascript:, we could make DiscardPendingEntryIfNeeded clear the entry in that case, leaving about:blank in the address bar.
Pros:
 - Makes the dangerous URL go away entirely.
 - Fix would be co-located with other code that it trying to avoid unsafe URLs in the omnibox.
Cons:
 - Hard to determine if the NavEntry's URL is going to be formatted to start with javascript:.  Too many implementation details to predict it (e.g., checking if it's invalid() is not robust).  Seems like a layering violation to call url_formatter::FormatUrl (with all the right parameters) to test it on the spot.

2) Change the URL transformations for the NavEntry URL or virtual URL.
Maybe try to avoid prepending http:// in this case?
Pros:
 - It seems a bit surprising that http:// was prepended, and might be nice to fix.
 - Would help with at least known cases for how to end up with a javascript: URL (if not all).
Cons:
 - No obvious solutions here, as noted above.
 - Not clear whether it would cover all the ways the attacker could get javascript: to appear after formatting.
 - Might cause regressions in other cases (e.g., www.example.com:/)

3) Change GetFormattedURL to avoid having a javascript: prefix in the output.
Pros:
 - Closer to the end result, so more confidence that we'd catch all cases.
Cons:
 - Not clear how such a check would work.
 - Such a check doesn't fit in this code, as noted in comment 38.

4) Apply StripJavascriptSchemas after the URL is formatted for display.
Pros:
 - Most clearly enforces the property we care about (i.e., no javascript: URLs left in omnibox), no matter how the transformations work.
 - Isn't likely to cause regressions, since it doesn't interfere with URLs being edited (e.g., when user types a javascript: URL), and javacript: URLs are never otherwise left in the omnibox (since they don't commit, and already get stripped out of drag/drop and paste).
Cons:
 - Wouldn't prevent chrome:// or https:// cases (but maybe that's ok).
 - Wouldn't be near other security checks.


I'm leaning toward option 4, for more complete enforcement.  I put together a quick draft in https://chromium-review.googlesource.com/c/chromium/src/+/1255453 which passes try jobs, but I'm sure it's not the right spot.  (It's in OmniboxEditModel::ResetDisplayTexts(), but I'm wondering if it belongs in view code where StripJavascriptSchemas is defined.)

pkasting@, maybe you can help find the right place for it, if you agree?

I don't think we should worry about whether something starts with (or could eventually start with) "javascript:" anywhere in the pipeline.  To me the correct answer is that if canonicalization doesn't result in a valid URL, we should never create a pending entry or try to navigate.

Probably this would happen during or just after FixupURLBeforeRewrite().  We should simply check if the GURL is_valid(), and if not, abort entirely.

This ought to fix all possible problems here at the earliest stage.  Not letting this URL get through to the omnibox then happens as a side effect.

Stripping javascript: in the omnibox seems wrong to me since it should never be necessary, and it's tricky to accomplish correctly because mucking with the display text won't necessarily affect what the user edits/submits (what you want is more |url_for_editing_|, but we really shouldn't be applying destructive transformations to that).

Comment 41: Unfortunately, that doesn't really look like it's feasible.  There's no way to simply "not navigate" by the time we get to CreateNavigationEntry-- the code doesn't support returning a null entry by that point, given all the assumptions the callers are making.  It would be a pretty large refactor to change it (especially given multiple entry points).

Moving such a check earlier than CreateNavigationEntry would also increase the chance of regression, rejecting URLs that might have become valid after passing through FixupURLBeforeRewrite in CreateNavigationEntry.  I'd be pretty worried about regression in the first place, since I'm not sure how many invalid URLs we might already support navigating to.

At best we could rewrite it to about:blank (as in FilterURL), which would still attempt to navigate to it.  That's not great, but maybe it's the best option we have?  I'd still be pretty worried about regressions, but I can at least put up a try job to see what happens.

If we did go that approach, how confident are we that the formatting logic won't leave us with javascript: as a prefix when given a valid GURL?

I'm confident you won't get a javascript: URL unless we navigated to an invalid URL.

Rewriting to about:blank seems arguably OK but kinda suboptimal.

How big of a refactor is this "large refactor" to actually fail to navigate, or split the navigation into two pieces (basically, "fix up the URL and prepare for navigation, telling me whether it's OK", and "actually navigate")?  Usually in the past I haven't had too much trouble plumbing bail-outs through the code, but I haven't worked in this area.  It really seems like that's the right long-term answer here even if we bandaid.

Comment 43: I'm not sure about the refactor.  There's at least 4 call sites we'll need to investigate and I'm not sure how far back it goes from there, but it might be possible.

That said, I'm still concerned about regressions from auto-canceling any navigation to an invalid URL.  My try jobs on https://chromium-review.googlesource.com/c/chromium/src/+/1257863 failed, and I haven't had time to dig in to see if we can just work through them yet.  Still, this seems like the sort of change that's very likely to end up causing bugs (e.g., in unusual ChromeOS filesystem URLs, Android WebView, etc).

I'll see if I can find time to look into the test failures, but I'd feel better about a less risky fix.

I feel fears too, but they're the opposite of yours :).  I'm very frightened of trying to apply display-level bandaids to something where a nonsensical blob of data has been passed through many layers of functionality and APIs.  That's the sort of way we get more security holes in the future.

GURL is_valid() is very conservative and allows a ton of things that won't navigate in practice -- it just ensures they fit some basic standard of parseability.  The sorts of cases you mention shouldn't cause problems.  If they do, they're likely latent bugs in the system already that we need to fix anyway.  If we were trying to shove a fix into a branch, I'd look for a bandaid, but for the long run, this route seems like a much less risky fix to me.

Update: I'm working through test failures and various complications to see if we can prevent all navigations to invalid URLs (in https://chromium-review.googlesource.com/c/chromium/src/+/1257863).  Most of the tests seem like things we can update, with only a few tricky cases.  Refactoring the code is looking plausible, though it's unfortunate that we'll need to check for invalid URLs both on the way in and after FixupURLBeforeRewrite (which changes the URL to become invalid in this repro).  That makes the code a little uglier but may still be manageable.

That said, I don't think I'd be comfortable landing that change just before the M71 branch, since I think it will need more bake time to discover if it causes regressions.

If we're reasonably confident that invalid URLs are the only way for javascript: to show up as a prefix of the formatted URL, then I think I'll try a plan with two steps:
1) Discard the pending entry if it has an invalid URL (basically https://chromium-review.googlesource.com/c/chromium/src/+/1252942 with a test).  Land in M71.
2) Try the bigger "cancel all invalid URLs" in M72 and let it bake.

This will give us something small and non-disruptive which we think should cover the cases that javascript: shows up, and which is in the same vein as the more comprehensive (but possibly risky) approach.

--

Coming back to comment 33 about severity-- earlier I wrote:
"On a meta note, I'm not sure about the High severity rating, given the mitigating factors that the user has to both focus the omnibox before the redirect and hit enter after the redirect.  That feels pretty like a pretty suspicious situation, with or without the dialog.  I still think it's important to fix this to avoid leaving a dangerous URL in the omnibox, but it feels more like Medium to me.  Anyway, we can discuss that more after we come up with a fix."

Understanding more about the bug and the fixes we're considering, I still think this is the case-- this bug is about getting the user to self-XSS and requires the user to behave in a particular way with particular timing.  That's a strong enough mitigating factor that I would consider this medium severity, so I'll update the label and summary.  We're also too late for M69-- it's possible the smaller fix might get into M70, so I'll tentatively put the target there.  I'll also note that desktop platforms were at least partly affected by this bug (the javascript: URL thing, if not the modal dialog part).

CL up for review at https://chromium-review.googlesource.com/c/chromium/src/+/1252942.

Unfortunately, it doesn't look like merging it to M70 is an option, as I noted in comment 37.  Something changed in how the matching unique ID works in M71, such that the fix works in M71 but not M70.  I've verified this on a local M70 branch, and I'm not sure how to bisect to find what the relevant change was.  That said, I think it's probably ok to get the fix into M71.

Separately, I'll continue to push on the larger navigation CL in https://chromium-review.googlesource.com/c/chromium/src/+/1257863.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0aa576040704401ae28ea73b862d0b5d84262d51

commit 0aa576040704401ae28ea73b862d0b5d84262d51
Author: Charlie Reis <creis@chromium.org>
Date: Fri Oct 05 21:22:26 2018

Don't preserve NavigationEntry for failed navigations with invalid URLs.

The formatting logic may rewrite such URLs into an unsafe state.  This
is a first step before preventing navigations to invalid URLs entirely.

Bug:  850824 
Change-Id: I71743bfb4b610d55ce901ee8902125f934a2bb23
Reviewed-on: https://chromium-review.googlesource.com/c/1252942
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597304}
[modify] https://crrev.com/0aa576040704401ae28ea73b862d0b5d84262d51/chrome/browser/chrome_navigation_browsertest.cc
[add] https://crrev.com/0aa576040704401ae28ea73b862d0b5d84262d51/chrome/test/data/frame_tree/invalid_link_to_new_window.html
[modify] https://crrev.com/0aa576040704401ae28ea73b862d0b5d84262d51/content/browser/frame_host/navigator_impl.cc

The javascript: URL part of this should be fixed in r597304.  As noted, we won't be able to merge this to M70, since it doesn't seem to work there.

I'll try to get the larger navigation CL ready to land for after the branch cut.

Quick update on the followup navigation CL (https://chromium-review.googlesource.com/c/chromium/src/+/1257863): I think I'm largely unblocked on the test failures.  I want to let clamy@'s https://chromium-review.googlesource.com/c/chromium/src/+/1097407 land first (which refactors a bit of the relevant code) and rebase on top of that, in addition to some cleanup of my own CL.  Both of those should be able to happen after tomorrow's branch point.

The bug reported here is fixed in M71, but that work should help prevent issues like that in a more comprehensive way.

Thanks for all the hard work here, and for going beyond just "land a security fix" to address the underlying root pattern.  I think your plan for how to segment and land this makes sense and I'm really glad you're doing it all.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Cheers luan.herrera@! $2,000 for this report :-)

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

(Already in 71)

Just to follow up on comment 52, I'm continuing to push on https://chromium-review.googlesource.com/c/chromium/src/+/1257863.  There were a lot of additional tests to update after I made the invalid URL checks more consistent (after rebasing on clamy@'s r599176).  I've gotten everything green on desktop, but there's still a few Android (particularly WebView) tests failing, along with some new-ish tests on Fuchsia.

I'm still hoping to disallow navigations to all invalid GURLs (including empty ones) if I can get these to pass, but making an exception to allow empty GURLs might be an option if Android WebView relies on them too much.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot