Opened 7 years ago

Last modified 6 months ago

#4280 assigned defect

build changes for TBB

Reported by: ioerror Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-security, apparmor
Cc: erinn, hellais, mikeperry, g.koppen@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

re #2176 - I came up with a list of things I think we should disable or change in TBB's build process.

I noticed that as it stands, we don't disable stuff like JSctypes. Which, well, if it's anything like python ctypes, holy moley!

diff --git a/build-scripts/config/dot_mozconfig b/build-scripts/config/dot_mozconfig
index 9333a6f..227bd01 100755
--- a/build-scripts/config/dot_mozconfig
+++ b/build-scripts/config/dot_mozconfig
@@ -5,5 +5,16 @@ mk_add_options MOZ_APP_DISPLAYNAME=TorBrowser
 
 ac_add_options --enable-optimize
 ac_add_options --enable-strip
+ac_add_options --enable-install-strip
 ac_add_options --disable-tests
 ac_add_options --disable-debug
+ac_add_options --disable-ctypes
+ac_add_options --disable-necko-disk-cache
+ac_add_options --disable-necko-wifio
+ac_add_options --disable-installer
+ac_add_options --disable-updater
+ac_add_options --disable-parental-controls
+
+
+# Linux options
+ac_add_options --disable-dbus

Child Tickets

Change History (13)

comment:1 Changed 7 years ago by mikeperry

Cc: mikeperry added

You already get arbitrary code long before you get to ctypes. As soon as you get any sort of JS running with XPCOM privs, it is game over. Disabling ctypes just makes it impossible for people to run addons like Moxie's Convergence in TBB. It's not terribly safe for them to do it anyway, but at least we should not require they rebuild TBB to test it and other addons for us.

The WifiIO has similar uses in extensionland to detect network changes. And why is location data necessarily deanonymizing if you enable it temporarily in emergency situations while on the road? I've done this from time to time while traveling. It can be very handy.

Does it actually make the build smaller? If not, who cares? All of it is dealt with in extensionland via torbutton.

Disabling everything in two places just makes our job harder if we want to use that functionality later, or if users insist on testing new and exciting configurations for us.

Nearly everything on this list is something we've contemplated using at one point or another, except strip, parental controls, and disk-cache. And does the disk-cache option really disable just the disk cache, or the memory cache too?

comment:2 in reply to:  1 ; Changed 7 years ago by ioerror

Replying to mikeperry:

You already get arbitrary code long before you get to ctypes. As soon as you get any sort of JS running with XPCOM privs, it is game over. Disabling ctypes just makes it impossible for people to run addons like Moxie's Convergence in TBB. It's not terribly safe for them to do it anyway, but at least we should not require they rebuild TBB to test it and other addons for us.

That sounds like a good reason to disable it. We should aim to reduce object code and even functionality when we currently do not use it. I just imagine a user installing any old plugin thinking that it's safe - isn't that exactly what we want to avoid? We should only *support* as opposed to allow extensions that we have either audited or that we believe to be safe.

The WifiIO has similar uses in extensionland to detect network changes. And why is location data necessarily deanonymizing if you enable it temporarily in emergency situations while on the road? I've done this from time to time while traveling. It can be very handy.

That seems pretty much like a freak chance that by allowing we'll see users screwed.

Does it actually make the build smaller? If not, who cares? All of it is dealt with in extensionland via torbutton.

Yeah, I think that it does.

Disabling everything in two places just makes our job harder if we want to use that functionality later, or if users insist on testing new and exciting configurations for us.

I disagree that it makes our job harder. We have experimental builds for experiments and we have stable builds for stability, security and anonymity.

Nearly everything on this list is something we've contemplated using at one point or another, except strip, parental controls, and disk-cache. And does the disk-cache option really disable just the disk cache, or the memory cache too?

According to the docs, I see no reason to enable those flags. YMMV. I think it's worth considering.

comment:3 in reply to:  2 ; Changed 7 years ago by mikeperry

Replying to ioerror:

Replying to mikeperry:

Disabling everything in two places just makes our job harder if we want to use that functionality later, or if users insist on testing new and exciting configurations for us.

I disagree that it makes our job harder. We have experimental builds for experiments and we have stable builds for stability, security and anonymity.

It does in fact make it harder. You just haven't been active enough in TBB development to see it.

Just dealing with all the places we have disabled flash was a huge pain. It took us 3 or 4 builds to get flash loading again on all platforms. And I still can't figure out all the ways we prevent cookies from being written to disk so that users who want to store protected cookies can do so.

Every change we make to disable something requires everyone knowing about it and all other changes when they want to undo them to add features.

We also don't currently have experimental TBB builds... Making them always behave differently (as opposed to just working on them until we can call them stable) is more work and maintenance. It will also introduce fun surprises due to differences between "stable" and "experimental" behavior when we try to convert out alpha "experimental" series into the new "stable".

