devtools: Add security-check.py #6854

Merged
merged 1 commit into from Oct 22, 2015

Conversation

Projects
None yet
7 participants
@laanwj
Member

laanwj commented Oct 19, 2015

Performs the following ELF security checks:

  • PIE: Check for position independent executable (PIE), allowing for address space randomization
  • NX: Check that no sections are writable and executable (including the stack)
  • RELRO: Check for read-only relocations, binding at startup
  • Canary: Check for use of stack canary

Also add a check to symbol-check.py that checks that only the subset of allowed libraries is imported (to avoid incompatibilities).

This needs to be integrated into Travis, or at least the gitian build so that we won't do releases that don't pass these checks. We also need similar checks for OSX and Windows at some point.

@laanwj laanwj added the Build system label Oct 19, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 19, 2015

Member

Nice. Concept ACK.

ASLR/PIE check on OSX can be done with otool -hv <file>. Not sure how we would execute otools in gitian. NX checks can be done over a vmmap check. For Travis we should create a native osx build (which could run otools to check the executable).

Member

jonasschnelli commented Oct 19, 2015

Nice. Concept ACK.

ASLR/PIE check on OSX can be done with otool -hv <file>. Not sure how we would execute otools in gitian. NX checks can be done over a vmmap check. For Travis we should create a native osx build (which could run otools to check the executable).

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Oct 19, 2015

Contributor

Nice! So testing this would be:
Recompile with --disable-hardening (or subsets of those flags) and making sure security-check.py complains?

Contributor

gavinandresen commented Oct 19, 2015

Nice! So testing this would be:
Recompile with --disable-hardening (or subsets of those flags) and making sure security-check.py complains?

@LongShao007

This comment has been minimized.

Show comment
Hide comment
@LongShao007

LongShao007 Oct 19, 2015

Contributor

How to check security for Windows ?

Contributor

LongShao007 commented Oct 19, 2015

How to check security for Windows ?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 19, 2015

Member

@gavinandresen Right, to speed up testing you could use any C file:

cat >  test1.c << EOF
#include <stdio.h>

int main()
{
    printf("the quick brown fox jumps over the lazy god\n");
    return 0;
}
EOF
gcc test1.c -o test1 -Wl,-zexecstack;
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1 -fstack-protector-all
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1 -fstack-protector-all -pie -fPIE
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1 -fstack-protector-all -pie -fPIE -Wl,-zrelro  -Wl,-z,now
contrib/devtools/security-check.py test1; echo $?

Output:

test1: failed PIE NX RELRO Canary
1
test1: failed PIE RELRO Canary
1
test1: failed PIE RELRO
1
test1: failed RELRO
1
0

@LongShao007 yes, how? (readelf doesn't, but objdump -x does in fact work for PE files, but I wouldn't know what flags to look for)

Member

laanwj commented Oct 19, 2015

@gavinandresen Right, to speed up testing you could use any C file:

cat >  test1.c << EOF
#include <stdio.h>

int main()
{
    printf("the quick brown fox jumps over the lazy god\n");
    return 0;
}
EOF
gcc test1.c -o test1 -Wl,-zexecstack;
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1 -fstack-protector-all
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1 -fstack-protector-all -pie -fPIE
contrib/devtools/security-check.py test1; echo $?
gcc test1.c -o test1 -fstack-protector-all -pie -fPIE -Wl,-zrelro  -Wl,-z,now
contrib/devtools/security-check.py test1; echo $?

Output:

test1: failed PIE NX RELRO Canary
1
test1: failed PIE RELRO Canary
1
test1: failed PIE RELRO
1
test1: failed RELRO
1
0

@LongShao007 yes, how? (readelf doesn't, but objdump -x does in fact work for PE files, but I wouldn't know what flags to look for)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 19, 2015

Member

On windows:

NX: DllCharacteristics bit 0x100 signifies nxcompat (DEP):

-DllCharacteristics     00000100
+DllCharacteristics     00000000

PIE: DllCharacteristics bit 0x40 signifies dynamicbase (ASLR):

+DllCharacteristics     00000000
-DllCharacteristics     00000040

Not sure whether there is a RELRO equivalent, Canary is there but difficult to check from the outside as the mingw library is linked statically.

Edit: Ok, added the windows checks above for PE executables, and converted the above test to a test script test-security-check.py.

Member

laanwj commented Oct 19, 2015

On windows:

NX: DllCharacteristics bit 0x100 signifies nxcompat (DEP):

-DllCharacteristics     00000100
+DllCharacteristics     00000000

PIE: DllCharacteristics bit 0x40 signifies dynamicbase (ASLR):

+DllCharacteristics     00000000
-DllCharacteristics     00000040

Not sure whether there is a RELRO equivalent, Canary is there but difficult to check from the outside as the mingw library is linked statically.

Edit: Ok, added the windows checks above for PE executables, and converted the above test to a test script test-security-check.py.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 21, 2015

Member

@theuni paths to tools like READELF, OBJDUMP can now be overridden

Member

laanwj commented Oct 21, 2015

@theuni paths to tools like READELF, OBJDUMP can now be overridden

@MarcoFalke

View changes

contrib/devtools/security-check.py
+ try:
+ etype = identify_executable(filename)
+ if etype is None:
+ print('%s: unknown format')

This comment has been minimized.

@MarcoFalke

MarcoFalke Oct 21, 2015

Member

Nit: orphan %s.

@MarcoFalke

MarcoFalke Oct 21, 2015

Member

Nit: orphan %s.

This comment has been minimized.

@laanwj

laanwj Oct 21, 2015

Member

Good catch. Fixed

@laanwj

laanwj Oct 21, 2015

Member

Good catch. Fixed

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 21, 2015

Contributor

concept ACK

Contributor

dcousens commented Oct 21, 2015

concept ACK

@theuni

View changes

contrib/devtools/security-check.py
+ '''
+ have_gnu_relro = False
+ for (typ, flags) in get_ELF_program_headers(executable):
+ if typ == 'GNU_RELRO' and flags == 'R':

This comment has been minimized.

@theuni

theuni Oct 21, 2015

Member

Interestingly on my machine, whether or not this shows up as R or RW seems to depend on the linker. I'm afraid I don't know enough to determine if having this section writable is a security implication.

$ g++ ... -o bitcoin-cli -fuse-ld=bfd
$ readelf -l -W bitcoin-cli | grep RELRO
  GNU_RELRO      0x243e20 0x0000000000443e20 0x0000000000443e20 0x0201e0 0x0201e0 R   0x1
$ g++ ... -o bitcoin-cli -fuse-ld=gold
$ readelf -l -W bitcoin-cli | grep RELRO
  GNU_RELRO      0x244e20 0x0000000000245e20 0x0000000000245e20 0x0201e0 0x0201e0 RW  0x20
@theuni

theuni Oct 21, 2015

Member

Interestingly on my machine, whether or not this shows up as R or RW seems to depend on the linker. I'm afraid I don't know enough to determine if having this section writable is a security implication.

$ g++ ... -o bitcoin-cli -fuse-ld=bfd
$ readelf -l -W bitcoin-cli | grep RELRO
  GNU_RELRO      0x243e20 0x0000000000443e20 0x0000000000443e20 0x0201e0 0x0201e0 R   0x1
$ g++ ... -o bitcoin-cli -fuse-ld=gold
$ readelf -l -W bitcoin-cli | grep RELRO
  GNU_RELRO      0x244e20 0x0000000000245e20 0x0000000000245e20 0x0201e0 0x0201e0 RW  0x20

This comment has been minimized.

@laanwj

laanwj Oct 22, 2015

Member

One important point of RELRO in combination with BIND_NOW is that the area can be read-only, as that part contains function pointers. Common heap overflow exploits allow overwriting a word, these frequently invoked function pointers make good targets.

  LOAD           0x000000 0x0000000000400000 0x0000000000400000 0x0007d4 0x0007d4 R E 0x200000
   02     .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame 

  LOAD           0x000db8 0x0000000000600db8 0x0000000000600db8 0x000258 0x000260 RW  0x200000
   03     .init_array .fini_array .jcr .dynamic .got .data .bss 
...
  GNU_RELRO      0x000db8 0x0000000000600db8 0x0000000000600db8 0x000248 0x000248 R   0x1
   08     .init_array .fini_array .jcr .dynamic .got 

However, all the sections are part of the second LOAD section which are what actually maps memory and determine the permissions, and those are RW, also with my linker. I don't think the flags on GNU_RELRO have any influence.

It looks like the executable itself takes care of mprotecting this area at start (glibc _dl_protect_relro):

mprotect(0x600000, 4096, PROT_READ)     = 0

I experimentally verified that the section is unwritable during runtime, both when linked with bfd and with gold.
This all makes sense, as the dynamic linker does need to write to it to set the pointers in the first place...

@laanwj

laanwj Oct 22, 2015

Member

One important point of RELRO in combination with BIND_NOW is that the area can be read-only, as that part contains function pointers. Common heap overflow exploits allow overwriting a word, these frequently invoked function pointers make good targets.

  LOAD           0x000000 0x0000000000400000 0x0000000000400000 0x0007d4 0x0007d4 R E 0x200000
   02     .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame 

  LOAD           0x000db8 0x0000000000600db8 0x0000000000600db8 0x000258 0x000260 RW  0x200000
   03     .init_array .fini_array .jcr .dynamic .got .data .bss 
...
  GNU_RELRO      0x000db8 0x0000000000600db8 0x0000000000600db8 0x000248 0x000248 R   0x1
   08     .init_array .fini_array .jcr .dynamic .got 

However, all the sections are part of the second LOAD section which are what actually maps memory and determine the permissions, and those are RW, also with my linker. I don't think the flags on GNU_RELRO have any influence.

It looks like the executable itself takes care of mprotecting this area at start (glibc _dl_protect_relro):

mprotect(0x600000, 4096, PROT_READ)     = 0

I experimentally verified that the section is unwritable during runtime, both when linked with bfd and with gold.
This all makes sense, as the dynamic linker does need to write to it to set the pointers in the first place...

This comment has been minimized.

@laanwj

laanwj Oct 22, 2015

Member

See also this thread: http://permalink.gmane.org/gmane.comp.gnu.binutils/71347
Will remove the check - although I agree with Alan that having it as 'R' is clearer, as that is the eventual intention.

@laanwj

laanwj Oct 22, 2015

Member

See also this thread: http://permalink.gmane.org/gmane.comp.gnu.binutils/71347
Will remove the check - although I agree with Alan that having it as 'R' is clearer, as that is the eventual intention.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Oct 21, 2015

Member

Looks good other than the comment above. Something to note:
The libevent brought in a dependency on clock_gettime, which is a glibc compat issue. Once we bump to a newer gcc, we'll have to work that out for releases.

As discussed on IRC, this should also be hooked up to a make target for easier testing w/ travis and gitian. That can be done as a next step.

Member

theuni commented Oct 21, 2015

Looks good other than the comment above. Something to note:
The libevent brought in a dependency on clock_gettime, which is a glibc compat issue. Once we bump to a newer gcc, we'll have to work that out for releases.

As discussed on IRC, this should also be hooked up to a make target for easier testing w/ travis and gitian. That can be done as a next step.

devtools: Add security-check.py
Perform the following ELF security checks:

- PIE: Check for position independent executable (PIE), allowing for address space randomization
- NX: Check that no sections are writable and executable (including the stack)
- RELRO: Check for read-only relocations, binding at startup
- Canary: Check for use of stack canary

Also add a check to symbol-check.py that checks that only the subset of
allowed libraries is imported (to avoid incompatibilities).

@laanwj laanwj merged commit 579b863 into bitcoin:master Oct 22, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 22, 2015

Merge pull request #6854
579b863 devtools: Add security-check.py (Wladimir J. van der Laan)

str4d added a commit to str4d/zcash that referenced this pull request Oct 15, 2016

Remaining changes from bitcoin/bitcoin#6854
Add a check to symbol-check.py that checks that only the subset of
allowed libraries is imported (to avoid incompatibilities).

See 56734f4 for the earlier changes.

@str4d str4d referenced this pull request in zcash/zcash Oct 15, 2016

Merged

Upstream gitian updates #1541

zkbot pushed a commit to zcash/zcash that referenced this pull request Oct 17, 2016

zkbot
Auto merge of #1541 - str4d:upstream-gitian-updates, r=bitcartel
Upstream gitian updates

This PR pulls in all gitian-related PRs that have been merged upstream since 0.11.2. The only ones I left out were documentation-only PRs, because we removed `doc/gitian-building.md` at some point. Here are the commits applied here, in the order shown in `git log` (ie. last to first):

- bitcoin/bitcoin#7283
  - fa42a67
  - fa58c76
- bitcoin/bitcoin#8175
  - 74c1347
- bitcoin/bitcoin#8167
  - 7e7eb27
  - ad38204
  - b676f38
- bitcoin/bitcoin#7776
  - f063863
- bitcoin/bitcoin#7424
  - a81c87f ~ we already partly applied
  - a8ce872
  - f3d3eaf ~ we already partly applied
  - 475813b
  - ~~cd27bf5~~ X we already applied
- bitcoin/bitcoin#7060
  - 3b468a0 ~ we removed doc/gitian-building.md
  - ~~99fda26~~ X we removed doc/gitian-building.md
- bitcoin/bitcoin#7251
  - fa09562
- bitcoin/bitcoin#6900
  - ~~2cecb24~~ X we removed doc/gitian-building.md
  - 957c0fd
  - 2e31d74
  - ~~0b416c6~~ X we removed QT
  - 9f251b7
- bitcoin/bitcoin#6854
  - 579b863 ~ we already partly applied

Part of #540
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment