git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows
@ 2019-04-29 21:56 Johannes Schindelin via GitGitGadget
  2019-04-29 21:56 ` [PATCH 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-04-29 21:56 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

These two techniques make it harder to come up with exploits, by reducing
what is commonly called the "attack surface" in security circles: by making
the addresses less predictable, and by making it harder to inject data that
is then (mis-)interpreted as code, this hardens Git's executables on
Windows.

These patches have been carried in Git for Windows for over 3 years, and
should therefore be considered battle-tested.

İsmail Dönmez (2):
  mingw: do not let ld strip relocations
  mingw: enable DEP and ASLR

 config.mak.uname | 8 ++++++++
 1 file changed, 8 insertions(+)


base-commit: 39ffebd23b1ef6830bf86043ef0b5c069d9299a9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-134%2Fdscho%2Faslr-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-134/dscho/aslr-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/134
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/2] mingw: do not let ld strip relocations
  2019-04-29 21:56 [PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
@ 2019-04-29 21:56 ` İsmail Dönmez via GitGitGadget
  2019-04-29 21:56 ` [PATCH 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget
  2019-05-08 11:30 ` [PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
  2 siblings, 0 replies; 16+ messages in thread
From: İsmail Dönmez via GitGitGadget @ 2019-04-29 21:56 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, İsmail Dönmez

From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>

This is the first step for enabling ASLR (Address Space Layout
Randomization) support. We want to enable ASLR for better protection
against exploiting security holes in Git: it makes it harder to attack
software by making code addresses unpredictable.

The problem fixed by this commit is that `ld.exe` seems to be stripping
relocations which in turn will break ASLR support. We just make sure
it's not stripping the main executable entry.

Signed-off-by: İsmail Dönmez <ismail@i10z.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index b37fa8424c..e7c7d14e5f 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -573,10 +573,12 @@ else
 		ifeq (MINGW32,$(MSYSTEM))
 			prefix = /mingw32
 			HOST_CPU = i686
+			BASIC_LDFLAGS += -Wl,--pic-executable,-e,_mainCRTStartup
 		endif
 		ifeq (MINGW64,$(MSYSTEM))
 			prefix = /mingw64
 			HOST_CPU = x86_64
+			BASIC_LDFLAGS += -Wl,--pic-executable,-e,mainCRTStartup
 		else
 			COMPAT_CFLAGS += -D_USE_32BIT_TIME_T
 			BASIC_LDFLAGS += -Wl,--large-address-aware
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] mingw: enable DEP and ASLR
  2019-04-29 21:56 [PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
  2019-04-29 21:56 ` [PATCH 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
@ 2019-04-29 21:56 ` İsmail Dönmez via GitGitGadget
  2019-04-30  6:26   ` Johannes Sixt
  2019-05-08 11:30 ` [PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
  2 siblings, 1 reply; 16+ messages in thread
From: İsmail Dönmez via GitGitGadget @ 2019-04-29 21:56 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, İsmail Dönmez

From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>

Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout
Randomization) support. This applies to both 32bit and 64bit builds
and makes it substantially harder to exploit security holes in Git by
offering a much more unpredictable attack surface.

ASLR interferes with GDB's ability to set breakpoints. A similar issue
holds true when compiling with -O2 (in which case single-stepping is
messed up because GDB cannot map the code back to the original source
code properly). Therefore we simply enable ASLR only when an
optimization flag is present in the CFLAGS, using it as an indicator
that the developer does not want to debug in GDB anyway.

Signed-off-by: İsmail Dönmez <ismail@i10z.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index e7c7d14e5f..a9edcc5f0b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -570,6 +570,12 @@ else
 	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
 		# MSys2
 		prefix = /usr/
+		# Enable DEP
+		BASIC_LDFLAGS += -Wl,--nxcompat
+		# Enable ASLR (unless debugging)
+		ifneq (,$(findstring -O,$(CFLAGS)))
+			BASIC_LDFLAGS += -Wl,--dynamicbase
+		endif
 		ifeq (MINGW32,$(MSYSTEM))
 			prefix = /mingw32
 			HOST_CPU = i686
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-04-29 21:56 ` [PATCH 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget
@ 2019-04-30  6:26   ` Johannes Sixt
  2019-04-30 22:41     ` Johannes Schindelin
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Sixt @ 2019-04-30  6:26 UTC (permalink / raw)
  To: İsmail Dönmez, Johannes Schindelin
  Cc: İsmail Dönmez via GitGitGadget, git, Junio C Hamano

[had to add Dscho as recipient manually, mind you]

Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:
> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>
> 
> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout
> Randomization) support. This applies to both 32bit and 64bit builds
> and makes it substantially harder to exploit security holes in Git by
> offering a much more unpredictable attack surface.
> 
> ASLR interferes with GDB's ability to set breakpoints. A similar issue
> holds true when compiling with -O2 (in which case single-stepping is
> messed up because GDB cannot map the code back to the original source
> code properly). Therefore we simply enable ASLR only when an
> optimization flag is present in the CFLAGS, using it as an indicator
> that the developer does not want to debug in GDB anyway.
> 
> Signed-off-by: İsmail Dönmez <ismail@i10z.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  config.mak.uname | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/config.mak.uname b/config.mak.uname
> index e7c7d14e5f..a9edcc5f0b 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -570,6 +570,12 @@ else
>  	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
>  		# MSys2
>  		prefix = /usr/
> +		# Enable DEP
> +		BASIC_LDFLAGS += -Wl,--nxcompat
> +		# Enable ASLR (unless debugging)
> +		ifneq (,$(findstring -O,$(CFLAGS)))
> +			BASIC_LDFLAGS += -Wl,--dynamicbase
> +		endif
>  		ifeq (MINGW32,$(MSYSTEM))
>  			prefix = /mingw32
>  			HOST_CPU = i686
> 

I'm a bit concerned that this breaks my debug sessions where I use -O0.
But I'll test without -O0 before I really complain.

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-04-30  6:26   ` Johannes Sixt
@ 2019-04-30 22:41     ` Johannes Schindelin
  2019-04-30 22:59       ` Johannes Sixt
                         ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Johannes Schindelin @ 2019-04-30 22:41 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

Hi Hannes,

On Tue, 30 Apr 2019, Johannes Sixt wrote:

> [had to add Dscho as recipient manually, mind you]

I usually pick up responses to GitGitGadget patch series even if I am not
on explicit Cc: (but it might take a couple of days when I am too busy
elsewhere to read the Git mailing list).

> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:
> > From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>
> >
> > Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout
> > Randomization) support. This applies to both 32bit and 64bit builds
> > and makes it substantially harder to exploit security holes in Git by
> > offering a much more unpredictable attack surface.
> >
> > ASLR interferes with GDB's ability to set breakpoints. A similar issue
> > holds true when compiling with -O2 (in which case single-stepping is
> > messed up because GDB cannot map the code back to the original source
> > code properly). Therefore we simply enable ASLR only when an
> > optimization flag is present in the CFLAGS, using it as an indicator
> > that the developer does not want to debug in GDB anyway.
> >
> > Signed-off-by: İsmail Dönmez <ismail@i10z.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  config.mak.uname | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/config.mak.uname b/config.mak.uname
> > index e7c7d14e5f..a9edcc5f0b 100644
> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -570,6 +570,12 @@ else
> >  	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
> >  		# MSys2
> >  		prefix = /usr/
> > +		# Enable DEP
> > +		BASIC_LDFLAGS += -Wl,--nxcompat
> > +		# Enable ASLR (unless debugging)
> > +		ifneq (,$(findstring -O,$(CFLAGS)))
> > +			BASIC_LDFLAGS += -Wl,--dynamicbase
> > +		endif
> >  		ifeq (MINGW32,$(MSYSTEM))
> >  			prefix = /mingw32
> >  			HOST_CPU = i686
> >
>
> I'm a bit concerned that this breaks my debug sessions where I use -O0.
> But I'll test without -O0 before I really complain.

