Closed Bug 642243 Opened 13 years ago Closed 13 years ago

run binscope to alert us and turn the tree red when we don't use ASLR and other protection tools

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla11

People

(Reporter: chofmann, Assigned: imelven)

References

Details

(Whiteboard: [sg:want P2])

Attachments

(2 files, 8 obsolete files)

BinScope report for 4.0rc1
1.80 MB, text/html
Details
patch v5
5.42 KB, patch
imelven
: review+
Details | Diff | Splinter Review 
follow up from [Bug 641630] ANGLE's libEGL.dll and libGLESv2.dll don't have ASLR enabled.

confidential until we get something in place and/or that bug is openned.

wonder if it would make sense to add this as a test and turn the tree red when SDL requirements are not met

http://www.microsoft.com/downloads/en/details.aspx?displaylang=en&FamilyID=90e6181c-5905-4799-826a-772eafd4440a

dveditz looked at this a bit, and it may not be the right tool but we need it or something similar.
I ran all BinScope tests against 4.0rc1, we'll have to figure out which ones we actually care about. There are several failures we should fix.
Whiteboard: [sg:want P2][private while bug 641630 is private]
It'd probably be easier to use dumpbin, or just write a custom tool in C++ to do this. Parsing the PE headers isn't terribly hard, Microsoft provides all the structure definitions in ImageHlp.h. (I wrote attachment 449081 [details] which uses them to get at some information.)
dumpbin /headers can get some of these (NX and ASLR) : 

            8140 DLL characteristics
                   Dynamic base
                   NX compatible
                   Terminal Server Aware

binscope seems to do a bit more than this obviously - /GS is a really big one we need as well, need to look more into how binscope detects that
I haven't ever looked into it, but presumably you could look for an import of __security_init_cookie. /GS is on by default in VC8 or newer, so it seems less likely to be a problem.
i just ran binscope against browser.exe and plugin-container.exe for nightly (7), aurora (6), and FF5 (release) and saw no issues :)

the symbol server string SRV*c:\symcache\*http://symbols.mozilla.org/firefox needs to be used to obtain results

binscope can also be run in command line mode and we can supply an xslt as well to get results in whatever format we need for the build system

i guess the next step is to find someone in releng to talk to about this ? any suggestions from folks cc'd on the bug ?
seems like to might be somewhere in the middle of figuring out how this fits into the build infrastructure or the test automation infrastructure, which sadly would mean no one steps up to figure out to make it start happening.  cc'ing bmoss and joduin to find the best owner for this.
Group: core-security
Whiteboard: [sg:want P2][private while bug 641630 is private] → [sg:want P2]
If the tools are on the machines and can be run from the command line, we can just add it to make check.
(In reply to comment #7)
> If the tools are on the machines and can be run from the command line, we
> can just add it to make check.

binscope definitely won't be on the machine by default, it also required NET Framework 2.0 and maybe some other fun dependencies. i'll find some time soon to take a look at make check and see how the command line version of binscope could integrate while we see if someone from releng wants to join the fun :)
This looks pretty simple from my brief read through of this bug and the microsoft link in comment 0.  As far as automation integration is concerned, there are three steps that need to be resolved.  

1. Get the dependencies/tools onto the build slaves
2. Run the tool
3. Parse the results

Note that 1 and 2,3 can be (and should be) done in parallel.

= Get the Binaries onto the Build Slaves =
* Figure out what the dependencies are in terms of Microsoft toolchain (as different from what we currently have on the build machines)
* File a bug to change the windows build reference platform w.r.t. the tool additions

= Running the tool =
RelEng prefers to have one simple way to run things - one script to call, for example.  There are options
* We have access to bash on windows (as provided by msys) and so you can provide RelEng with a bash script that executes the proper commands for binscope.  Alternatively, you could also write a python script that wraps the binscope exectuable and call it that way.  
* Once you have a "one-executable-method" to run the tool then we need to write the buildbot step that will run this script. 
* Or if you wire this in with your own C++ code, you could invoke that through a make target.

= Getting the results =
Buildbot sucks at result parsing.  I'd prefer to do the parsing of the results on the buildbot slave side - so ideally, however we run the tool outputs a "PASS" or "FAIL" status and provides us with a log.  It is then pretty easy to wire that reporting into buildbot.  (It uses PASS or FAIL to turn the build different colors, and the log can be appended to the buildlog).

Note that since we're talking about microsoft tool that (as I understand it) requires the microsoft VC toolchain, we will need to do this on *buildslaves* not *testslaves*.  This distinction is important once you start talking with RelEng.  So, keep that in mind.
from http://www.microsoft.com/download/en/details.aspx?id=11910 :

Supported Operating Systems: Windows 7, Windows Server 2008, Windows Vista

For Standalone version:

• Host system must be running the latest version of the Windows OS supported by your application

• .NET Framework 2.0 or later

• Windows Server 2003 SP2 – with manual modifications detailed in Instructions

• Windows Server 2008 (or higher) 

---

this implies the machines running this need to have Windows 7 on them. i'll look into what the build and test machines are running for Windows. 

binscope doesn't require visual studio to be installed. just the NET Framework 2.0 or higher, and apparently Windows 7

as far as an easy to parse etc script, i can definitely write up something that outputs PASS or FAIL and dumps a log of the rest of the info. i'll also look into if there's python or something like that on the build/test slaves as well as bash.
https://wiki.mozilla.org/ReferencePlatforms/Test/Win7

looks like we have a win 7 test reference platform
oh i see that clint said in c#9 that it can be a python script. i'll try to whip that up soon.
I thought that this was a change to the builder only, not test side.  What is the usecase to have all the test slaves that download the build from changeset X do this test?  Why not just do this test once on the builder when the build from changeset X is produced.

I think doing this on the tests is a waste of developer time in that it will make the end to end time longer, which is the opposite of what I want to be doing. :)

So, build reference platform for win32:
https://wiki.mozilla.org/ReferencePlatforms/Win32

Build reference platform for win64:
https://wiki.mozilla.org/ReferencePlatforms/Win64
(In reply to comment #13)
> I thought that this was a change to the builder only, not test side.  What
> is the usecase to have all the test slaves that download the build from
> changeset X do this test?  Why not just do this test once on the builder
> when the build from changeset X is produced.
> 
> I think doing this on the tests is a waste of developer time in that it will
> make the end to end time longer, which is the opposite of what I want to be
> doing. :)
> 
> So, build reference platform for win32:
> https://wiki.mozilla.org/ReferencePlatforms/Win32
> 
> Build reference platform for win64:
> https://wiki.mozilla.org/ReferencePlatforms/Win64

