The bug could be in one of two places:
1. AGSA isn't sending the title string.
2. CCT is suppressing both the title string and normal origin display.

Yusuf, Malte, can you debug? I'm not sure how to tell which is the case.

I'm marking a missing URL as high-severity for now while we figure out what the scope is.

This repros in both Chrome 56.0.2924.87 and Chrome 57.0.2987.132.

+ryjust

There's a GWS change rolling out right now which he thinks should address this issue.

I made a change to GWS that includes a space at the end of the title and after a short delay removes the space from the title ('From nytimes.com ' -> 'From nytimes.com'). We've seen that updating the title makes the text appear again so we're hoping this works.

Since the no-title bug is nondeterministic, I cannot be entirely sure that it works but I can say that I have not noticed it since. The GWS with the fix is currently in canary.

Re #5: I see it reliably every time I open an AMP page from AGSA, it doesn't seem nondeterministic to me.

This feels very brittle -- it seems we don't know where the actual bug is? If GWS is sending the title, then suspect it's in CCT?

For context, AMP is setting the title but I think something about the timing of when we're setting the title and when CCT is swapping the prerendered page into the tab is causing the title to not actually get shown.

We set the title as early as we possibly can (in the fragment change event) and now we will be setting it again after the page has been displayed, just to make sure that it becomes visible.

We need a pretty strong guarantee that the URL is going to be shown. Are there end to end tests in GWS to make sure the title is always displayed? I'm guessing not -- can you add them?

If we can't guarantee that this bug is fixed, we should add a DumpWithoutCrashing call in CCT if the title is blank to debug when this is happening.

We have GWS tests ensuring that the title is getting set. We may be able to update our AGSA/CCT integration test to check that it's displaying, though I'd have to discuss with my AGSA contacts of how we'd manage that. I'm not very familiar with the capabilities of those tests.

I'm surprised that you see it so frequently. I've personally only seen it a handful of times and I am working on it every day. I do think that it has become more common in more recent versions of Chrome.

Re #10: Can you please clarify, how long has this been a known bug for from the GWS side?

Did this reach
a: a stable AGSA build
b: a stable Chrome build
?

If both yes, we should probably do a postmortem and ensure proper testing / sensible fallbacks. CCT should likely never show nothing.

The initial report was in November https://b.corp.google.com/issues/32693577

Until the last month or so, I only encountered it twice that I can recall. Once I noticed it more often (though admittedly still not often), I came up with the hacky fix that I put into GWS.

To #12: the fallback (i.e. non-special-case) here would have been showing only "google.com." I'm not sure that's better.

(A post-mortem sounds appropriate though!)

Re #12: Both yes, and +1 to postmortem.

When faced with a similar problem in iOS once upon a time, if there were uncertainty about the URL, Chrome falls back to not displaying the page. (Instead of displaying "google.com".) Can we bake something like that into CCT?

I'm also not convinced that this problem is resolved from the GWS side.

It looks like the GWS fix rolled out March 30, so it should be available now. Yet I am still consistently seeing it. I can repro on many domains as long as I pick queries that are not news articles.

The GWS with the fix was promoted to live today. I'm not sure how far it has rolled out. Again, I'm not sure if it works, but it is the only thing that we can do from GWS that we've come up with.

go/searchstackversions suggests that 25% of GWS have the fix.

The AI for the postmortem falls on me here. This is most likely a race about the title showing on the custom tabs side. Ryan's fix might have resolved it, but like mentioned before, we should never have no title for AMP in CCT with the current admittedly brittle AGSA special casing.

Like Ryan said, the first report about this was in November, but we couldn't reproduce it locally to know that it was a recurring issue. And there were no follow up reports. It recently came back to our attention again and the frequency I have been hearing about this in the last two weeks and #16 suggests this occurs much more now. I am guessing whatever race there was before got exposed due to a toolbar signaling related change.

I will look into this today and try to figure out what steps to take on the CCT side.

Debugging this with more logs, here is what I found:

With felt@'s repro steps on #1, this is 100% reproducible. Looking at what I am seeing from toolbar, we never get a title update. I tried to force a title update thinking we are missing it while the page is prerendered and running into a race, but that came back with no title at all being set for the page.

So at least in the above case, the AMP page seems to not have a title set. This is not true across all AMP pages. AMP carousel doesn't seem to have this issue. AMP blue links in top results don't seem to have this.

The only way to consistently get this has been with blue links below the fold. Not sure what is different in terms of title behavior for these, but so far there is not much I can do from the CCT side.

I will keep investigating..

Looking at the code, one thing I can still do is to force the title update still, just in case while prerendering there was a title update and we display nothing due to toolbar not being created when we get that update.

I am now able to %100 reproduce and fix this issue locally. Despite what I said in #22, we do have the title set from the page(I had not found the right signal to check it at before). But it gets set while the page is prerendering and the toolbar doesn't have any logic to make sure there is something there.

Right now the fix involves:
1) At the time where we would normally set the page domain, force update the title if it is non-empty.
2) If it is empty, post a task to check this later.

My first reaction for 2) was to set the mode back to showing the domain, but from #15, I am guessing we might not want that? 

felt@, what do you think is a good duration to check for posting the task here if the title came to us as empty in this mode(or should we not wait at all)? And if there is still no title after the possible delay, is falling back to displaying origin not acceptable?

Re #24: sorry, I've gotten lost, do we know what the root cause is? Is the title getting set during prerendering and then lost when we swap in the prerendered page? Why would checking later for a title help?

(Are there code pointers you could post that would help me understand?)

Thanks for digging into this!

The toolbar registers observers that get url and title updates after it is created and updates them when it receives these signals. In this case, we seem to never get the title update signals even though the title does get set while prerendering before the toolbar is even created.

We do force updates on creation for the domain itself, and in all the repro cases, when we get the initial domain update, we seem to have a valid title, so in my current fix I am forcing a title update as well when we are on this special mode. See my WIP fix here: https://codereview.chromium.org/2806593003/

But even though this fix the current issue, this code remains brittle still because we still rely on the page setting a title. So we need some sort of fallback if after a certain time, we still have no titles.

Showing google.com seems like a bad default. If possible, I'd prefer if we can cease showing the page. 

Charlie, what's the best way to do that? 

An interstitial would be too large to merge; if we do a full navigation like to about://blank, someone could just navigate back and end up on the page anyway right?

This is still AMP dependent logic, but if they move away from this page and come back, we would come back with a double bar and actual page title. So the Viewer will be getting out of the special AGSA-CCT mode.

Or I can move away from the page to about:blank with setShouldReplaceCurrentEntry set to true, so that we dont have this page in the navigation history.

Re #27-28: Showing about:blank (possibly with replacement) is an option, though it seems like a confusing outcome to the user.  An error page might be another way to do it.

Personally, I'm torn on whether showing google.com is a bad idea.  I get that this is untrusted content being hosted on a subframe of a google.com page, but doesn't the user see google.com in the address bar anyway if they choose "Open in Chrome" from the ... menu of the CCT?  It seems to me that we might as well do the same thing here for now, given that the untrusted content itself presumably can't force the title to be blank to exploit this.  It's just a fallback if AGSA gets it wrong.

Re comment 30: my concern is that I don't think we understand how this can happen (if it can at all after Yusuf's fix mentioned in comment 26), so I don't think we can state definitively that an attacker can't force the title to be blank.

If they choose "Open in Chrome", then I think the user is at least supposed to have the double URL bar indicating the publisher's origin, though admittedly I only see the double URL bar appearing sometimes right now. Actually "Open in Chrome" is behaving really weirdly for me right now, it opens https://www.google.com/amp/... with no security information attached (no green lock). If I refresh I see the double URL bar with a normal https://www.google.com/amp/... URL in the omnibox and the publisher origin below it.

before refresh.png is what I see when I do Open in Chrome. after refresh.png is what I see after refreshing, what I think is what I'm supposed to see for Open in Chrome. This is probably a separate bug.

Deleted: before refresh.png 806 KB

	before refresh.png 806 KB View Download

Deleted: after refresh.png 811 KB

	after refresh.png 811 KB View Download

I can confirm comment 32.  Please file do another bug for that.  Thanks for catching it!

Sending to about:blank or an error page still seems unfortunate to me, but it's an option and I don't have a better idea.

(FWIW, we do also have a mechanism for blanking the visual part of the page that we use to prevent some types of URL spoofs, but it's not ideal for this case.  It's helpful mainly when a renderer becomes unresponsive immediately after sending a commit message and never paints the new page.  It's not a great fit here, where the renderer will continue running and the user might interact with it.)

so I guess the two cherrypickable options are

1) Set to show domain which will be google.com
2) navigate to about:blank with setShouldReplaceCurrentEntry to true.

Are we leaning towards 2) then? felt@?

What is the exact URL that would be shown in these cases?

google.com/amp/…
or
google.com/amp/embedded/…
?

I sympathize with showing an error, but we cannot do it for XX% of page views from AGSA.

re #36, it will say "google.com"

Another option is to just cherrypick the force update fix that gets rid of the current issue for m58

And then we can come up with a better resolution on trunk for the case where Viewer doesn't send a title (interstitial etc).

+1 to #37. I also think Yusuf's first change should only fix the bug live in prod.

The in-depth defense solution doesn't need to cherry picked and should probably go in a separate change. Our google.com viewer is the only whitelisted user of this API, if for any reason we failed to set the title (this has never happened so far), we can also quickly fix it on our end via a GWS data push even. 

From Chrome's perspective, not showing anything there is a high severity security bug, so we need a fallback in M58.  (It's great if we never happen to hit it if AGSA manages to always send a title, but Chrome needs to handle the case when that doesn't happen.)

Sounds reasonable. I would suggest to submit two incremental changes though if possible.
1) Force update the title after the swap to fix the bug live in production.
2) Fallback for cases when the title is not set for any reason.

The former is a simpler change with no downsides that fixes all occurrences of the bug in production. The latter needs the fallback decision and needs to be extra cautious with avoiding false positives (you never know with timeouts). It would be unfortunate if the live bug fix 1) had to wait on 2).

I am going to be landing a patch that does 1) in 40# today and cherrypicking that to m58 today.

If we can come to a conclusion for the fallback in a timely manner, we will have the option to cherrypick that as well.

After creis@'s comments I am a bit worried about navigating to about:blank being too confusing. Here is another option: Although I have been saying making the domain visible will say "google.com", we can also set the title to the whole url in the absence of any title set by the page. That will fallback to a much more descriptive UI. 

I still think we should be conservative (show an error page or about:blank) until we understand if this is actually happening and how. The remainder of the URL after google.com is in the untrusted publisher's control; it wouldn't be hard for an attacker to construct the URL such that showing the whole URL is just as convincing a spoof as showing www.google.com.

Let me also add some context of the usage of this CCT API, in case this helps with evaluating any solution trade off for the fallback case handling.

- The CCT API to hide the page origin and only display the page title is only available to the Android GSA.
- It's only used to open AMP pages from the Search Results Page, soon Now as well. These results need to be indexed and ranked. AGSA renders these AMP pages inside the google/amp/embedded/... AMP Viewer container.
- the google.com/amp/embedded/... viewer is only enabled for Android Chrome and otherwise it's a 404. In addition, within this viewer, we only hide the AMP Viewer full grey header iff we see the AGSA referer. 

The last fallback case Yusuf is now handling would require the following conditions:
- The AMP result ranks in AGSA Search/Now (the greeen display url is included showing the origin domain)
- The user clicks on it in AGSA (which opens the AMP Viewer with the clicked result in CCT with this API)
- The AMP viewer somehow fails to set the document.title accordingly because of some new bug we introduce.

Re #42, an attacker would have to construct a fishing page, rank it in Search, and take advantage of a new bug introduced in the viewer before we notice it and fix it (note that the viewer is in GWS, which has hourly data pushes for JS fixes if needed). The fallback is certainly important in general, not sure if the scenario is likely.

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

commit e03e0c39a2a26eae5b0affe79522ce0c2e1b0a21
Author: yusufo <yusufo@chromium.org>
Date: Mon Apr 10 19:20:06 2017

Force update CCT Title when url is set if it only shows title

In the mode where CCT only shows the page title, we should never have an
empty title displayed. Force this by checking the title even if there
are no title update after Toolbar creation since those updates might
have been missing while prerendering. If there is no title set yet, post
a callback to check this and fallback to showing the domain if there is
not title set after a second.

BUG= 708957 

Review-Url: https://codereview.chromium.org/2806593003
Cr-Commit-Position: refs/heads/master@{#463355}

[modify] https://crrev.com/e03e0c39a2a26eae5b0affe79522ce0c2e1b0a21/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Re comment 43: thanks, IMO the unlikelihood of this scenario is exactly why we should play it safe by not showing the page content. It's not something that should happen in the course of normal operations; it's a fail-safe in case something goes really really wrong.

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

commit e51123bfa9fd260cb58612d28f74b876b8c679a0
Author: Yusuf Ozuysal <yusufo@google.com>
Date: Mon Apr 10 22:03:11 2017

Force update CCT Title when url is set if it only shows title

In the mode where CCT only shows the page title, we should never have an
empty title displayed. Force this by checking the title even if there
are no title update after Toolbar creation since those updates might
have been missing while prerendering. If there is no title set yet, post
a callback to check this and fallback to showing the domain if there is
not title set after a second.

BUG= 708957 

Review-Url: https://codereview.chromium.org/2806593003
Cr-Commit-Position: refs/heads/master@{#463355}
Committed: https://chromium.googlesource.com/chromium/src/+/e03e0c39a2a26eae5b0affe79522ce0c2e1b0a21

patch from issue 2806593003 at patchset 40001 (http://crrev.com/2806593003#ps40001)

Cr-Commit-Position: refs/branch-heads/3029@{#656}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/e51123bfa9fd260cb58612d28f74b876b8c679a0/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

Removing the merge related labels and setting status back to started as there is still follow ups to do on this.

The force update fix landed and got cherrypicked to m58.

We still need a final decision on the fallback, land that and cherrypick if we have time.

I agree with estark. If it's feasible to merge, I'd like the about:blank fallback plus UMA metrics to tell us if we ever end up using the fallback. Someone should sign up for chirp alerts for the histogram and immediately look into the problem if it starts happening again.

Charlie posed a good question over chat: why do we want a fallback here, when we already show "google.com" in other cases for AMP content? The reason is because normally AMP shows a bar at the top with the content's original domain, but in this case they don't show it because it's expected that it'll be shown in the CCT toolbar.

I am done with working on a fallback and adding tests for various cases of TITLE_ONLY setups including cases that necessitate the fallback (page sets an empty title).

Then I realized that we actually can't ever have an empty title, as long as we update that textbox at least once with the title.(Which we do now after the initial fix).

We propagate the title all the way from getDisplayedTitle in navigation_entry_impl.cc and there is always a fallback to the original url there.

So if AGSA fails to set title here, we would actually show the url itself (no scheme), without adding any fallback logic.

I do have the patch ready with tests (https://codereview.chromium.org/2816333002), but under this new condition, I wanted to check one more time, whether security sees this as a requirement. Again without having any logic, we already fallback to the full url.

I still think we should not show the publisher content with the original google.com URL in the omnibox for the reason mentioned in Comment 51: without the AMP URL bar, there's no indication of where the content came from and it makes for a very easy phishing or spoofing attack on google.com.

Alright, thanks for the response, estark@

I will be landing the patch as is then.

Emily, could we display "about:blank" without showing an error page and just do the logging for now? 
Sorry to slow the fallback fix down.

I am just concerned that if we happen to have a bug in Chrome and were to trigger the fallback at the wrong time, we could end up showing about:blank for ~10% of AGSA clicks. In addition, the only way to fix that would have to wait for a Chrome release, so that would force us to disable AMP in AGSA basically.
If we start with logging + about:blank we could then see if this ever happens.

Re #55: I don't understand; are you proposing that we do just logging and not about:blank for now? (In parts of your comment you say "logging + about:blank" but I think maybe you mean just logging?)

Really this ought to be a question for a Google security team, not Chrome security. If Chrome is displaying the true URL of the page, then Chrome is doing what it's supposed to be doing. If Google is okay with the possibility of serving phishing content on https://www.google.com without the publisher URL bar to mitigate, then I guess that's not Chrome's job to protect against. I don't know who would be the right person to ask about that, though (someone from ISE?).

I meant either logging only, or the "about blank" text in the header without the error page.

Yusuf also mentioned that he can add a serverside flag to turn the fallback handling off in case of bugs, which certainly works for us.

evn@ has been the AMP in Search Security reviewer if we need to run anything by him.

I'll start an email thread with evn@. The more I think about it the more this question really belongs in Search land; it's up to them if they want a safe fallback at the risk of breakage.

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

commit 5febbe024bd044dc8ffb83cbe7acb58621d8f2c1
Author: yusufo <yusufo@chromium.org>
Date: Wed Apr 26 20:40:16 2017

Add tests for TITLE_ONLY Custom Tab Toolbar

Added test checking various conditions with TITLE_ONLY state. There
were conditions with prerender that were not covered by any integration
tests.

BUG= 708957 

Review-Url: https://codereview.chromium.org/2816333002
Cr-Commit-Position: refs/heads/master@{#467442}

[modify] https://crrev.com/5febbe024bd044dc8ffb83cbe7acb58621d8f2c1/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java

Friendly ping from Security FixIt team. Is this issue Fixed?

It looks like the root cause has been fixed and the patch has been merged.

The only blocker left is the decision regarding the fallback as per c#50 and c#58.

Emily, let me please assign this to you to get an update from the email thread with evn@.

No reply from ISE yet, I'm pinging the thread.

Per email thread, sounds like ISE prefers the "safe but break-y" fallback. I guess with a Finch flag to disable it if necessary?

yusufo@, can we close this?

I think it makes sense to move this to 60 if it is OK with security to handle the fallback solution there. The actual fix has been cherrypicked to 58 and I dont think we will want to cherrypick the fallback to 59.

Security, feel free to move the MS back if you disagree. I am going to leave the severity the same. Should we lower that for the fallback?

The fallback seems OK, but maybe we can do better if we can change the API.

Regarding that, I've wrote this in the email thread (no response received yet in the thread):

"""
Also, there's the issue that in this setup, the header can be changed to an arbitrary text if there's an AMP XSS.

IIUC, this is an experimental API currently only available to AGSA, so I've been thinking about whether this issue could be fixed by fine tuning the API.

Would this API be made widely available later? If yes, what's the main use case?

Wouldn't it be better to set the header text from the opening app?

- If this API is only going to be available to AGSA or just first party apps, we can trust the app to set the header text.
- Otherwise, we could let the app set the title only if it has verified that it owns the domain that it displays (IIUC Android provides app-domain association verification).

In this case, AGSA could just set the title to the domain and the displayed page couldn't interfere with it.
"""

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

Sorry for the late response regarding the API. Here are the details.

We are not planning to extend this API to wider use. For a while now, AMP and Chrome teams have been working on a more long term substitute for this API that can be widely available.

The best case would be for the browser to set the header text "in the right way", since we do want some text to show even for 1st parties and in the current setup, we rely heavily on the opening app setting that text. (We trust Google apps on doing the right thing, but can't trust them to not have bugs, hence the necessity of some sort of fallback for both cases whether the web viewer sets the title or the app sets the title).

Actually the worry of bugs on AGSA code would be even higher, since for the Viewer we can roll fixes much faster than Android client updates.

Thanks for the clarification. Maybe I've misunderstood the issue all along. Is the CCT title set by the AGSA app directly, or the AMP runtime on the displayed page by setting document.title? So far I thought it's the latter and was proposing the former, but now after re-reading it seems like it's actually the former. Sorry for the confusion in this case.

I still think "safe but break-y" should be the preferred option independently of the above confusion.

Friendly ping from your security sheriff. yusufo: is there any update here?

Yusuf is out through Monday, but the proposal to replace this special-case is currently in product review.

Friendly ping from your security sheriff.
yusufo, sbirch: is there any update here?

Sorry, for the delay here. The final decision here was to move forward with the launch of Issue 780251 which will change the way this special casing is handled and make it dependent on browser logic. That will make the need for a fallback unnecessary.

Closing this one as it will be clearer to track that implementation on its own launch bug.

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