New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 781670 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
OOO until 4th Feb
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Team-Security-UX



Sign in to add a comment

Add DCHECK which ensures that valid origins are passed to GetPermissionStatus

Project Member Reported by raymes@chromium.org, Nov 6 2017

Issue description

Things can fail in unexpected ways if empty/invalid GURLs are passed.

Initial attempt here: https://chromium-review.googlesource.com/c/chromium/src/+/576409
 
I am interested to work on this
Cc: cm.san...@samsung.com
Owner: raymes@chromium.org
Feel free to take a look. The main thing that has to be fixed are failing tests.
Status: Assigned (was: Available)
Dear Raymes,

I debugged few failues as below:

Callstack : Because of our decheck

[24891:24891:1106/220952.965221:FATAL:permission_context_base.cc(229)] Check failed: requesting_origin.is_valid() && embedding_origin.is_valid(). 
#0 0x000003b7f6bc base::debug::StackTrace::StackTrace()
#1 0x000003b9959c logging::LogMessage::~LogMessage()
#2 0x000003d68733 PermissionContextBase::GetPermissionStatus()
#3 0x000003d6d226 PermissionManager::GetPermissionStatusHelper()
#4 0x000003d6d0f9 PermissionManager::GetPermissionStatus()
#5 0x000006a988be PageInfo::PresentSitePermissions()
#6 0x000006a9653a PageInfo::PageInfo()
#7 0x0000069a6616 PageInfoBubbleView::PageInfoBubbleView()
#8 0x0000069a5f77 PageInfoBubbleView::CreatePageInfoBubble()
#9 0x0000069a7c22 ShowPageInfoDialogImpl()
#10 0x000006877393 ShowPageInfoDialog()
#11 0x000001227b98 (anonymous namespace)::OpenPageInfoBubble()
#12 0x000001227a22 PageInfoBubbleViewBrowserTest_ShowBubble_Test::RunTestOnMainThread()


LOGS as below
[31778:31778:1107/183549.833640:ERROR:page_info_dialog.cc(33)] CHANDRA :: ShowPageInfoDialog URL = about:blank
[31778:31778:1107/183549.833693:ERROR:page_info_bubble_view.cc(463)]  CHANDRA :: PageInfoBubbleView::CreatePageInfoBubble URL =about:blank
[31778:31778:1107/183549.834829:ERROR:page_info_bubble_view.cc(556)] CHANDRA :: PageInfoBubbleView::PageInfoBubbleView URL = about:blank
[31778:31778:1107/183549.834919:ERROR:page_info.cc(823)] CHANDRA :: PageInfo::PageInfo URL=about:blank
[31778:31778:1107/183549.834948:ERROR:permission_manager.cc(603)] CHANDRA :: PermissionManager::GetPermissionStatusHelper requesting_origin = about:blank
[31778:31778:1107/183549.834968:ERROR:permission_manager.cc(604)] CHANDRA :: PermissionManager::GetPermissionStatusHelper embedding_origin = about:blank
[31778:31778:1107/183549.835001:ERROR:permission_manager.cc(608)] CHANDRA :: PermissionManager::GetPermissionStatusHelper canonical_requesting_origin = about:blank
[31778:31778:1107/183549.835020:ERROR:permission_manager.cc(609)] CHANDRA :: PermissionManager::GetPermissionStatusHelper canonical_requesting_origin.GetOrigin() = 
[31778:31778:1107/183549.835042:ERROR:permission_manager.cc(610)] CHANDRA :: PermissionManager::GetPermissionStatusHelper embedding_origin.GetOrigin() =
 
[31778:31778:1107/183549.835065:ERROR:permission_context_base.cc(229)] CHANDRA :: PermissionContextBase::GetPermissionStatus requesting_origin = 
[31778:31778:1107/183549.835085:ERROR:permission_context_base.cc(232)] CHANDRA :: PermissionContextBase::GetPermissionStatus embedding_origin = 


Note: As per my understanding GetOrigin() returning EMPTY as its not standard url.

Plz suggest

Majority failures due to same reason. Do we need to modify test cases with some valid urls or modify chromium code?
Plz suggest.

Thanks in advance
@Raymes,
Could you please check and let me know

Comment 7 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt

Comment 8 by raymes@chromium.org, Nov 10 2017

Sorry, I'm really busy with some urgent things but I'll try to take a look at this late next week.
Cc: benwells@chromium.org
cm.sanchi: interesting. From a quick play page info is available on about:blank, so removing those DCHECKs, going to about:blank, and bringing up page info will probably cause the DCHECK to fail.

Is that right?
sorry that should say, '... so adding those DCHECKs, going to ....'
I mean to say below function returns NULL as its not a standard URL.

GURL GURL::GetOrigin() const {
  // This doesn't make sense for invalid or nonstandard URLs, so return
  // the empty URL.
  if (!is_valid_ || !IsStandard())
    return GURL();

  if (SchemeIsFileSystem())
    return inner_url_->GetOrigin();

  url::Replacements<char> replacements;
  replacements.ClearUsername();
  replacements.ClearPassword();
  replacements.ClearPath();
  replacements.ClearQuery();
  replacements.ClearRef();

  return ReplaceComponents(replacements);
}

So, adding DECHECK fails because we do validate for corrrect urls
DCHECK(requesting_origin.is_valid() && embedding_origin.is_valid());
Right. Yurch. What happens is, permission manager calls .GetOrigin() on urls before passing them into permission context base. GetOrigin does something weird with about:blank, and returns an empty URL.

I think there are two approaches here:

1. Convert everything to use proper url::Origins, instead of GURLs for origins. This way weird things like about:blank can just be represented as unique origins and basically ignored by the permissions system.

2. Specially handle about:blank in ... various places. Maybe in GetOrigin ... maybe in PermissionManager and places that call PermissionContextBase ... maybe the UI.

I prefer 1. but I think we have to figure out file: URLs before we can do that.
Cc: dominickn@chromium.org
+Dom as I mentioned file URLs in #12 and he was talking about them the other day.
Labels: -Hotlist-EnamelAndFriendsFixIt

Sign in to add a comment