i am still learning how all our build/test stuff works :) yeah, doing it on the builder makes a lot more sense and catching it upstream from the test boxes. i definitely agree that we don't want to make the end to end time longer (and this change doesn't require doing so)
the win32 build is based off of win 2003 server sp2 which the binscope page says it supports, the win64 build build is win 2008 server so we should be good to go there. i'll see if binscope works 'out of the box' on w2k3 server sp2 and do the python script when i can.
(In reply to comment #15)
> the win32 build is based off of win 2003 server sp2 which the binscope page
> says it supports, the win64 build build is win 2008 server so we should be
> good to go there. i'll see if binscope works 'out of the box' on w2k3 server
> sp2 and do the python script when i can.

Awesome, sounds great.
ok, Windows Server 2003 SP2 fully patched still needs NET Framwork 3.5 apparently. i'll file a bug to get this on the W2K3 build servers once the script is ready to go.
both Windows 2003 and Windows 2008 are going to need "Debugging Tools for Windows" installed, or at least symchk.exe to download symbols, this is now a component of the Windows 7 SDK and can't be downloaded separately any more apparently :(
a fully patched Windows 2008 server doesn't need anything other than the Debugging Tools installed to run binscope (no .NET framework).
Attachment #548050 - Attachment description: python script to run all the binscope modules via integratation with the build → python script to run all the binscope modules via integration with the build
autobinscope.py produces output like :

C:\src>python autobinscope.py
Microsoft SDL BinScope binary analysis tool v1.0.4027.29711
PASS

(would be FAIL if one or more of the tests failed). all the paths and such are constants in the script. 

Debugging Tools/Windows SDK doesn't need to be installed anywhere, binscope is smart enough to understand a Windows symbol path pointing to the Mozilla symbol server. i'll file some followup bugs to start the other bits (NET Framework 3.5 on the Windows 2003 build machines, Binscope itself installed as well) moving.
You'll want to print 'TEST-UNEXPECTED-FAIL' to get the tinderbox log parser to pick it up.
Attached file v2 python script (obsolete) — 
Thanks Kyle ! changed to output "TEST-UNEXPECTED-FAIL" on a fail
Attachment #548050 - Attachment is obsolete: true
Just some notes:
a) The build slaves already have "Debugging Tools for Windows" installed, we use some binaries for there for our source server support
b) You shouldn't need to point at symbols.mozilla.org, the build machine has all the PDB files locally since it just built the binaries
c) The TARGET_BINARY should be passed on the commandline, since it's not going to be actually installed (it will be in a known location in the objdir)
d) You might need to change some of those other hardcoded paths, I'm not sure where the best place to put things on the build slave is
e) You should add some more information to the fail line so people looking at a build log can figure out exactly what failed, like "TEST-UNEXPECTED-FAIL | autobinscope | Binaries are not properly DEP/ASLR enabled"
(In reply to comment #24)
> Just some notes:
> a) The build slaves already have "Debugging Tools for Windows" installed, we
> use some binaries for there for our source server support

we don't need this any more, binscope knows how to do symbols without needing symchk.exe and friends from Debugging Tools :)

> b) You shouldn't need to point at symbols.mozilla.org, the build machine has
> all the PDB files locally since it just built the binaries

unfortunately i couldn't get binscope to work with passing in a local symbol dir, it seems to require a Windows Symbol SRV type path :/

> c) The TARGET_BINARY should be passed on the commandline, since it's not
> going to be actually installed (it will be in a known location in the objdir)

great point, i'll modify the script to do this. the other thing i thought of since yesterday is that we will also want to do the check on plugin-container as well as firefox.exe. i'll make the script expect to be passed paths to both
these binaries as its arguments

> d) You might need to change some of those other hardcoded paths, I'm not
> sure where the best place to put things on the build slave is

right, i was hoping for some tips from our RelEng friends on proper values here. 

> e) You should add some more information to the fail line so people looking
> at a build log can figure out exactly what failed, like
> "TEST-UNEXPECTED-FAIL | autobinscope | Binaries are not properly DEP/ASLR
> enabled"

another good point, i'll do this and also add a pointer to the log binscope itself writes out in the error message as well.

thanks very much for the feedback !
(In reply to comment #25)
> > b) You shouldn't need to point at symbols.mozilla.org, the build machine has
> > all the PDB files locally since it just built the binaries
> 
> unfortunately i couldn't get binscope to work with passing in a local symbol
> dir, it seems to require a Windows Symbol SRV type path :/

We'll need to rethink this then, as build machines are not allowed to access machines outside of the build network.
(In reply to comment #26)
> (In reply to comment #25)
> > > b) You shouldn't need to point at symbols.mozilla.org, the build machine has
> > > all the PDB files locally since it just built the binaries
> > 
> > unfortunately i couldn't get binscope to work with passing in a local symbol
> > dir, it seems to require a Windows Symbol SRV type path :/
> 
> We'll need to rethink this then, as build machines are not allowed to access
> machines outside of the build network.

the docs do claim that it works with a local symbol directory, i'll re-investigate. the restriction on the build machines going outside of the build network makes total sense to me.
ok binscope works just fine with using a local directory as the symbol directory. it also (thankfully) expects the pdbs to be in that directory in a flat structure (ie it doesn't require the Microsoft symbol hashing etc stuff). i'll upload a new version of the script incoporating this and the other feedback from Ted.
Depends on: 674250
Attached file script updated with feedback (obsolete) — 
updated the script with Ted's feedback, thanks !
Attachment #548052 - Attachment is obsolete: true
Assignee: nobody → imelven
Comment on attachment 548493 [details]
script updated with feedback

># autobinscope.py
># imelven@mozilla.com 7/26/11
># run Microsoft's Binscope tool (http://www.microsoft.com/download/en/details.aspx?id=11910)
># against a fresh build. output a 'binscope.log' file with full details
># of the run and appropriate strings to integrate with the buildbots
>
># from the docs : "The error code returned when running under the command line is equal 
># to the number of failures the tool reported plus the number of errors. BinScope will return 
># 0 only if there are no errors or failures."
>
>import sys
>import subprocess
>
>BINSCOPE_BINARY = "C:\\Program Files\\Microsoft\\SDL BinScope\\binscope.exe"

This is a sensible fallback, but it might be worthwhile looking for it in %PATH% first, to allow for alternate installation locations.

># CHANGEME : please point these to where you would like the binscope output logs to go 
>BINSCOPE_OUTPUT_LOGFILE_FIREFOX = "C:\\binscope_xml_output_firefox.log"
>BINSCOPE_OUTPUT_LOGFILE_PLUGIN_CONTAINER = "C:\\binscope_xml_output_plugin_container.log"

Can binscope.exe be output to stdout directly? Unless these logs are going to be uploaded somewhere, there's not much point in writing to them.

If we do need to write to logs, and if this is going to be wrapped by the build system, these should have an option to be overridden by a parameter or environment variable so that they can be outputted somewhere in the objdir.

>
># usage
>if len(sys.argv) < 3:
>  print "usage : autobinscope.by <path to firefox binary> <path to plugin-container binary <path to symbols>"
>  sys.exit(0)

It would be nice to be a bit more generic here and say "app_path" - other applications (Thunderbird, SeaMonkey) can probably make use of this, too. It might even be worthwhile dropping the plugin-container binary argument and having the program called twice. It looks like the binscope invocation is the same for both.
(In reply to comment #30)

thanks very much for the feedback. i made the log file path an optional parameter since it doesn't appear that i can make binscope write to standard out, look in the %PATH% for binscope first, falling back to the default install location and made the script more generic (implying the build system now needs to run it twice, as you suggested).
Attached file v4 python script (obsolete) — 
updated with feedback from bhearsum
Attachment #548493 - Attachment is obsolete: true
bhearsum has gotten binscope on to the build machines, can someone help with next steps to get calling the script into the build ?
What you'll need to do is figure out a place to put this in mozilla-central, then add a Makefile target to call it as part of the build (perhaps in a check:: rule, so that it gets run as part of "make check").
Depends on: 677797
how does /build/win32 sound as a place for autobinscope.py to live ?

i think i'm still going to need some releng help to find the appropriate Makefile's make check to stick the invocation of autobinscope.py in - it needs to be at the end of the Windows build when the binaries etc are all there - does this imply it needs to go in the top level Makefile ?
build/win32 seems fine for the location. I think what you want to do is hook this into a "check" target. "make check" gets run after the build is complete, this is the perfect time to run your test.

You can simply add a rule like:
ifeq (WINNT, $(OS_TARGET))
check::
    $(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/whatever.exe ...
endif

to the bottom of build/win32/Makefile.in.
(In reply to Ted Mielczarek [:ted, :luser] from comment #36)
> build/win32 seems fine for the location. I think what you want to do is hook
> this into a "check" target. "make check" gets run after the build is
> complete, this is the perfect time to run your test.
> 
> You can simply add a rule like:
> ifeq (WINNT, $(OS_TARGET))
> check::
>     $(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/whatever.exe ...
> endif
> 
> to the bottom of build/win32/Makefile.in.

awesome, thanks Ted, i'll give it a try and then try on Try :D
Summary: run binscope or some other tool to alert us and turn the tree red when we don't use ASLR and other protection tools → run binscope to alert us and turn the tree red when we don't use ASLR and other protection tools
Attached patch WIP patch (obsolete) — — Splinter Review 
WIP patch - i need to figure out where the symbols end up after a win32 build when i get back to somewhere i have a windows box. then i can try this locally and if it works do a win32 only try run to see it work there.
ted pointed me to 'make buildsymbols' which when run from the objdir copies the symbols (and compresses them) to dist/crashreporter-symbols

right now i'm having trouble with getting autobinscope.py to be able to find the binscope.exe from when i run it within the mozilla build environment, it keeps telling me file not found although the same path works fine for executing the binary outside of the python script. it's a little confusing because i'm not sure what style of path python is expecting when it's running inside mozilla build - neither a cygwin /c/Program Files/ etc. or c:\Program Files\ etc. path seems to be working...
Attached patch patch v2 (obsolete) — — Splinter Review 
cleaned up the script and got the default path stuff working in mozilla-build. ready for a Windows try run to see if it works there.
Attachment #567900 - Attachment is obsolete: true
Python is a normal Windows binary, so you need to give it real Windows-style paths. You'll either have to escape the backslashes or use raw strings, like:
"c:\\foo\\bar" or r"c:\foo\bar".

Also, you're going to want to accept the path to binscope.exe as an environment variable that RelEng will set in the buildbot environment, since normal developers won't have it on their machines. Your check test should exit gracefully if it's not set, so that "make check" succeeds on developer machines.
(In reply to Ted Mielczarek [:ted, :luser] from comment #41)
> Python is a normal Windows binary, so you need to give it real Windows-style
> paths. You'll either have to escape the backslashes or use raw strings, like:
> "c:\\foo\\bar" or r"c:\foo\bar".

yeah i finally got this going yesterday. 

> Also, you're going to want to accept the path to binscope.exe as an
> environment variable that RelEng will set in the buildbot environment, since
> normal developers won't have it on their machines. Your check test should
> exit gracefully if it's not set, so that "make check" succeeds on developer
> machines.

the script looks in $PATH for binscope before using the default location, should i check another env var for a specific binscope path too ? i'm fine with that.

i'll clean it up so it exits cleanly if binscope isn't found. my try run found an issue with exception syntax being different between 2.5 (which is what was on the try machine) and 2.6 (which is what's in mozilla build).
Attached patch patch v3 (obsolete) — — Splinter Review 
Ted, please let me know if it's enough for binscope to look in PATH for binscope and then fall back to the default or if you prefer a specific env variable (i would probably try the env variable location, PATH, and then the default if that's the case)

i realized i needed to call this for plugin-container as well in the make check and cleaned up a few other things. 

successful try run : https://tbpl.mozilla.org/?tree=Try&rev=8c75e96a943d

log looks like :

d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/try-w32/build/build/win32/autobinscope.py ../../dist/bin/firefox.exe ../../dist/crashreporter-symbols/
Microsoft SDL BinScope binary analysis tool v1.0.4027.29711
Could not locate binscope in PATH, trying default location of :
c:\Program Files\Microsoft\SDL BinScope\binscope.exe
PASS |autobinscope.py| ../../dist/bin/firefox.exe succeeded
d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/try-w32/build/build/win32/autobinscope.py ../../dist/bin/plugin-container.exe ../../dist/crashreporter-symbols/
Microsoft SDL BinScope binary analysis tool v1.0.4027.29711
Could not locate binscope in PATH, trying default location of :
c:\Program Files\Microsoft\SDL BinScope\binscope.exe
PASS |autobinscope.py| ../../dist/bin/plugin-container.exe succeeded
Attachment #549152 - Attachment is obsolete: true
Attachment #569245 - Attachment is obsolete: true
Attachment #569547 - Flags: review?(ted.mielczarek)
Comment on attachment 569547 [details] [diff] [review]
patch v3

Review of attachment 569547 [details] [diff] [review]:
-----------------------------------------------------------------

r- for a few things, but a lot of them are nitpicky.

::: build/win32/Makefile.in
@@ +111,5 @@
>  endif # WIN32_REDIST_DIR
> +
> +# run the binscope tool to make sure the binary and all libraries
> +# are using all available Windows OS-level security mechanisms
> +ifeq (WINNT, $(OS_TARGET))

This is redundant, since you're in build/win32, which only gets built on Windows.

@@ +114,5 @@
> +# are using all available Windows OS-level security mechanisms
> +ifeq (WINNT, $(OS_TARGET))
> +check::
> +	$(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/firefox.exe $(DIST)/crashreporter-symbols/
> +	$(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/plugin-container.exe $(DIST)/crashreporter-symbols/

Might be nice to switch this to take multiple .exe files on the command line. Maybe you could switch the script args to be like <symbol path> <exe> [<exe> ...]? Then you could get away with a single invocation.

::: build/win32/autobinscope.py
@@ +1,1 @@
> +# autobinscope.py

You want a real license header here. You can use the MPL-tri license: http://www.mozilla.org/MPL/boilerplate-1.1/ or something simpler (BSD/MIT/Public domain) since this is just for testing. Also, put a shebang line at the start? The canonical Python one is generally #!/usr/bin/env python

@@ +13,5 @@
> +
> +import sys
> +import subprocess
> +
> +#BINSCOPE_DEFAULT_PATH = "c:\\Program Files (x86)\\Microsoft\\SDL BinScope\\binscope.exe"

Don't leave stuff in but commented out, just remove it.

@@ +14,5 @@
> +import sys
> +import subprocess
> +
> +#BINSCOPE_DEFAULT_PATH = "c:\\Program Files (x86)\\Microsoft\\SDL BinScope\\binscope.exe"
> +BINSCOPE_DEFAULT_PATH = "c:\\Program Files\\Microsoft\\SDL BinScope\\binscope.exe"

FYI you can also do raw strings in Python like r"foo\bar", the slashes are treated as literals. Handy for writing out Windows paths.

@@ +21,5 @@
> +
> +# usage
> +if len(sys.argv) < 3:
> +  print "usage : autobinscope.by path_to_binary path_to_symbols [log_file_path]"
> +  print "log_file_path is optional, log will be written to .\binscope_xml_output.log by default"

You didn't escape the backslash here, FYI. Also, you could have done this as a triple-quoted string:
print """usage: ...
...."""

@@ +59,5 @@
> +        "/c", "ATLVersionCheck", "/c", "ATLVulnCheck", "/c", "FunctionPointersCheck",
> +        "/c", "SharedSectionCheck", "/c", "APTCACheck", "/c", "NXCheck",
> +        "/c", "GSCheck", "/c", "GSFunctionSafeBuffersCheck", "/c", "GSFriendlyInitCheck",
> +        "/c", "CompilerVersionCheck", "/c", "SafeSEHCheck", "/c", "SNCheck",
> +        "/c", "DBCheck"], stdout=subprocess.PIPE)

Can you put these args in a list up above and reuse that list instead of duplicating it? Also, given how this winds up looking, I think I'd prefer if you just required either:
a) BINSCOPE set in the environment to the path to the binary or
b) Installed in the default location. (Is this how it's installed on our build slaves?)

@@ +70,5 @@
> +        print "Could not locate binscope at default location of :\n" + BINSCOPE_DEFAULT_PATH
> +        print "Binscope wasn't installed, skipping this check and exiting..."
> +        sys.exit(0)
> +
> +proc1.wait

You're not actually calling a function here...

@@ +72,5 @@
> +        sys.exit(0)
> +
> +proc1.wait
> +
> +output = proc1.communicate()[0]

...but it works out okay because communicate calls wait internally.

@@ +75,5 @@
> +
> +output = proc1.communicate()[0]
> +
> +#print "\noutput from binscope is : " + output
> +#print "\n"

Again, don't leave commented code in.

@@ +82,5 @@
> +#print "\nretcode is " + str(proc1.returncode)
> +
> +# is this a PASS or a FAIL ? 
> +if proc1.returncode != 0:
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| " + binary_path + " is missing a needed Windows protection, such as /GS or ASLR"

I'd prefer if you used string formatting rather than concatenation, like;
print "TEST-UNEXPECTED FAIL | autobinscope.py | %s is missing .... " % binary_path

@@ +84,5 @@
> +# is this a PASS or a FAIL ? 
> +if proc1.returncode != 0:
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| " + binary_path + " is missing a needed Windows protection, such as /GS or ASLR"
> +else:
> +  print "PASS |autobinscope.py| " + binary_path + " succeeded"

Should be TEST-PASS.
Attachment #569547 - Flags: review?(ted.mielczarek) → review-
(In reply to Ted Mielczarek [:ted, :luser] from comment #44)
> Comment on attachment 569547 [details] [diff] [review] [diff] [details] [review]
> patch v3
>
> > +# run the binscope tool to make sure the binary and all libraries
> > +# are using all available Windows OS-level security mechanisms
> > +ifeq (WINNT, $(OS_TARGET))
> 
> This is redundant, since you're in build/win32, which only gets built on
> Windows.

fixed, deleted the ifeq

> Might be nice to switch this to take multiple .exe files on the command
> line. Maybe you could switch the script args to be like <symbol path> <exe>
> [<exe> ...]? Then you could get away with a single invocation.

this is the one change i didn't make - if you feel strongly about this let me know, i kind of like having one run of script -> one binscope invocation (for the binscope log file output being per run if nothing else)
 
> You want a real license header here. You can use the MPL-tri license:
> http://www.mozilla.org/MPL/boilerplate-1.1/ or something simpler
> (BSD/MIT/Public domain) since this is just for testing. Also, put a shebang
> line at the start? The canonical Python one is generally #!/usr/bin/env
> python

i used the MPL-tri license and added the shebang line as suggested
 
> > +#BINSCOPE_DEFAULT_PATH = "c:\\Program Files (x86)\\Microsoft\\SDL BinScope\\binscope.exe"
> 
> Don't leave stuff in but commented out, just remove it.

done !
 
> FYI you can also do raw strings in Python like r"foo\bar", the slashes are
> treated as literals. Handy for writing out Windows paths.

thanks, good to know :)
 
> > +  print "usage : autobinscope.by path_to_binary path_to_symbols [log_file_path]"
> > +  print "log_file_path is optional, log will be written to .\binscope_xml_output.log by default"
> 
> You didn't escape the backslash here, FYI. Also, you could have done this as
> a triple-quoted string:
> print """usage: ...
> ...."""

i switched to using """ here, thanks again for the tip
 
> Can you put these args in a list up above and reuse that list instead of
> duplicating it? Also, given how this winds up looking, I think I'd prefer if
> you just required either:
> a) BINSCOPE set in the environment to the path to the binary or
> b) Installed in the default location. (Is this how it's installed on our
> build slaves?)

the default location varies depending on if we're on 32 or 64 bit Windows so i changed the path to come from the BINSCOPE environment variable - i will file
a blocking bug to get this set up on the build slaves 

> > +proc1.wait
> 
> You're not actually calling a function here...

fixed !
 
> > +#print "\noutput from binscope is : " + output
> > +#print "\n"
> 
> Again, don't leave commented code in.

removed !
 
> I'd prefer if you used string formatting rather than concatenation, like;
> print "TEST-UNEXPECTED FAIL | autobinscope.py | %s is missing .... " %
> binary_path

fixed - another good Python tip :)

> > +  print "PASS |autobinscope.py| " + binary_path + " succeeded"
> 
> Should be TEST-PASS.

fixed !
Attached patch patch v4 (obsolete) — — Splinter Review 
Patch updated with review feedback as outlined above
Attachment #569547 - Attachment is obsolete: true
Attachment #572676 - Flags: review?(ted.mielczarek)
Depends on: 700513
Filed bug 700513 (need BINSCOPE env variable on windows build slaves pointing to dir binscope is installed in) as a dependency - when this is done and propagates, i will do a try run and if all looks good (pending review of course), this should be ready to go :)
Comment on attachment 572676 [details] [diff] [review]
patch v4

Review of attachment 572676 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with just a few nits/suggestions.

::: build/win32/autobinscope.py
@@ +72,5 @@
> +  
> +# execute binscope against the binary, using the BINSCOPE environment
> +# variable as the path where binscope.exe is located 
> +try:
> +  binscope_path = os.path.join(os.environ['BINSCOPE'], BINSCOPE_EXE)

Seems like you might as well just require %BINSCOPE% to be set to the full path to the .exe here.

@@ +74,5 @@
> +# variable as the path where binscope.exe is located 
> +try:
> +  binscope_path = os.path.join(os.environ['BINSCOPE'], BINSCOPE_EXE)
> +except KeyError:
> +  print "Failed when trying to get the value of the BINSCOPE environment variable, skipping this check"

I'd probably phrase this as "BINSCOPE environment variable is not set, can't check DEP/ASLR status."

@@ +78,5 @@
> +  print "Failed when trying to get the value of the BINSCOPE environment variable, skipping this check"
> +  sys.exit(0)
> +  
> +try:    
> +  proc1 = subprocess.Popen([binscope_path, "/target", binary_path,

You can probably just name this "proc" since you only do one invocation now.

@@ +101,5 @@
> +output = proc1.communicate()[0]
> +
> +# is this a PASS or a FAIL ? 
> +if proc1.returncode != 0:
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| %s is missing a needed Windows protection, such as /GS or ASLR" % binary_path

Nit: spaces around the | characters (here and below).

@@ +105,5 @@
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| %s is missing a needed Windows protection, such as /GS or ASLR" % binary_path
> +else:
> +  print "TEST-PASS |autobinscope.py| %s succeeded" % binary_path
> +
> +# we're done here.

This comment seems superfluous.
Attachment #572676 - Flags: review?(ted.mielczarek) → review+
note to self for when i do the last fix up prior to landing : in bug 700513 releng has BINSCOPE pointing to the actual .exe - so take this into consideration when making final review edits as above
Attached patch patch v5 — — Splinter Review 
(hopefully) final version, addressing ted's r+ comment - carrying ted's r+ over. will do a try run and mark checkin-needed if everything looks good there.
Attachment #572676 - Attachment is obsolete: true
Attachment #575298 - Flags: review+
Successful try run : https://tbpl.mozilla.org/?tree=Try&rev=340fff0b9d57

marking checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c3a085773624
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
In skimming my SeaMonkey make check log I noticed a reference to firefox.exe, so double checked and I should have caught this earlier.

Got r=khuey over IRC for the fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/069e4da44a5f
Oops, I missed that. Thanks for the fix!
The patch in this bug only made us run BinScope on firefox.exe and plugin-container.exe

Notably, we're not running BinScope against libEGL.dll and libGLESv2.dll (mentioned in comment 0) and we're not running BinScope against xul.dll

I've filed bug 1021214 for running BinScope against the rest of our DLLs and EXEs
Blocks: 1021214
Blocks: 1236356
See Also: → 1274844
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: