Opened 3 years ago

Closed 3 years ago

#21962 closed defect (fixed)

Segmentation fault with "high" security when changing in about:addons to "Extensions" or "Appearance"

Reported by: viktorj Owned by: mcs
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-crash, tbb-usability, ff52-esr, tbb-7.0-must-alpha, TorBrowserTeam201704R
Cc: viktor_jaegerskuepper@…, arthuredelstein, mcs, brade, gk, notanon740, Dbryrtfbcbhgf Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor4

Description

Tor-Browser version: 7.0a3-build4, 64 bit Linux

The segmentation fault is maybe only the most annoying part of the underlying issue. If the security level is set to "High" and after a restart of Tor Browser, opening about:addons and trying to change to the "Extensions" or "Appearance" panel leads to a segmentation fault.

Other effects of setting the security level to "High" which may be related or may help you finding the root cause:
(after closing and restarting)

  • the symbols in front of the panel descritions in about:preferences are not visible
  • the checkboxes in about:preferences don't show a check mark or a dot if they are selected

Unfortunately I don't know how to debug this, but I hope you can reproduce this easily.

Child Tickets

Change History (23)

comment:1 Changed 3 years ago by gk

Cc: arthuredelstein mcs brade added
Keywords: tbb-crash tbb-usability ff52-esr tbb-7.0-must-alpha TorBrowserTeam201704 added
Priority: MediumVery High
Severity: NormalMajor
Sponsor: Sponsor4

Might be good to get that squeezed into the final 7.0a3 if possible.

comment:2 Changed 3 years ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newaccepted

comment:3 Changed 3 years ago by cypherpunks

Block SVG in content: now all pages are content (e10s?). What is with new ff patch, the same?

comment:4 in reply to:  3 Changed 3 years ago by arthuredelstein

Replying to cypherpunks:

Block SVG in content: now all pages are content (e10s?). What is with new ff patch, the same?

Indeed this problem seems to be related to the svg.in-content.enabled pref. If I choose High Security but set this pref to "true" then the problems in the description (including the crash) go away.

comment:5 Changed 3 years ago by gk

Cc: gk added

comment:6 Changed 3 years ago by gk

FWIW: This is not e10s related. We are crashing somewhere in CSS code:

#0  nsAttrValue::Contains (this=this@entry=0xa5a5a5a5a5a5a5a5, 
    aValue=0x7fffc869cb80, aCaseSensitive=eCaseMatters)
    at /home/debian/build/tor-browser/dom/base/nsAttrValue.cpp:1163
#1  0x00007ffff3a0024b in SelectorMatches (aElement=0x7fffc407d600, 
    aSelector=0x7fffd708e460, aNodeMatchContext=..., aTreeMatchContext=..., 
    aSelectorFlags=aSelectorFlags@entry=SelectorMatchesFlags::NONE, 
    aDependence=0x0, this=<optimized out>)
    at /home/debian/build/tor-browser/layout/style/nsCSSRuleProcessor.cpp:1723
#2  0x00007ffff3a00d21 in SelectorMatches (aElement=aElement@entry=0x7fffc407d600, 
    aSelector=aSelector@entry=0x7fffd708e460, aNodeMatchContext=..., 
    aTreeMatchContext=..., 
    aSelectorFlags=aSelectorFlags@entry=SelectorMatchesFlags::NONE, 
    aDependence=aDependence@entry=0x0, this=<optimized out>)
    at /home/debian/build/tor-browser/layout/style/nsCSSRuleProcessor.cpp:1670
#3  0x00007ffff3a00d6c in nsCSSRuleProcessor::RestrictedSelectorMatches (
    aElement=aElement@entry=0x7fffc407d600, aSelector=0x7fffd708e460, 
    aTreeMatchContext=...)
    at /home/debian/build/tor-browser/layout/style/nsCSSRuleProcessor.cpp:2299
#4  0x00007ffff3ac3085 in mozilla::ElementRestyler::SelectorMatchesForRestyle (
    this=0x7fffffffae20, aElement=0x7fffc407d600)
    at /home/debian/build/tor-browser/layout/base/RestyleManager.cpp:2537

comment:7 Changed 3 years ago by mcs

Kathy and I are also looking at this ticket. Arthur, please let us know if you are making progress so we are not duplicating work.

We can reproduce the crash, and we believe that the immediate cause is the static_cast that is in this code from Element.cpp:

const nsAttrValue*
nsIContent::DoGetClasses() const
{
  MOZ_ASSERT(HasFlag(NODE_MAY_HAVE_CLASS), "Unexpected call");
  MOZ_ASSERT(IsElement(), "Only elements can have classes");

  if (IsSVGElement()) {
    const nsAttrValue* animClass =
      static_cast<const nsSVGElement*>(this)->GetAnimatedClassName();
    if (animClass) {
      return animClass;
    }
  }

  return AsElement()->GetParsedAttr(nsGkAtoms::_class);
}

But the above code is not new. Our current working theory is that SVGs are being blocked in error early during creation of the about:addons document (and possibly in other cases) even though they should be allowed. If some time later SVGs are perceived as allowed, then Bad Things will occur such as doing a static_cast to the wrong kind of object.

In theory, and hopefully in practice, the Mozilla patch to block SVGs is better than our approach because it assigns an alternate namespace for SVGs at element creation time, which should avoid these kinds of static_cast bugs.

comment:8 Changed 3 years ago by mcs

Kathy and I tracked down the root cause of the crash (which is also causing SVG images to not appear in about:preferences). Apparently, for some subresource documents, SVG elements are created before the document is attached to the parent window. This causes NS_SVGEnabledForChannel() to fail to perform its whitelist check for documents such as toolkit/mozapps/extensions/content/extensions.xml (because we end up with a NULL topDocURI), which in turn causes SVGs to be disabled at first and later allowed (because ultimately the subresource is part of about:addons, which is whitelisted).

I am not sure what changed between Firefox 45 and 52 to cause this problem, but adding a check against the system principal in this specific case seems to fix things. It is also worth noting that Mozilla's patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1216893 uses IsSystemPrincipal() checks too.

We will post a patch soon.

comment:9 Changed 3 years ago by cypherpunks

Shouldn't you just backport the Mozilla's patches?

comment:10 Changed 3 years ago by arthuredelstein

Status: acceptednew

I've been working on the backport approach, which does get rid of the crash. Unfortunately there is a lot missing from Mozilla's version that needs to be added back from Kathy and Mark's old SVG patch and I'm still debugging some issues. I'm opening a new ticket (#22002) for the backport, because I think we should go with Mark and Kathy's old patch (with fixup) for now.

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:11 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201704 removed
Status: newneeds_review

comment:12 Changed 3 years ago by gk

Cc: notanon740 added

#22008 is a duplicate.

comment:13 Changed 3 years ago by mcs

Cc: Dbryrtfbcbhgf added

#22023 is a duplicate.

comment:14 Changed 3 years ago by arthuredelstein

Owner: changed from arthuredelstein to mcs
Status: needs_reviewassigned

comment:15 Changed 3 years ago by arthuredelstein

Status: assignedneeds_review

comment:16 Changed 3 years ago by cypherpunks

This bug in content crashes the parent process that shouldn't be at any circumstances. e10s design flaw?

comment:17 in reply to:  11 Changed 3 years ago by gk

Replying to mcs:

Here is our patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug21962-01&id=4cc114a57704a76988cb7862c6be2fd94639d144

Thanks for tracking this down and providing this fix. Good job! I've tested the fix on Windows and a 32bit Linux system and it fixes the issue. The code looks good to me as well.

Arthur, what do you think?

comment:18 in reply to:  16 Changed 3 years ago by mcs

Replying to cypherpunks:

This bug in content crashes the parent process that shouldn't be at any circumstances. e10s design flaw?

Both content and chrome callers go through the same codepath for SVGs, so I would say the bug was not limited to content.

comment:19 Changed 3 years ago by arthuredelstein

This patch also looks good to me. A couple of nitpicky questions occur to me:

  • Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?
  • I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

comment:20 in reply to:  19 ; Changed 3 years ago by mcs

Status: needs_reviewneeds_information

Replying to arthuredelstein:

This patch also looks good to me. A couple of nitpicky questions occur to me:

  • Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?

Yes, for example if isSVGAllowed is set to true because the load context is not content.

  • I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Maybe, but Kathy and I prefer to leave the debug logging together near the end of the function to ensure that something is logged in all cases (and to make it easier to determine that is the case).

Georg, what do you think?

comment:21 in reply to:  20 ; Changed 3 years ago by gk

Replying to mcs:

Replying to arthuredelstein:

This patch also looks good to me. A couple of nitpicky questions occur to me:

  • Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?

Yes, for example if isSVGAllowed is set to true because the load context is not content.

  • I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Maybe, but Kathy and I prefer to leave the debug logging together near the end of the function to ensure that something is logged in all cases (and to make it easier to determine that is the case).

Georg, what do you think?

I am with you here, let's leave this as-is. Arthur: anything else or do you think this is fine for the alpha?

comment:22 in reply to:  21 Changed 3 years ago by arthuredelstein

Replying to gk:

Replying to mcs:

Replying to arthuredelstein:

This patch also looks good to me. A couple of nitpicky questions occur to me:

  • Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?

Yes, for example if isSVGAllowed is set to true because the load context is not content.

Gotcha.

  • I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Maybe, but Kathy and I prefer to leave the debug logging together near the end of the function to ensure that something is logged in all cases (and to make it easier to determine that is the case).

Georg, what do you think?

I am with you here, let's leave this as-is. Arthur: anything else or do you think this is fine for the alpha?

I think this patch is fine. Thanks for the clarifications.

comment:23 Changed 3 years ago by gk

Resolution: fixed
Status: needs_informationclosed

This is commit 2d386f0b0ce34aaf97b814da1867bb94a93b62a2 on tor-browser-52.1.0esr-7.0-2 now. Thanks all.

Note: See TracTickets for help on using tickets.