Weird. Jameson Miller also mentioned this very concern in an internal
review.

I guess I'll do something like

	ifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))

Does that work for you?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-04-30 22:41     ` Johannes Schindelin
@ 2019-04-30 22:59       ` Johannes Sixt
  2019-05-01 18:39       ` Alban Gruin
  2019-05-01 20:46       ` Jeff King
  2 siblings, 0 replies; 16+ messages in thread
From: Johannes Sixt @ 2019-04-30 22:59 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

Am 01.05.19 um 00:41 schrieb Johannes Schindelin:
> On Tue, 30 Apr 2019, Johannes Sixt wrote:
>> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:
>>> diff --git a/config.mak.uname b/config.mak.uname
>>> index e7c7d14e5f..a9edcc5f0b 100644
>>> --- a/config.mak.uname
>>> +++ b/config.mak.uname
>>> @@ -570,6 +570,12 @@ else
>>>  	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
>>>  		# MSys2
>>>  		prefix = /usr/
>>> +		# Enable DEP
>>> +		BASIC_LDFLAGS += -Wl,--nxcompat
>>> +		# Enable ASLR (unless debugging)
>>> +		ifneq (,$(findstring -O,$(CFLAGS)))
>>> +			BASIC_LDFLAGS += -Wl,--dynamicbase
>>> +		endif
>>>  		ifeq (MINGW32,$(MSYSTEM))
>>>  			prefix = /mingw32
>>>  			HOST_CPU = i686
>>>
>>
>> I'm a bit concerned that this breaks my debug sessions where I use -O0.
>> But I'll test without -O0 before I really complain.
> 
> Weird. Jameson Miller also mentioned this very concern in an internal
> review.
> 
> I guess I'll do something like
> 
> 	ifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))
> 
> Does that work for you?

