{"thread":{"id":"51008","subject":"[PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows","startedAt":"2019-04-29T21:57:00Z","lastAt":"2019-05-08T11:34:10Z","messageCount":16,"participants":["Johannes Schindelin via GitGitGadget","İsmail Dönmez via GitGitGadget","Johannes Sixt","Johannes Schindelin","Alban Gruin","Jeff King","Jonathan Nieder","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"374670","messageId":"pull.134.git.gitgitgadget@gmail.com","threadId":"51008","inReplyTo":null,"subject":"[PATCH 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T21:56:56Z","receivedAt":"2019-04-29T21:57:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These two techniques make it harder to come up with exploits, by reducing\nwhat is commonly called the \"attack surface\" in security circles: by making\nthe addresses less predictable, and by making it harder to inject data that\nis then (mis-)interpreted as code, this hardens Git's executables on\nWindows.\n\nThese patches have been carried in Git for Windows for over 3 years, and\nshould therefore be considered battle-tested.\n\nİsmail Dönmez (2):\n  mingw: do not let ld strip relocations\n  mingw: enable DEP and ASLR\n\n config.mak.uname | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\n\nbase-commit: 39ffebd23b1ef6830bf86043ef0b5c069d9299a9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-134%2Fdscho%2Faslr-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-134/dscho/aslr-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/134\n-- \ngitgitgadget\n"},{"id":"374671","messageId":"e142c1396ec3541486317819e885cf42be24af34.1556575015.git.gitgitgadget@gmail.com","threadId":"51008","inReplyTo":"pull.134.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] mingw: enable DEP and ASLR","fromName":"İsmail Dönmez via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T21:56:58Z","receivedAt":"2019-04-29T21:57:03Z","isPatch":true,"sender":{"key":"ismail@pardus.org.tr","avatar":null},"body":"From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n\nEnable DEP (Data Execution Prevention) and ASLR (Address Space Layout\nRandomization) support. This applies to both 32bit and 64bit builds\nand makes it substantially harder to exploit security holes in Git by\noffering a much more unpredictable attack surface.\n\nASLR interferes with GDB's ability to set breakpoints. A similar issue\nholds true when compiling with -O2 (in which case single-stepping is\nmessed up because GDB cannot map the code back to the original source\ncode properly). Therefore we simply enable ASLR only when an\noptimization flag is present in the CFLAGS, using it as an indicator\nthat the developer does not want to debug in GDB anyway.\n\nSigned-off-by: İsmail Dönmez <ismail@i10z.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n config.mak.uname | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex e7c7d14e5f..a9edcc5f0b 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -570,6 +570,12 @@ else\n \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n \t\t# MSys2\n \t\tprefix = /usr/\n+\t\t# Enable DEP\n+\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n+\t\t# Enable ASLR (unless debugging)\n+\t\tifneq (,$(findstring -O,$(CFLAGS)))\n+\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n+\t\tendif\n \t\tifeq (MINGW32,$(MSYSTEM))\n \t\t\tprefix = /mingw32\n \t\t\tHOST_CPU = i686\n-- \ngitgitgadget\n"},{"id":"374672","messageId":"e6acdba58659d6176a03037aeb63b0ba84e126ff.1556575015.git.gitgitgadget@gmail.com","threadId":"51008","inReplyTo":"pull.134.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] mingw: do not let ld strip relocations","fromName":"İsmail Dönmez via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T21:56:57Z","receivedAt":"2019-04-29T21:57:04Z","isPatch":true,"sender":{"key":"ismail@pardus.org.tr","avatar":null},"body":"From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n\nThis is the first step for enabling ASLR (Address Space Layout\nRandomization) support. We want to enable ASLR for better protection\nagainst exploiting security holes in Git: it makes it harder to attack\nsoftware by making code addresses unpredictable.\n\nThe problem fixed by this commit is that `ld.exe` seems to be stripping\nrelocations which in turn will break ASLR support. We just make sure\nit's not stripping the main executable entry.\n\nSigned-off-by: İsmail Dönmez <ismail@i10z.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n config.mak.uname | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b37fa8424c..e7c7d14e5f 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -573,10 +573,12 @@ else\n \t\tifeq (MINGW32,$(MSYSTEM))\n \t\t\tprefix = /mingw32\n \t\t\tHOST_CPU = i686\n+\t\t\tBASIC_LDFLAGS += -Wl,--pic-executable,-e,_mainCRTStartup\n \t\tendif\n \t\tifeq (MINGW64,$(MSYSTEM))\n \t\t\tprefix = /mingw64\n \t\t\tHOST_CPU = x86_64\n+\t\t\tBASIC_LDFLAGS += -Wl,--pic-executable,-e,mainCRTStartup\n \t\telse\n \t\t\tCOMPAT_CFLAGS += -D_USE_32BIT_TIME_T\n \t\t\tBASIC_LDFLAGS += -Wl,--large-address-aware\n-- \ngitgitgadget\n\n"},{"id":"374695","messageId":"8e59dbf6-a339-74f3-4e60-e56b3817aea5@kdbg.org","threadId":"51008","inReplyTo":"e142c1396ec3541486317819e885cf42be24af34.1556575015.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-04-30T06:26:26Z","receivedAt":"2019-04-30T06:26:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"[had to add Dscho as recipient manually, mind you]\n\nAm 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:\n> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n> \n> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout\n> Randomization) support. This applies to both 32bit and 64bit builds\n> and makes it substantially harder to exploit security holes in Git by\n> offering a much more unpredictable attack surface.\n> \n> ASLR interferes with GDB's ability to set breakpoints. A similar issue\n> holds true when compiling with -O2 (in which case single-stepping is\n> messed up because GDB cannot map the code back to the original source\n> code properly). Therefore we simply enable ASLR only when an\n> optimization flag is present in the CFLAGS, using it as an indicator\n> that the developer does not want to debug in GDB anyway.\n> \n> Signed-off-by: İsmail Dönmez <ismail@i10z.com>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  config.mak.uname | 6 ++++++\n>  1 file changed, 6 insertions(+)\n> \n> diff --git a/config.mak.uname b/config.mak.uname\n> index e7c7d14e5f..a9edcc5f0b 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -570,6 +570,12 @@ else\n>  \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n>  \t\t# MSys2\n>  \t\tprefix = /usr/\n> +\t\t# Enable DEP\n> +\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n> +\t\t# Enable ASLR (unless debugging)\n> +\t\tifneq (,$(findstring -O,$(CFLAGS)))\n> +\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n> +\t\tendif\n>  \t\tifeq (MINGW32,$(MSYSTEM))\n>  \t\t\tprefix = /mingw32\n>  \t\t\tHOST_CPU = i686\n> \n\nI'm a bit concerned that this breaks my debug sessions where I use -O0.\nBut I'll test without -O0 before I really complain.\n\n-- Hannes\n"},{"id":"374729","messageId":"nycvar.QRO.7.76.6.1904301838400.45@tvgsbejvaqbjf.bet","threadId":"51008","inReplyTo":"8e59dbf6-a339-74f3-4e60-e56b3817aea5@kdbg.org","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-30T22:41:29Z","receivedAt":"2019-04-30T22:41:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 30 Apr 2019, Johannes Sixt wrote:\n\n> [had to add Dscho as recipient manually, mind you]\n\nI usually pick up responses to GitGitGadget patch series even if I am not\non explicit Cc: (but it might take a couple of days when I am too busy\nelsewhere to read the Git mailing list).\n\n> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:\n> > From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n> >\n> > Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout\n> > Randomization) support. This applies to both 32bit and 64bit builds\n> > and makes it substantially harder to exploit security holes in Git by\n> > offering a much more unpredictable attack surface.\n> >\n> > ASLR interferes with GDB's ability to set breakpoints. A similar issue\n> > holds true when compiling with -O2 (in which case single-stepping is\n> > messed up because GDB cannot map the code back to the original source\n> > code properly). Therefore we simply enable ASLR only when an\n> > optimization flag is present in the CFLAGS, using it as an indicator\n> > that the developer does not want to debug in GDB anyway.\n> >\n> > Signed-off-by: İsmail Dönmez <ismail@i10z.com>\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >  config.mak.uname | 6 ++++++\n> >  1 file changed, 6 insertions(+)\n> >\n> > diff --git a/config.mak.uname b/config.mak.uname\n> > index e7c7d14e5f..a9edcc5f0b 100644\n> > --- a/config.mak.uname\n> > +++ b/config.mak.uname\n> > @@ -570,6 +570,12 @@ else\n> >  \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n> >  \t\t# MSys2\n> >  \t\tprefix = /usr/\n> > +\t\t# Enable DEP\n> > +\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n> > +\t\t# Enable ASLR (unless debugging)\n> > +\t\tifneq (,$(findstring -O,$(CFLAGS)))\n> > +\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n> > +\t\tendif\n> >  \t\tifeq (MINGW32,$(MSYSTEM))\n> >  \t\t\tprefix = /mingw32\n> >  \t\t\tHOST_CPU = i686\n> >\n>\n> I'm a bit concerned that this breaks my debug sessions where I use -O0.\n> But I'll test without -O0 before I really complain.\n\nWeird. Jameson Miller also mentioned this very concern in an internal\nreview.\n\nI guess I'll do something like\n\n\tifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))\n\nDoes that work for you?\n\nCiao,\nDscho\n"},{"id":"374733","messageId":"9ec02c05-daf9-ffe3-ef64-2c550e29f5b7@kdbg.org","threadId":"51008","inReplyTo":"nycvar.QRO.7.76.6.1904301838400.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-04-30T22:59:40Z","receivedAt":"2019-04-30T22:59:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.05.19 um 00:41 schrieb Johannes Schindelin:\n> On Tue, 30 Apr 2019, Johannes Sixt wrote:\n>> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:\n>>> diff --git a/config.mak.uname b/config.mak.uname\n>>> index e7c7d14e5f..a9edcc5f0b 100644\n>>> --- a/config.mak.uname\n>>> +++ b/config.mak.uname\n>>> @@ -570,6 +570,12 @@ else\n>>>  \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n>>>  \t\t# MSys2\n>>>  \t\tprefix = /usr/\n>>> +\t\t# Enable DEP\n>>> +\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n>>> +\t\t# Enable ASLR (unless debugging)\n>>> +\t\tifneq (,$(findstring -O,$(CFLAGS)))\n>>> +\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n>>> +\t\tendif\n>>>  \t\tifeq (MINGW32,$(MSYSTEM))\n>>>  \t\t\tprefix = /mingw32\n>>>  \t\t\tHOST_CPU = i686\n>>>\n>>\n>> I'm a bit concerned that this breaks my debug sessions where I use -O0.\n>> But I'll test without -O0 before I really complain.\n> \n> Weird. Jameson Miller also mentioned this very concern in an internal\n> review.\n> \n> I guess I'll do something like\n> \n> \tifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))\n> \n> Does that work for you?\n\nThat could work. I'm a bit distracted at the moment, so it may take some\ntime until I can test.\n\n-- Hannes\n"},{"id":"374776","messageId":"2e7be484-74d7-7258-954e-3a4a34a36c01@gmail.com","threadId":"51008","inReplyTo":"nycvar.QRO.7.76.6.1904301838400.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-05-01T18:39:22Z","receivedAt":"2019-05-01T18:39:35Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes,\n\nLe 01/05/2019 à 00:41, Johannes Schindelin a écrit :\n> Hi Hannes,\n> \n> On Tue, 30 Apr 2019, Johannes Sixt wrote:\n> \n>> [had to add Dscho as recipient manually, mind you]\n> \n> I usually pick up responses to GitGitGadget patch series even if I am not\n> on explicit Cc: (but it might take a couple of days when I am too busy\n> elsewhere to read the Git mailing list).\n> \n>> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:\n>>> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n>>>\n>>> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout\n>>> Randomization) support. This applies to both 32bit and 64bit builds\n>>> and makes it substantially harder to exploit security holes in Git by\n>>> offering a much more unpredictable attack surface.\n>>>\n>>> ASLR interferes with GDB's ability to set breakpoints. A similar issue\n>>> holds true when compiling with -O2 (in which case single-stepping is\n>>> messed up because GDB cannot map the code back to the original source\n>>> code properly). Therefore we simply enable ASLR only when an\n\nI don’t know if it stands true when combined with something like -ggdb3,\nbut I may be very wrong.  Feel free to correct me.\n\n>>> optimization flag is present in the CFLAGS, using it as an indicator\n>>> that the developer does not want to debug in GDB anyway.\n>>>\n>>> Signed-off-by: İsmail Dönmez <ismail@i10z.com>\n>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>>> ---\n>>>  config.mak.uname | 6 ++++++\n>>>  1 file changed, 6 insertions(+)\n>>>\n>>> diff --git a/config.mak.uname b/config.mak.uname\n>>> index e7c7d14e5f..a9edcc5f0b 100644\n>>> --- a/config.mak.uname\n>>> +++ b/config.mak.uname\n>>> @@ -570,6 +570,12 @@ else\n>>>  \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n>>>  \t\t# MSys2\n>>>  \t\tprefix = /usr/\n>>> +\t\t# Enable DEP\n>>> +\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n>>> +\t\t# Enable ASLR (unless debugging)\n>>> +\t\tifneq (,$(findstring -O,$(CFLAGS)))\n>>> +\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n>>> +\t\tendif\n>>>  \t\tifeq (MINGW32,$(MSYSTEM))\n>>>  \t\t\tprefix = /mingw32\n>>>  \t\t\tHOST_CPU = i686\n>>>\n>>\n>> I'm a bit concerned that this breaks my debug sessions where I use -O0.\n>> But I'll test without -O0 before I really complain.\n> \n> Weird. Jameson Miller also mentioned this very concern in an internal\n> review.\n> \n> I guess I'll do something like\n> \n> \tifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))\n> \n\n-Og also exists to debug[0], even if it’s far less known.  Perhaps it’s\nbetter to check for -g (and its variants[1]) as the user clearly states\ntheir intent to debug the resulting binary, rather than checking for\nspecial cases.\n\n> Does that work for you?\n> \n> Ciao,\n> Dscho\n> \n\n[0] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-Og\n[1] https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html\n\nCheers,\nAlban\n\n"},{"id":"374787","messageId":"20190501204631.GB13372@sigill.intra.peff.net","threadId":"51008","inReplyTo":"nycvar.QRO.7.76.6.1904301838400.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-01T20:46:31Z","receivedAt":"2019-05-01T20:46:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2019 at 06:41:29PM -0400, Johannes Schindelin wrote:\n\n> > I'm a bit concerned that this breaks my debug sessions where I use -O0.\n> > But I'll test without -O0 before I really complain.\n> \n> Weird. Jameson Miller also mentioned this very concern in an internal\n> review.\n> \n> I guess I'll do something like\n> \n> \tifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))\n> \n> Does that work for you?\n\nI wonder if this points to this patch touching the wrong level. These\ncompiler flags are a thing that _some_ builds want (i.e., production\nbuilds where people care most about security and not about debugging),\nbut not necessarily all.\n\nI'd have expected this to be tweakable by a Makefile knob (either a\nspecific knob, or just the caller setting the right CFLAGS etc), and\nthen for the builds of Git for Windows to turn those knobs when making a\npackage to distribute.\n\nOur internal package builds at GitHub all have this in their config.mak\n(for Linux, of course):\n\n  CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1\n  CFLAGS += -fstack-protector-strong\n\n  CFLAGS += -fpie\n  LDFLAGS += -z relro -z now\n  LDFLAGS += -pie\n\nand I wouldn't be surprised if other binary distributors (like the\nDebian package) do something similar.\n\n-Peff\n"},{"id":"374792","messageId":"20190501220219.GA42435@google.com","threadId":"51008","inReplyTo":"20190501204631.GB13372@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-01T22:02:19Z","receivedAt":"2019-05-01T22:02:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> I wonder if this points to this patch touching the wrong level. These\n> compiler flags are a thing that _some_ builds want (i.e., production\n> builds where people care most about security and not about debugging),\n> but not necessarily all.\n>\n> I'd have expected this to be tweakable by a Makefile knob (either a\n> specific knob, or just the caller setting the right CFLAGS etc), and\n> then for the builds of Git for Windows to turn those knobs when making a\n> package to distribute.\n>\n> Our internal package builds at GitHub all have this in their config.mak\n> (for Linux, of course):\n>\n>   CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1\n>   CFLAGS += -fstack-protector-strong\n>\n>   CFLAGS += -fpie\n>   LDFLAGS += -z relro -z now\n>   LDFLAGS += -pie\n>\n> and I wouldn't be surprised if other binary distributors (like the\n> Debian package) do something similar.\n\nYes, the Debian package uses\n\n\tCFLAGS := -Wall \\\n\t\t$(shell dpkg-buildflags --get CFLAGS) \\\n\t\t$(shell dpkg-buildflags --get CPPFLAGS)\n\nand then passes CFLAGS='$(CFLAGS)' to \"make\".\n\nThat means we're using\n\n\t-g -O2 -fstack-protector-strong -Wformat -Werror=format-security\n\t-Wdate-time -D_FORTIFY_SOURCE=2\n\nDscho's suggestion for the Windows build sounds fine to me (if\nchecking for -Og, too).  Maybe it would make sense to factor out a\nmakefile variable for this, that could be used for builds on other\nplatforms, too.  That way, the autodetection can be in one place, and\nthere is a standard way to override it when the user wants something\nelse.\n\nThanks,\nJonathan\n"},{"id":"374797","messageId":"20190501233641.GC202237@genre.crustytoothpaste.net","threadId":"51008","inReplyTo":"2e7be484-74d7-7258-954e-3a4a34a36c01@gmail.com","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-01T23:36:41Z","receivedAt":"2019-05-01T23:36:51Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, May 01, 2019 at 08:39:22PM +0200, Alban Gruin wrote:\n> -Og also exists to debug[0], even if it’s far less known.  Perhaps it’s\n> better to check for -g (and its variants[1]) as the user clearly states\n> their intent to debug the resulting binary, rather than checking for\n> special cases.\n\nI can't speak for the Windows folks, but Debian frequently builds with\n-O2 -g and strips the debugging symbols into a separate package that can\nbe installed in case of a crash. So -g need not be an indication of\nnon-production use.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375132","messageId":"nycvar.QRO.7.76.6.1905081319570.44@tvgsbejvaqbjf.bet","threadId":"51008","inReplyTo":"20190501220219.GA42435@google.com","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-08T11:27:08Z","receivedAt":"2019-05-08T11:27:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan & Peff,\n\nOn Wed, 1 May 2019, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n>\n> > I wonder if this points to this patch touching the wrong level. These\n> > compiler flags are a thing that _some_ builds want (i.e., production\n> > builds where people care most about security and not about debugging),\n> > but not necessarily all.\n> >\n> > I'd have expected this to be tweakable by a Makefile knob (either a\n> > specific knob, or just the caller setting the right CFLAGS etc), and\n> > then for the builds of Git for Windows to turn those knobs when making a\n> > package to distribute.\n> >\n> > Our internal package builds at GitHub all have this in their config.mak\n> > (for Linux, of course):\n> >\n> >   CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1\n> >   CFLAGS += -fstack-protector-strong\n> >\n> >   CFLAGS += -fpie\n> >   LDFLAGS += -z relro -z now\n> >   LDFLAGS += -pie\n> >\n> > and I wouldn't be surprised if other binary distributors (like the\n> > Debian package) do something similar.\n>\n> Yes, the Debian package uses\n>\n> \tCFLAGS := -Wall \\\n> \t\t$(shell dpkg-buildflags --get CFLAGS) \\\n> \t\t$(shell dpkg-buildflags --get CPPFLAGS)\n>\n> and then passes CFLAGS='$(CFLAGS)' to \"make\".\n>\n> That means we're using\n>\n> \t-g -O2 -fstack-protector-strong -Wformat -Werror=format-security\n> \t-Wdate-time -D_FORTIFY_SOURCE=2\n>\n> Dscho's suggestion for the Windows build sounds fine to me (if\n> checking for -Og, too).  Maybe it would make sense to factor out a\n> makefile variable for this, that could be used for builds on other\n> platforms, too.  That way, the autodetection can be in one place, and\n> there is a standard way to override it when the user wants something\n> else.\n\nIndeed, if I was to add a generic \"are we building for production?\"\nfunction, this would be incorrect.\n\nBut this is not the case here, we are doing something very specific,\nWindows-only here, and for the sole reason to keep debuggability (for\nwhich the presence of the `-g` option indeed would not be a good\nindicator: in Git for Windows, we build `.pdb` files so that stackdumps\ncan be more meaningful, but we do not want to have full debug information\nin those executables).\n\nIn the long run, I think we need to become more explicit about this, by\nadding a \"FOR_PRODUCTION\" flag. It's really no good if we use\nimplementation details such as CFLAGS to deduce intent.\n\nThat's for another patch series, though, as it is pretty clear-cut here:\nIf you build with optimization flags using Git for Windows' SDK, you\ncannot use gdb for single-stepping, likewise if you use ASLR, so we can\ntotally piggyback the latter onto the former.\n\nCiao,\nDscho\n"},{"id":"375133","messageId":"pull.134.v2.git.gitgitgadget@gmail.com","threadId":"51008","inReplyTo":"pull.134.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Enable Data Execution Protection and Address Space Layout Randomization on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-08T11:30:57Z","receivedAt":"2019-05-08T11:31:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These two techniques make it harder to come up with exploits, by reducing\nwhat is commonly called the \"attack surface\" in security circles: by making\nthe addresses less predictable, and by making it harder to inject data that\nis then (mis-)interpreted as code, this hardens Git's executables on\nWindows.\n\nThese patches have been carried in Git for Windows for over 3 years, and\nshould therefore be considered battle-tested.\n\nChanges since v1:\n\n * When determining whether we build with optimization, -O0 and -Og are\n   explicitly ignored.\n\nİsmail Dönmez (2):\n  mingw: do not let ld strip relocations\n  mingw: enable DEP and ASLR\n\n config.mak.uname | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\n\nbase-commit: 83232e38648b51abbcbdb56c94632b6906cc85a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-134%2Fdscho%2Faslr-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-134/dscho/aslr-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/134\n\nRange-diff vs v1:\n\n 1:  e6acdba586 = 1:  828913e96c mingw: do not let ld strip relocations\n 2:  e142c1396e ! 2:  9f1da73829 mingw: enable DEP and ASLR\n     @@ -21,13 +21,13 @@\n       --- a/config.mak.uname\n       +++ b/config.mak.uname\n      @@\n     - \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n     + \tifneq ($(shell expr \"$(uname_R)\" : '1\\.'),2)\n       \t\t# MSys2\n       \t\tprefix = /usr/\n      +\t\t# Enable DEP\n      +\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n      +\t\t# Enable ASLR (unless debugging)\n     -+\t\tifneq (,$(findstring -O,$(CFLAGS)))\n     ++\t\tifneq (,$(findstring -O,$(filter-out -O0 -Og,$(CFLAGS))))\n      +\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n      +\t\tendif\n       \t\tifeq (MINGW32,$(MSYSTEM))\n\n-- \ngitgitgadget\n"},{"id":"375134","messageId":"9f1da73829c2275f6f65f7a6e385426aa7afa6c5.1557315057.git.gitgitgadget@gmail.com","threadId":"51008","inReplyTo":"pull.134.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] mingw: enable DEP and ASLR","fromName":"İsmail Dönmez via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-08T11:30:59Z","receivedAt":"2019-05-08T11:31:04Z","isPatch":true,"sender":{"key":"ismail@pardus.org.tr","avatar":null},"body":"From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n\nEnable DEP (Data Execution Prevention) and ASLR (Address Space Layout\nRandomization) support. This applies to both 32bit and 64bit builds\nand makes it substantially harder to exploit security holes in Git by\noffering a much more unpredictable attack surface.\n\nASLR interferes with GDB's ability to set breakpoints. A similar issue\nholds true when compiling with -O2 (in which case single-stepping is\nmessed up because GDB cannot map the code back to the original source\ncode properly). Therefore we simply enable ASLR only when an\noptimization flag is present in the CFLAGS, using it as an indicator\nthat the developer does not want to debug in GDB anyway.\n\nSigned-off-by: İsmail Dönmez <ismail@i10z.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n config.mak.uname | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex f2ac755753..ea0df3fe1b 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -573,6 +573,12 @@ else\n \tifneq ($(shell expr \"$(uname_R)\" : '1\\.'),2)\n \t\t# MSys2\n \t\tprefix = /usr/\n+\t\t# Enable DEP\n+\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n+\t\t# Enable ASLR (unless debugging)\n+\t\tifneq (,$(findstring -O,$(filter-out -O0 -Og,$(CFLAGS))))\n+\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n+\t\tendif\n \t\tifeq (MINGW32,$(MSYSTEM))\n \t\t\tprefix = /mingw32\n \t\t\tHOST_CPU = i686\n-- \ngitgitgadget\n"},{"id":"375135","messageId":"828913e96c77ba4eb94b35cb4c1f1bf92b2e1c3f.1557315057.git.gitgitgadget@gmail.com","threadId":"51008","inReplyTo":"pull.134.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] mingw: do not let ld strip relocations","fromName":"İsmail Dönmez via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-08T11:30:58Z","receivedAt":"2019-05-08T11:31:05Z","isPatch":true,"sender":{"key":"ismail@pardus.org.tr","avatar":null},"body":"From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n\nThis is the first step for enabling ASLR (Address Space Layout\nRandomization) support. We want to enable ASLR for better protection\nagainst exploiting security holes in Git: it makes it harder to attack\nsoftware by making code addresses unpredictable.\n\nThe problem fixed by this commit is that `ld.exe` seems to be stripping\nrelocations which in turn will break ASLR support. We just make sure\nit's not stripping the main executable entry.\n\nSigned-off-by: İsmail Dönmez <ismail@i10z.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n config.mak.uname | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 3605fead53..f2ac755753 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -576,10 +576,12 @@ else\n \t\tifeq (MINGW32,$(MSYSTEM))\n \t\t\tprefix = /mingw32\n \t\t\tHOST_CPU = i686\n+\t\t\tBASIC_LDFLAGS += -Wl,--pic-executable,-e,_mainCRTStartup\n \t\tendif\n \t\tifeq (MINGW64,$(MSYSTEM))\n \t\t\tprefix = /mingw64\n \t\t\tHOST_CPU = x86_64\n+\t\t\tBASIC_LDFLAGS += -Wl,--pic-executable,-e,mainCRTStartup\n \t\telse\n \t\t\tCOMPAT_CFLAGS += -D_USE_32BIT_TIME_T\n \t\t\tBASIC_LDFLAGS += -Wl,--large-address-aware\n-- \ngitgitgadget\n\n"},{"id":"375136","messageId":"nycvar.QRO.7.76.6.1905081331060.44@tvgsbejvaqbjf.bet","threadId":"51008","inReplyTo":"2e7be484-74d7-7258-954e-3a4a34a36c01@gmail.com","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-08T11:33:08Z","receivedAt":"2019-05-08T11:33:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Wed, 1 May 2019, Alban Gruin wrote:\n\n> Le 01/05/2019 à 00:41, Johannes Schindelin a écrit :\n> >\n> > On Tue, 30 Apr 2019, Johannes Sixt wrote:\n> >\n> >> [had to add Dscho as recipient manually, mind you]\n> >\n> > I usually pick up responses to GitGitGadget patch series even if I am not\n> > on explicit Cc: (but it might take a couple of days when I am too busy\n> > elsewhere to read the Git mailing list).\n> >\n> >> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget:\n> >>> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= <ismail@i10z.com>\n> >>>\n> >>> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout\n> >>> Randomization) support. This applies to both 32bit and 64bit builds\n> >>> and makes it substantially harder to exploit security holes in Git by\n> >>> offering a much more unpredictable attack surface.\n> >>>\n> >>> ASLR interferes with GDB's ability to set breakpoints. A similar issue\n> >>> holds true when compiling with -O2 (in which case single-stepping is\n> >>> messed up because GDB cannot map the code back to the original source\n> >>> code properly). Therefore we simply enable ASLR only when an\n>\n> I don’t know if it stands true when combined with something like -ggdb3,\n> but I may be very wrong.  Feel free to correct me.\n\nPossibly, but that makes my job here harder, so I won't even try right now\n;-)\n\n> >>> optimization flag is present in the CFLAGS, using it as an indicator\n> >>> that the developer does not want to debug in GDB anyway.\n> >>>\n> >>> Signed-off-by: İsmail Dönmez <ismail@i10z.com>\n> >>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >>> ---\n> >>>  config.mak.uname | 6 ++++++\n> >>>  1 file changed, 6 insertions(+)\n> >>>\n> >>> diff --git a/config.mak.uname b/config.mak.uname\n> >>> index e7c7d14e5f..a9edcc5f0b 100644\n> >>> --- a/config.mak.uname\n> >>> +++ b/config.mak.uname\n> >>> @@ -570,6 +570,12 @@ else\n> >>>  \tifeq ($(shell expr \"$(uname_R)\" : '2\\.'),2)\n> >>>  \t\t# MSys2\n> >>>  \t\tprefix = /usr/\n> >>> +\t\t# Enable DEP\n> >>> +\t\tBASIC_LDFLAGS += -Wl,--nxcompat\n> >>> +\t\t# Enable ASLR (unless debugging)\n> >>> +\t\tifneq (,$(findstring -O,$(CFLAGS)))\n> >>> +\t\t\tBASIC_LDFLAGS += -Wl,--dynamicbase\n> >>> +\t\tendif\n> >>>  \t\tifeq (MINGW32,$(MSYSTEM))\n> >>>  \t\t\tprefix = /mingw32\n> >>>  \t\t\tHOST_CPU = i686\n> >>>\n> >>\n> >> I'm a bit concerned that this breaks my debug sessions where I use -O0.\n> >> But I'll test without -O0 before I really complain.\n> >\n> > Weird. Jameson Miller also mentioned this very concern in an internal\n> > review.\n> >\n> > I guess I'll do something like\n> >\n> > \tifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS))))\n> >\n>\n> -Og also exists to debug[0], even if it’s far less known.\n\nGood point.\n\n> Perhaps it’s better to check for -g (and its variants[1]) as the user\n> clearly states their intent to debug the resulting binary, rather than\n> checking for special cases.\n\nI don't think we can use that, as we specifically build Git for Windows\nwith optimization *and* with debug symbols (and then use cv2pdb to extract\nthose debug symbols into external .pdb files for use with advanced\npost-mortem tools, i.e. we do *not* need to single-step).\n\nThanks,\nDscho\n"},{"id":"375137","messageId":"nycvar.QRO.7.76.6.1905081333220.44@tvgsbejvaqbjf.bet","threadId":"51008","inReplyTo":"20190501233641.GC202237@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] mingw: enable DEP and ASLR","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-08T11:33:54Z","receivedAt":"2019-05-08T11:34:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Wed, 1 May 2019, brian m. carlson wrote:\n\n> On Wed, May 01, 2019 at 08:39:22PM +0200, Alban Gruin wrote:\n> > -Og also exists to debug[0], even if it’s far less known.  Perhaps it’s\n> > better to check for -g (and its variants[1]) as the user clearly states\n> > their intent to debug the resulting binary, rather than checking for\n> > special cases.\n>\n> I can't speak for the Windows folks, but Debian frequently builds with\n> -O2 -g and strips the debugging symbols into a separate package that can\n> be installed in case of a crash. So -g need not be an indication of\n> non-production use.\n\nPrecisely, Git for Windows imitates this strategy.\n\nThanks,\nDscho\n"}]}