Opened 3 years ago

Closed 3 years ago

#20439 closed defect (fixed)

The firefox binary in Tor Browser on OSX is not PIE

Reported by: boklm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-hardened, TorBrowserTeam201611R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

otool -hv says that the firefox binary from Tor Browser on OSX is not PIE:

$ otool -hv firefox 
firefox:
Mach header
      magic cputype cpusubtype  caps    filetype ncmds sizeofcmds      flags
MH_MAGIC_64  X86_64        ALL LIB64     EXECUTE    22       2752   NOUNDEFS DYLDLINK TWOLEVEL BINDS_TO_WEAK

While on the firefox binary from Mozilla, it says this:

$ otool -hv /Volumes/Firefox/Firefox.app/Contents/MacOS/firefox
/Volumes/Firefox/Firefox.app/Contents/MacOS/firefox:
Mach header
      magic cputype cpusubtype  caps    filetype ncmds sizeofcmds      flags
MH_MAGIC_64  X86_64        ALL LIB64     EXECUTE    22       2744   NOUNDEFS DYLDLINK TWOLEVEL BINDS_TO_WEAK PIE

Child Tickets

Change History (9)

comment:1 Changed 3 years ago by boklm

I attached a first version of a patch to fix that. After building with this patch, the firefox binary is reported as PIE by otool -hv.

Before adding wrappers for clang and clang++ adding the -fPIE option, I first tried to add it to the CFLAGS variable in .mozconfig-mac, but this makes the build fail with this error:

libtool: compile:  /home/debian/build/tor-browser/clang/bin/clang -target x86_64-apple-darwin10 -mlinker-version=136 -B /home/debian/build/tor-browser/cctools/bin -isysroot /home/debian/build/tor-browser/MacOSX10.7.sdk -DHAVE_CONFIG_H -I. -I/home/debian/build/tor-browser/js/src/ctypes/libffi -I. -I/home/debian/build/tor-browser/js/src/ctypes/libffi/include -Iinclude -I/home/debian/build/tor-browser/js/src/ctypes/libffi/src -Qunused-arguments -Qunused-arguments -Wall -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits -Wno-unused -std=gnu99 -fno-strict-aliasing -fno-math-errno -pthread -DNO_X11 -pipe -fexceptions -MT src/x86/ffi.lo -MD -MP -MF src/x86/.deps/ffi.Tpo -c /home/debian/build/tor-browser/js/src/ctypes/libffi/src/x86/ffi.c  -fno-common -DPIC -o src/x86/ffi.o
depbase=`echo src/x86/darwin.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
        /bin/bash ./libtool    --mode=compile /home/debian/build/tor-browser/clang/bin/clang -target x86_64-apple-darwin10 -mlinker-version=136 -B /home/debian/build/tor-browser/cctools/bin -isysroot /home/debian/build/tor-browser/MacOSX10.7.sdk -fPIE -DHAVE_CONFIG_H -I. -I/home/debian/build/tor-browser/js/src/ctypes/libffi  -I. -I/home/debian/build/tor-browser/js/src/ctypes/libffi/include -Iinclude -I/home/debian/build/tor-browser/js/src/ctypes/libffi/src -Qunused-arguments  -I. -I/home/debian/build/tor-browser/js/src/ctypes/libffi/include -Iinclude -I/home/debian/build/tor-browser/js/src/ctypes/libffi/src -Qunused-arguments -Wall -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits -Wno-unused -std=gnu99 -fno-strict-aliasing -fno-math-errno -pthread -DNO_X11 -pipe -MT src/x86/darwin.lo -MD -MP -MF $depbase.Tpo -c -o src/x86/darwin.lo /home/debian/build/tor-browser/js/src/ctypes/libffi/src/x86/darwin.S &&\
        mv -f $depbase.Tpo $depbase.Plo
libtool: compile: unable to infer tagged configuration
libtool: compile: specify a tag with `--tag'

Maybe the wrappers should be created in gitian-utils.yml instead of gitian-firefox.yml? Unless there is a better way to make -fPIC the default.

The mac-pie.patch patch should also be a separate patch for tor-browser.git.

comment:2 Changed 3 years ago by gk

Keywords: tbb-hardened added

I think ideally I'd like to have all necessary changes in one place and not split into different repos (especially if it is only about setting the proper compiler/linker flags). I am not sure, though, I understand yet why your first try is failing while the second succeeds. Thus, it is a bit hard to make a good case for e.g. putting everything into .mozconfig-mac. That said, if missing PIE affects other components as well (tor comes to mind here) we might indeed want to think about a more general, non-mozconfig solution anyway...

comment:3 in reply to:  2 Changed 3 years ago by boklm

Replying to gk:

I think ideally I'd like to have all necessary changes in one place and not split into different repos (especially if it is only about setting the proper compiler/linker flags).

I am not sure, though, I understand yet why your first try is failing while the second succeeds.

I am not completely sure either, but it looks like libtool in js/src/ctypes/libffi is parsing the arguments to find the type of command it is running, and having -fPIE in the arguments makes it fail. I think the second succeeds because -fPIE is not in the list of arguments in this case.

A better fix might be to patch libffi to use the --tag= options when calling libtool. I will try that.

Thus, it is a bit hard to make a good case for e.g. putting everything into .mozconfig-mac. That said, if missing PIE affects other components as well (tor comes to mind here) we might indeed want to think about a more general, non-mozconfig solution anyway...

tor is not affected, I think because the configure.ac is adding the -fPIE and -pie flags. The pluggable transports are not PIE, but they are not built using llvm. So the only component affected by this at the moment seems to be firefox.

comment:4 Changed 3 years ago by gk

FWIW: I'd rather be happy to have some kind of patch for the next alpha than nothing. Thus, if we don't get to the bottom of the problem let's go with the wrapper for now.

comment:5 Changed 3 years ago by boklm

Keywords: TorBrowserTeam201611R added
Status: newneeds_review

I attached 2 patches to this ticket:

  • tor-browser-0001-Bug-20439-make-the-build-PIE-on-OSX.patch is a patch for tor-browser.git adding -Wl,-pie to LDFLAGS.
  • tor-browser-bundle-0001-Bug-20439-make-the-build-PIE-on-OSX.patch is a patch for builders/tor-browser-bundle.git creating wrappers on clang and clang++ to add the -fPIE flag.

comment:6 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. One nit: I added a fixup! rule for the .mozconfig-mac patch (in case you were wondering). This is on tor-browser-bundle master commit 565f2cc6e5c8e496bc50f03defa98947e24c93c4 and on tor-browser-45.4.0esr-6.5-1 db846f717b90c587b1934c65dd973e0ba0f14acd.

Note: See TracTickets for help on using tickets.