That could work. I'm a bit distracted at the moment, so it may take some
time until I can test.

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-04-30 22:41     ` Johannes Schindelin
  2019-04-30 22:59       ` Johannes Sixt
@ 2019-05-01 18:39       ` Alban Gruin
  2019-05-01 23:36         ` brian m. carlson
  2019-05-08 11:33         ` Johannes Schindelin
  2019-05-01 20:46       ` Jeff King
  2 siblings, 2 replies; 16+ messages in thread
From: Alban Gruin @ 2019-05-01 18:39 UTC (permalink / raw)
  To: Johannes Schindelin, Johannes Sixt
  Cc: İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

Hi Johannes,

Le 01/05/2019 à 00:41, Johannes Schindelin a écrit :
> Hi Hannes,
> 
> On Tue, 30 Apr 2019, Johannes Sixt wrote:
> 
>> [had to add Dscho as recipient manually, mind you]
> 
> I usually pick up responses to GitGitGadget patch series even if I am not
> on explicit Cc: (but it might take a couple of days when I am too busy
> elsewhere to read the Git mailing list).
> 
>> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:
>>> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>
>>>
>>> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout
>>> Randomization) support. This applies to both 32bit and 64bit builds
>>> and makes it substantially harder to exploit security holes in Git by
>>> offering a much more unpredictable attack surface.
>>>
>>> ASLR interferes with GDB's ability to set breakpoints. A similar issue
>>> holds true when compiling with -O2 (in which case single-stepping is
>>> messed up because GDB cannot map the code back to the original source
>>> code properly). Therefore we simply enable ASLR only when an

I don’t know if it stands true when combined with something like -ggdb3,
but I may be very wrong.  Feel free to correct me.

>>> optimization flag is present in the CFLAGS, using it as an indicator
>>> that the developer does not want to debug in GDB anyway.
>>>
>>> Signed-off-by: İsmail Dönmez <ismail@i10z.com>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>>  config.mak.uname | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/config.mak.uname b/config.mak.uname
>>> index e7c7d14e5f..a9edcc5f0b 100644
>>> --- a/config.mak.uname
>>> +++ b/config.mak.uname
>>> @@ -570,6 +570,12 @@ else
>>>  	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
>>>  		# MSys2
>>>  		prefix = /usr/
>>> +		# Enable DEP
>>> +		BASIC_LDFLAGS += -Wl,--nxcompat
>>> +		# Enable ASLR (unless debugging)
>>> +		ifneq (,$(findstring -O,$(CFLAGS)))
>>> +			BASIC_LDFLAGS += -Wl,--dynamicbase
>>> +		endif
>>>  		ifeq (MINGW32,$(MSYSTEM))
>>>  			prefix = /mingw32
>>>  			HOST_CPU = i686
>>>
>>
>> I'm a bit concerned that this breaks my debug sessions where I use -O0.
>> But I'll test without -O0 before I really complain.
> 
> Weird. Jameson Miller also mentioned this very concern in an internal
> review.
> 
> I guess I'll do something like
> 
> 	ifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))
> 

-Og also exists to debug[0], even if it’s far less known.  Perhaps it’s
better to check for -g (and its variants[1]) as the user clearly states
their intent to debug the resulting binary, rather than checking for
special cases.

> Does that work for you?
> 
> Ciao,
> Dscho
> 

[0] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-Og
[1] https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html

Cheers,
Alban


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-04-30 22:41     ` Johannes Schindelin
  2019-04-30 22:59       ` Johannes Sixt
  2019-05-01 18:39       ` Alban Gruin
@ 2019-05-01 20:46       ` Jeff King
  2019-05-01 22:02         ` Jonathan Nieder
  2 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2019-05-01 20:46 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Jonathan Nieder, Johannes Sixt, İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

On Tue, Apr 30, 2019 at 06:41:29PM -0400, Johannes Schindelin wrote:

> > I'm a bit concerned that this breaks my debug sessions where I use -O0.
> > But I'll test without -O0 before I really complain.
> 
> Weird. Jameson Miller also mentioned this very concern in an internal
> review.
> 
> I guess I'll do something like
> 
> 	ifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))
> 
> Does that work for you?

I wonder if this points to this patch touching the wrong level. These
compiler flags are a thing that _some_ builds want (i.e., production
builds where people care most about security and not about debugging),
but not necessarily all.

I'd have expected this to be tweakable by a Makefile knob (either a
specific knob, or just the caller setting the right CFLAGS etc), and
then for the builds of Git for Windows to turn those knobs when making a
package to distribute.

Our internal package builds at GitHub all have this in their config.mak
(for Linux, of course):

  CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
  CFLAGS += -fstack-protector-strong

  CFLAGS += -fpie
  LDFLAGS += -z relro -z now
  LDFLAGS += -pie

and I wouldn't be surprised if other binary distributors (like the
Debian package) do something similar.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-05-01 20:46       ` Jeff King
@ 2019-05-01 22:02         ` Jonathan Nieder
  2019-05-08 11:27           ` Johannes Schindelin
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2019-05-01 22:02 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin, Johannes Sixt, İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

Hi,

Jeff King wrote:

> I wonder if this points to this patch touching the wrong level. These
> compiler flags are a thing that _some_ builds want (i.e., production
> builds where people care most about security and not about debugging),
> but not necessarily all.
>
> I'd have expected this to be tweakable by a Makefile knob (either a
> specific knob, or just the caller setting the right CFLAGS etc), and
> then for the builds of Git for Windows to turn those knobs when making a
> package to distribute.
>
> Our internal package builds at GitHub all have this in their config.mak
> (for Linux, of course):
>
>   CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
>   CFLAGS += -fstack-protector-strong
>
>   CFLAGS += -fpie
>   LDFLAGS += -z relro -z now
>   LDFLAGS += -pie
>
> and I wouldn't be surprised if other binary distributors (like the
> Debian package) do something similar.

Yes, the Debian package uses

	CFLAGS := -Wall \
		$(shell dpkg-buildflags --get CFLAGS) \
		$(shell dpkg-buildflags --get CPPFLAGS)

and then passes CFLAGS='$(CFLAGS)' to "make".

That means we're using

	-g -O2 -fstack-protector-strong -Wformat -Werror=format-security
	-Wdate-time -D_FORTIFY_SOURCE=2

Dscho's suggestion for the Windows build sounds fine to me (if
checking for -Og, too).  Maybe it would make sense to factor out a
makefile variable for this, that could be used for builds on other
platforms, too.  That way, the autodetection can be in one place, and
there is a standard way to override it when the user wants something
else.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-05-01 18:39       ` Alban Gruin
@ 2019-05-01 23:36         ` brian m. carlson
  2019-05-08 11:33           ` Johannes Schindelin
  2019-05-08 11:33         ` Johannes Schindelin
  1 sibling, 1 reply; 16+ messages in thread
From: brian m. carlson @ 2019-05-01 23:36 UTC (permalink / raw)
  To: Alban Gruin
  Cc: Johannes Schindelin, Johannes Sixt, İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Wed, May 01, 2019 at 08:39:22PM +0200, Alban Gruin wrote:
> -Og also exists to debug[0], even if it’s far less known.  Perhaps it’s
> better to check for -g (and its variants[1]) as the user clearly states
> their intent to debug the resulting binary, rather than checking for
> special cases.

