Opened 5 months ago

Last modified 2 months ago

#21953 merge_ready defect

Dealing with Tor hardening on Windows properly

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 030-backport, 029-backport
Cc: gk, arthuredelstein Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

While gk is waiting (for something?) in #12426, Tor needs its security mitigations to be corrected according to https://blogs.microsoft.com/microsoftsecure/2009/08/06/setting-sdl-memory-related-requirements-before-your-application-starts/ before the release.
So, adding

  HeapSetInformation(NULL, HeapEnableTerminationOnCorruption, NULL, 0);

after https://gitweb.torproject.org/tor.git/tree/src/or/main.c?h=release-0.3.0#n3570
and changing

    if (setdeppolicy) setdeppolicy(1); /* PROCESS_DEP_ENABLE */

with

    if (setdeppolicy) setdeppolicy(3); /* PROCESS_DEP_ENABLE | PROCESS_DEP_DISABLE_ATL_THUNK_EMULATION */

will do the trick.

Child Tickets

Change History (17)

comment:1 Changed 5 months ago by cypherpunks

Keywords: tbb-needs added
Owner: set to nickm
Points: 0.1
Status: newassigned

Is there anybody here to apply this small fix to rc?

comment:2 Changed 5 months ago by nickm

Cc: gk added
Keywords: 030-backport added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final
Owner: nickm deleted

It needs a reviewer who knows Windows well. Having a patch would also be nice.

Gk, do you agree this is tbb-needs?

comment:3 in reply to:  2 Changed 5 months ago by gk

Keywords: tbb-needs removed

Replying to nickm:

It needs a reviewer who knows Windows well. Having a patch would also be nice.

Gk, do you agree this is tbb-needs?

I don't think so. It would surely be a good thing to have but it is not even Tor Browser specific. Thus, removing the keyword. cypherpunks: Please, do not set random keywords we need to keep track of our workflow. Thanks.

comment:4 Changed 5 months ago by cypherpunks

Thanks, Nick. Let's see how long does the formal process take.

@gk: It is tbb-wants since 2009 at least, so it could be tbb-needs in 2017. Official Tor builds for Windows are used (and produced) primarily by Tor Browser, so it is affected.

comment:5 Changed 5 months ago by tom

I can review for Windows.

Patch is good hygiene and worth taking, but it's not going to radically improve tor's security.

As noted, HeapEnableTerminationOnCorruption is enabled by default in Win8+.

PROCESS_DEP_DISABLE_ATL_THUNK_EMULATION I do not believe is needed because we're not using ATL library/framework stuff. But it's fine to have, and like I said, good hygiene.

comment:6 Changed 5 months ago by nickm

Keywords: 029-backport added
Status: assignedmerge_ready

Okay; I've written a branch here as branch ticket21953_029 in my public git repository. Is it what people had in mind? Please confirm if so, and correct if I've got it wrong.

comment:7 in reply to:  6 Changed 5 months ago by tom

Replying to nickm:

Okay; I've written a branch here as branch ticket21953_029 in my public git repository. Is it what people had in mind? Please confirm if so, and correct if I've got it wrong.

LGTM

comment:8 Changed 4 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.0.x-final

Merging to 0.3.1; if it works out we can backport.

comment:9 Changed 4 months ago by teor

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final
Status: merge_readyneeds_revision

This breaks our jenkins Windows builder with:

gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I../tor  -I../tor/src/ext -Isrc/ext -I../tor/src/ext/trunnel -I../tor/src/trunnel -I../tor/src/common -Isrc/common -I../tor/src/ext/trunnel -I../tor/src/trunnel -I../tor/src/or -Isrc/or -DSHARE_DATADIR="\"/usr/share\"" -DLOCALSTATEDIR="\"/usr/var\"" -DBINDIR="\"/usr/bin\"" -I../tor/src -DTOR_UNIT_TESTS -I../tor/src/common -I../UPSTREAM/usr/include -I../UPSTREAM/usr/include -I../UPSTREAM/usr/include   -g -O2 -Wno-error=redundant-decls -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector -fasynchronous-unwind-tables -Wall -fno-strict-aliasing -Waddress -Warray-bounds -Wdouble-promotion -Wextra -Winit-self -Wlogical-op -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-noreturn -Woverlength-strings -Woverride-init -Wshadow -Wsync-nand -Wtrampolines -Wunused-but-set-parameter -Wunused-but-set-variable -Wunused-local-typedefs -Wvariadic-macros -W -Wfloat-equal -Wundef -Wpointer-arith -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls -Wchar-subscripts -Wcomment -Wformat=2 -Wwrite-strings -Wnested-externs -Wbad-function-cast -Wswitch-enum -Waggregate-return -Wpacked -Wunused -Wunused-parameter  -Wold-style-definition -Wmissing-declarations -Werror -MT src/or/src_or_libtor_testing_a-main.o -MD -MP -MF src/or/.deps/src_or_libtor_testing_a-main.Tpo -c -o src/or/src_or_libtor_testing_a-main.o `test -f 'src/or/main.c' || echo '../tor/'`src/or/main.c
../tor/src/or/main.c: In function 'tor_main':
../tor/src/or/main.c:3671:28: error: 'HeapEnableTerminationOnCorruption' undeclared (first use in this function)
../tor/src/or/main.c:3671:28: note: each undeclared identifier is reported only once for each function it appears in
make[1]: *** [src/or/src_or_libtor_testing_a-main.o] Error 1

https://jenkins.torproject.org/job/tor-ci-windows-commit/1293/

This part of the patch needs to be made conditional on the existence of the function.
We must fix this or back out the change before the 0.3.1 release.

comment:10 Changed 4 months ago by nickm

https://msdn.microsoft.com/en-us/library/windows/desktop/aa366705(v=vs.85).aspx says that HeapEnableTerminationOnCorruption is always 1. So there's that option...

comment:11 Changed 4 months ago by gk

I wonder why this does not break our Tor Browser nightly builds, though. The latest one has tor based on bbeba2412e58501d and got properly cross-compiled, it seems.

comment:12 Changed 4 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.0.x-final
Status: needs_revisionneeds_review

Added a new commit to bug21953_029 and merged it to master.

gk: my theory would be that some mingw versions have newer headers than others. In the past, the mingw headers have tended to lag the MSVC headers a bit.

comment:13 Changed 4 months ago by cypherpunks

If you don't like Windows headers so much (especially, windows.h), then, of course, 1 instead of HeapEnableTerminationOnCorruption is your option (like in setdeppolicy), but not redefinition of Windows constants.

HeapSetInformation(NULL, 1 /* HeapEnableTerminationOnCorruption */, NULL, 0);

comment:14 in reply to:  12 Changed 4 months ago by gk

Replying to nickm:

Added a new commit to bug21953_029 and merged it to master.

gk: my theory would be that some mingw versions have newer headers than others. In the past, the mingw headers have tended to lag the MSVC headers a bit.

Yes, I had the same theory but was a bit unsure as the respective change is available since 2013 in mingw-w64's headers. But maybe the version used in our Jenkins builds is indeed older than that.

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

comment:15 Changed 4 months ago by nickm

Status: needs_reviewmerge_ready

comment:16 Changed 4 months ago by nickm

Keywords: 030-backport 029-backport030-backport, 029-backport

comment:17 Changed 2 months ago by arthuredelstein

Cc: arthuredelstein added
Note: See TracTickets for help on using tickets.