According to the docs, I see no reason to enable those flags. YMMV. I think it's worth considering.

My vote is for only:
+ac_add_options --enable-install-strip
+ac_add_options --disable-parental-controls

This one needs code review to ensure it doesn't break stuff/disable all caching:
+ac_add_options --disable-necko-disk-cache

These two will probably make my life harder for #4234:
+ac_add_options --disable-installer
+ac_add_options --disable-updater

The others are just bad ideas that will break stuff for no gain, IMO.

In fact, I'm not even sure any of these are really worth it, unless they actually save us significant disk space in the .dmg or tgz.

comment:4 in reply to:  3 Changed 7 years ago by ioerror

Replying to mikeperry:

Replying to ioerror:

Replying to mikeperry:

Disabling everything in two places just makes our job harder if we want to use that functionality later, or if users insist on testing new and exciting configurations for us.

I disagree that it makes our job harder. We have experimental builds for experiments and we have stable builds for stability, security and anonymity.

It does in fact make it harder. You just haven't been active enough in TBB development to see it.

Ok, I won't argue that it's harder as that's clearly subjective. I've been involved with TBB for many years and I agree that lately, I've been watching development more than anything else. However, I strongly feel like shipping gobs of stuff that we don't support or use is a bad idea. It seems like you need an explicit design goal (and perhaps I missed it ) of "stay as close to Firefox as possible" for all things above anything else.

Just dealing with all the places we have disabled flash was a huge pain. It took us 3 or 4 builds to get flash loading again on all platforms. And I still can't figure out all the ways we prevent cookies from being written to disk so that users who want to store protected cookies can do so.

That's an argument for no Flash but I hear your point.

Every change we make to disable something requires everyone knowing about it and all other changes when they want to undo them to add features.

Yeah, forking a browser is a nightmare, I get it. That's why we need to document, work for minimal changes, decide what we support and so on.

We also don't currently have experimental TBB builds... Making them always behave differently (as opposed to just working on them until we can call them stable) is more work and maintenance. It will also introduce fun surprises due to differences between "stable" and "experimental" behavior when we try to convert out alpha "experimental" series into the new "stable".

I guess I mean alpha when I said experimental.

According to the docs, I see no reason to enable those flags. YMMV. I think it's worth considering.

My vote is for only:
+ac_add_options --enable-install-strip
+ac_add_options --disable-parental-controls

That's plus one - we already had strip, I believe.

This one needs code review to ensure it doesn't break stuff/disable all caching:
+ac_add_options --disable-necko-disk-cache

Agreed.

These two will probably make my life harder for #4234:
+ac_add_options --disable-installer
+ac_add_options --disable-updater

Ok, seems like not using those is fine.

The others are just bad ideas that will break stuff for no gain, IMO.

In fact, I'm not even sure any of these are really worth it, unless they actually save us significant disk space in the .dmg or tgz.

That's a reason to test - I think Erinn is be suited to tell us this? I mean, if it shaves off say, 5MB - is it worth it? I think so.

comment:5 Changed 7 years ago by gk

Cc: g.koppen@… added

comment:6 Changed 7 years ago by mikeperry

Component: Tor BrowserTor bundles/installation
Owner: changed from mikeperry to erinn

comment:7 Changed 7 years ago by erinn

Mike,

Just do double check, you only want me to enable these?

+ac_add_options --enable-install-strip
+ac_add_options --disable-parental-controls

Verify and I'll commit the changes to our mozconfigs. Thanks.

comment:8 Changed 7 years ago by erinn

Status: newneeds_information

comment:9 Changed 7 years ago by mikeperry

Status: needs_informationassigned

Well, tbh more accurately I see no harm in them. That doesn't mean there is no harm.

Also, I've thought about ioerror's other comments, and if you are using a least privileged wrapper around TBB like seatbelt, selinux, or apparmor, then the --disable-ctypes *will* actually increase security...

I think the best plan is to add the following to our alpha builds and see what breaks:

+ac_add_options --enable-install-strip
+ac_add_options --disable-parental-controls
+ac_add_options --disable-ctypes

If we can't add these options only to the alpha builds yet, we should hold off entirely until we can.

comment:10 Changed 5 years ago by erinn

Keywords: needs-triage added

comment:11 Changed 5 years ago by erinn

Component: Tor bundles/installationTor Browser

comment:12 Changed 20 months ago by cypherpunks

Keywords: tbb-security added; needs-triage removed
Owner: changed from erinn to tbb-team
Severity: Normal

Part of every rebasing.

comment:13 Changed 6 months ago by traumschule

Keywords: apparmor added

group tickets related to AppArmorForTBB/tor packages

Note: See TracTickets for help on using tickets.