I can't speak for the Windows folks, but Debian frequently builds with
-O2 -g and strips the debugging symbols into a separate package that can
be installed in case of a crash. So -g need not be an indication of
non-production use.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-05-01 22:02         ` Jonathan Nieder
@ 2019-05-08 11:27           ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2019-05-08 11:27 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Jeff King, Johannes Sixt, İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

Hi Jonathan & Peff,

On Wed, 1 May 2019, Jonathan Nieder wrote:

> Jeff King wrote:
>
> > I wonder if this points to this patch touching the wrong level. These
> > compiler flags are a thing that _some_ builds want (i.e., production
> > builds where people care most about security and not about debugging),
> > but not necessarily all.
> >
> > I'd have expected this to be tweakable by a Makefile knob (either a
> > specific knob, or just the caller setting the right CFLAGS etc), and
> > then for the builds of Git for Windows to turn those knobs when making a
> > package to distribute.
> >
> > Our internal package builds at GitHub all have this in their config.mak
> > (for Linux, of course):
> >
> >   CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
> >   CFLAGS += -fstack-protector-strong
> >
> >   CFLAGS += -fpie
> >   LDFLAGS += -z relro -z now
> >   LDFLAGS += -pie
> >
> > and I wouldn't be surprised if other binary distributors (like the
> > Debian package) do something similar.
>
> Yes, the Debian package uses
>
> 	CFLAGS := -Wall \
> 		$(shell dpkg-buildflags --get CFLAGS) \
> 		$(shell dpkg-buildflags --get CPPFLAGS)
>
> and then passes CFLAGS='$(CFLAGS)' to "make".
>
> That means we're using
>
> 	-g -O2 -fstack-protector-strong -Wformat -Werror=format-security
> 	-Wdate-time -D_FORTIFY_SOURCE=2
>
> Dscho's suggestion for the Windows build sounds fine to me (if
> checking for -Og, too).  Maybe it would make sense to factor out a
> makefile variable for this, that could be used for builds on other
> platforms, too.  That way, the autodetection can be in one place, and
> there is a standard way to override it when the user wants something
> else.

Indeed, if I was to add a generic "are we building for production?"
function, this would be incorrect.

But this is not the case here, we are doing something very specific,
Windows-only here, and for the sole reason to keep debuggability (for
which the presence of the `-g` option indeed would not be a good
indicator: in Git for Windows, we build `.pdb` files so that stackdumps
can be more meaningful, but we do not want to have full debug information
in those executables).

In the long run, I think we need to become more explicit about this, by
adding a "FOR_PRODUCTION" flag. It's really no good if we use
implementation details such as CFLAGS to deduce intent.

That's for another patch series, though, as it is pretty clear-cut here:
If you build with optimization flags using Git for Windows' SDK, you
cannot use gdb for single-stepping, likewise if you use ASLR, so we can
totally piggyback the latter onto the former.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows
  2019-04-29 21:56 [PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
  2019-04-29 21:56 ` [PATCH 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
  2019-04-29 21:56 ` [PATCH 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget
@ 2019-05-08 11:30 ` Johannes Schindelin via GitGitGadget
  2019-05-08 11:30   ` [PATCH v2 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
  2019-05-08 11:30   ` [PATCH v2 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget
  2 siblings, 2 replies; 16+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-05-08 11:30 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

These two techniques make it harder to come up with exploits, by reducing
what is commonly called the "attack surface" in security circles: by making
the addresses less predictable, and by making it harder to inject data that
is then (mis-)interpreted as code, this hardens Git's executables on
Windows.

These patches have been carried in Git for Windows for over 3 years, and
should therefore be considered battle-tested.

Changes since v1:

 * When determining whether we build with optimization, -O0 and -Og are
   explicitly ignored.

İsmail Dönmez (2):
  mingw: do not let ld strip relocations
  mingw: enable DEP and ASLR

 config.mak.uname | 8 ++++++++
 1 file changed, 8 insertions(+)


base-commit: 83232e38648b51abbcbdb56c94632b6906cc85a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-134%2Fdscho%2Faslr-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-134/dscho/aslr-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/134

Range-diff vs v1:

 1:  e6acdba586 = 1:  828913e96c mingw: do not let ld strip relocations
 2:  e142c1396e ! 2:  9f1da73829 mingw: enable DEP and ASLR
     @@ -21,13 +21,13 @@
       --- a/config.mak.uname
       +++ b/config.mak.uname
      @@
     - 	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
     + 	ifneq ($(shell expr "$(uname_R)" : '1\.'),2)
       		# MSys2
       		prefix = /usr/
      +		# Enable DEP
      +		BASIC_LDFLAGS += -Wl,--nxcompat
      +		# Enable ASLR (unless debugging)
     -+		ifneq (,$(findstring -O,$(CFLAGS)))
     ++		ifneq (,$(findstring -O,$(filter-out -O0 -Og,$(CFLAGS))))
      +			BASIC_LDFLAGS += -Wl,--dynamicbase
      +		endif
       		ifeq (MINGW32,$(MSYSTEM))

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/2] mingw: do not let ld strip relocations
  2019-05-08 11:30 ` [PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
@ 2019-05-08 11:30   ` İsmail Dönmez via GitGitGadget
  2019-05-08 11:30   ` [PATCH v2 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget
  1 sibling, 0 replies; 16+ messages in thread
From: İsmail Dönmez via GitGitGadget @ 2019-05-08 11:30 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, İsmail Dönmez

From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>

This is the first step for enabling ASLR (Address Space Layout
Randomization) support. We want to enable ASLR for better protection
against exploiting security holes in Git: it makes it harder to attack
software by making code addresses unpredictable.

The problem fixed by this commit is that `ld.exe` seems to be stripping
relocations which in turn will break ASLR support. We just make sure
it's not stripping the main executable entry.

Signed-off-by: İsmail Dönmez <ismail@i10z.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index 3605fead53..f2ac755753 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -576,10 +576,12 @@ else
 		ifeq (MINGW32,$(MSYSTEM))
 			prefix = /mingw32
 			HOST_CPU = i686
+			BASIC_LDFLAGS += -Wl,--pic-executable,-e,_mainCRTStartup
 		endif
 		ifeq (MINGW64,$(MSYSTEM))
 			prefix = /mingw64
 			HOST_CPU = x86_64
+			BASIC_LDFLAGS += -Wl,--pic-executable,-e,mainCRTStartup
 		else
 			COMPAT_CFLAGS += -D_USE_32BIT_TIME_T
 			BASIC_LDFLAGS += -Wl,--large-address-aware
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 2/2] mingw: enable DEP and ASLR
  2019-05-08 11:30 ` [PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
  2019-05-08 11:30   ` [PATCH v2 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
@ 2019-05-08 11:30   ` İsmail Dönmez via GitGitGadget
  1 sibling, 0 replies; 16+ messages in thread
From: İsmail Dönmez via GitGitGadget @ 2019-05-08 11:30 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, İsmail Dönmez

From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>

Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout
Randomization) support. This applies to both 32bit and 64bit builds
and makes it substantially harder to exploit security holes in Git by
offering a much more unpredictable attack surface.

ASLR interferes with GDB's ability to set breakpoints. A similar issue
holds true when compiling with -O2 (in which case single-stepping is
messed up because GDB cannot map the code back to the original source
code properly). Therefore we simply enable ASLR only when an
optimization flag is present in the CFLAGS, using it as an indicator
that the developer does not want to debug in GDB anyway.

Signed-off-by: İsmail Dönmez <ismail@i10z.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index f2ac755753..ea0df3fe1b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -573,6 +573,12 @@ else
 	ifneq ($(shell expr "$(uname_R)" : '1\.'),2)
 		# MSys2
 		prefix = /usr/
+		# Enable DEP
+		BASIC_LDFLAGS += -Wl,--nxcompat
+		# Enable ASLR (unless debugging)
+		ifneq (,$(findstring -O,$(filter-out -O0 -Og,$(CFLAGS))))
+			BASIC_LDFLAGS += -Wl,--dynamicbase
+		endif
 		ifeq (MINGW32,$(MSYSTEM))
 			prefix = /mingw32
 			HOST_CPU = i686
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-05-01 18:39       ` Alban Gruin
  2019-05-01 23:36         ` brian m. carlson
@ 2019-05-08 11:33         ` Johannes Schindelin
  1 sibling, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2019-05-08 11:33 UTC (permalink / raw)
  To: Alban Gruin
  Cc: Johannes Sixt, İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]

Hi Alban,

On Wed, 1 May 2019, Alban Gruin wrote:

> Le 01/05/2019 à 00:41, Johannes Schindelin a écrit :
> >
> > On Tue, 30 Apr 2019, Johannes Sixt wrote:
> >
> >> [had to add Dscho as recipient manually, mind you]
> >
> > I usually pick up responses to GitGitGadget patch series even if I am not
> > on explicit Cc: (but it might take a couple of days when I am too busy
> > elsewhere to read the Git mailing list).
> >
> >> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:
> >>> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>
> >>>
> >>> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout
> >>> Randomization) support. This applies to both 32bit and 64bit builds
> >>> and makes it substantially harder to exploit security holes in Git by
> >>> offering a much more unpredictable attack surface.
> >>>
> >>> ASLR interferes with GDB's ability to set breakpoints. A similar issue
> >>> holds true when compiling with -O2 (in which case single-stepping is
> >>> messed up because GDB cannot map the code back to the original source
> >>> code properly). Therefore we simply enable ASLR only when an
>
> I don’t know if it stands true when combined with something like -ggdb3,
> but I may be very wrong.  Feel free to correct me.

Possibly, but that makes my job here harder, so I won't even try right now
;-)

> >>> optimization flag is present in the CFLAGS, using it as an indicator
> >>> that the developer does not want to debug in GDB anyway.
> >>>
> >>> Signed-off-by: İsmail Dönmez <ismail@i10z.com>
> >>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >>> ---
> >>>  config.mak.uname | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/config.mak.uname b/config.mak.uname
> >>> index e7c7d14e5f..a9edcc5f0b 100644
> >>> --- a/config.mak.uname
> >>> +++ b/config.mak.uname
> >>> @@ -570,6 +570,12 @@ else
> >>>  	ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
> >>>  		# MSys2
> >>>  		prefix = /usr/
> >>> +		# Enable DEP
> >>> +		BASIC_LDFLAGS += -Wl,--nxcompat
> >>> +		# Enable ASLR (unless debugging)
> >>> +		ifneq (,$(findstring -O,$(CFLAGS)))
> >>> +			BASIC_LDFLAGS += -Wl,--dynamicbase
> >>> +		endif
> >>>  		ifeq (MINGW32,$(MSYSTEM))
> >>>  			prefix = /mingw32
> >>>  			HOST_CPU = i686
> >>>
> >>
> >> I'm a bit concerned that this breaks my debug sessions where I use -O0.
> >> But I'll test without -O0 before I really complain.
> >
> > Weird. Jameson Miller also mentioned this very concern in an internal
> > review.
> >
> > I guess I'll do something like
> >
> > 	ifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))
> >
>
> -Og also exists to debug[0], even if it’s far less known.

Good point.

> Perhaps it’s better to check for -g (and its variants[1]) as the user
> clearly states their intent to debug the resulting binary, rather than
> checking for special cases.

I don't think we can use that, as we specifically build Git for Windows
with optimization *and* with debug symbols (and then use cv2pdb to extract
those debug symbols into external .pdb files for use with advanced
post-mortem tools, i.e. we do *not* need to single-step).

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] mingw: enable DEP and ASLR
  2019-05-01 23:36         ` brian m. carlson
@ 2019-05-08 11:33           ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2019-05-08 11:33 UTC (permalink / raw)
  To: brian m. carlson
  Cc: Alban Gruin, Johannes Sixt, İsmail Dönmez,
	İsmail Dönmez via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

Hi Brian,

On Wed, 1 May 2019, brian m. carlson wrote:

> On Wed, May 01, 2019 at 08:39:22PM +0200, Alban Gruin wrote:
> > -Og also exists to debug[0], even if it’s far less known.  Perhaps it’s
> > better to check for -g (and its variants[1]) as the user clearly states
> > their intent to debug the resulting binary, rather than checking for
> > special cases.
>
> I can't speak for the Windows folks, but Debian frequently builds with
> -O2 -g and strips the debugging symbols into a separate package that can
> be installed in case of a crash. So -g need not be an indication of
> non-production use.

Precisely, Git for Windows imitates this strategy.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-05-08 11:34 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-29 21:56 [PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
2019-04-29 21:56 ` [PATCH 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
2019-04-29 21:56 ` [PATCH 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget
2019-04-30  6:26   ` Johannes Sixt
2019-04-30 22:41     ` Johannes Schindelin
2019-04-30 22:59       ` Johannes Sixt
2019-05-01 18:39       ` Alban Gruin
2019-05-01 23:36         ` brian m. carlson
2019-05-08 11:33           ` Johannes Schindelin
2019-05-08 11:33         ` Johannes Schindelin
2019-05-01 20:46       ` Jeff King
2019-05-01 22:02         ` Jonathan Nieder
2019-05-08 11:27           ` Johannes Schindelin
2019-05-08 11:30 ` [PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows Johannes Schindelin via GitGitGadget
2019-05-08 11:30   ` [PATCH v2 1/2] mingw: do not let ld strip relocations İsmail Dönmez via GitGitGadget
2019-05-08 11:30   ` [PATCH v2 2/2] mingw: enable DEP and ASLR İsmail Dönmez via GitGitGadget

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).