Opened 4 years ago

Closed 4 years ago

#16397 closed defect (fixed)

Tor Browser crashes on some SVG images

Reported by: mcap Owned by: mcs
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-crash, tbb-usability-website, TorBrowserTeam201506R
Cc: mcs, brade, mikeperry, gk, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hello,

When I visit this link http://www.theplantlist.org/browse/B/Sphagnaceae/Sphagnum/ Tor Browser just closes even if I have multiple tabs/windows open.

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by gk

Cc: mcs brade added
Keywords: tbb-crash tbb-usability-website added
Priority: normalcritical

This happens on the high security slider setting and is due to the SVG patch. mcs, brade could you have a look? I tried to poke around with GDB a bit but it was just freezing my browser giving no good stack trace back. I might look at it later today again.

Last edited 4 years ago by gk (previous) (diff)

comment:2 Changed 4 years ago by mcs

This crash is caused by a variation of the problem we fixed in #15794. The difference seems to be that the page at http://www.theplantlist.org/browse/B/Sphagnaceae/Sphagnum/ uses <object> to load the SVG instead of using an <svg> tag. Kathy and I are testing a fix.

comment:3 Changed 4 years ago by mcs

Owner: changed from tbb-team to mcs
Status: newassigned

comment:4 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201506R added
Status: assignedneeds_review

A fix is available here:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16397-01&id=2c1d0dada80bad43abba0f4a9d283bf5dc02c201
Please review.

This crash was in nsXMLContentSink.cpp (first change in our patch). But Kathy and I tried hard to locate each place where the Mozilla code assumed that a <script> element could be QI'd to an nsIScriptElement, and then we fixed the unsafe pointer dereferences.

I think this fix can wait for our next scheduled release. But it is a NULL pointer dereference and a potential denial of service for users who have the security slider set to the high setting.

comment:5 Changed 4 years ago by mcs

Cc: mikeperry gk added

comment:6 Changed 4 years ago by gk

Looks good to me (I have not tried to compile the patch and to run a respective build, though).

comment:7 in reply to:  6 Changed 4 years ago by mcs

Cc: arthuredelstein added

Replying to gk:

Looks good to me (I have not tried to compile the patch and to run a respective build, though).

Thanks for your review. It would be nice to include this patch in 5.0a3 so it gets some testing. Mike or Arthur, please provide a second review if you have time.

comment:8 Changed 4 years ago by gk

FWIW: I think we should fix that in the next stable as well.

comment:9 Changed 4 years ago by mikeperry

Status: needs_reviewneeds_revision

This looks OK to me. I picked this branch up for the stable, but had to cheat and turn it into a patch. It seems to be based off of a weird branch..

It did not apply to the 38 branch at all, due to file relocations, etc. Can you make a new patch against tor-browser.git/tor-browser-38.1.0esr-5.x-1?

comment:10 in reply to:  9 Changed 4 years ago by mcs

Status: needs_revisionneeds_review

Replying to mikeperry:

This looks OK to me. I picked this branch up for the stable, but had to cheat and turn it into a patch. It seems to be based off of a weird branch..

Hmmm. It should not have been based off anything weird. Thanks for fixing it.

It did not apply to the 38 branch at all, due to file relocations, etc. Can you make a new patch against tor-browser.git/tor-browser-38.1.0esr-5.x-1?

A rebased patch is available here:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16397-02&id=455a21820dff054fb8bde1417f3d0a289917d932
We updated to the latest tor-browser-38.1.0esr-5.x-1 a few hours ago and built and tested this, so it should be good to go.

comment:11 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed
Summary: Tor Browser closes unexpectedlyTor Browser crashes on some SVG images

Ok, merged for 5.0a3 as well.

Note: See TracTickets for help on using tickets.