Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15290 closed defect (fixed)

Compile everything with secure flags

Reported by: DrWhax Owned by: datallah
Milestone: 2.10.7 Component: winpidgin (gtk)
Version: 2.10.6 Keywords: security aslr dep build
Cc: ioerror

Description

Hi,

I tested out if Pidgin.exe had secure flags enabled like ASLR and DEP protection. This wasn't the case and should really be build with ASLR and DEP enforced on the binary.

The DLL's shipped with Pidgin are not build with secure flags either. I compiled a list using BinScope? here: http://cryptohub.nl/pidgin/pidgin.html also see #15286

At the moment exploitation of Pidgin is like '90's style, anybody can do it with zero to no skills...

I hope we can come up with a secure build sequence which will guarantee, NX, DEP, ASLR, /GS, SafeSEH(am I missing something?)

Attachments (2)

win32_secure_flags.diff (4.6 KB) - added by datallah 4 years ago.
win32_secure_flags2.diff (4.9 KB) - added by datallah 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 in reply to: ↑ description ; follow-up: Changed 4 years ago by Robby

Replying to DrWhax:

I hope we can come up with a secure build sequence which will guarantee, NX, DEP, ASLR, /GS, SafeSEH(am I missing something?)

Who's "we" then?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by ioerror

Replying to Robby:

Replying to DrWhax:

I hope we can come up with a secure build sequence which will guarantee, NX, DEP, ASLR, /GS, SafeSEH(am I missing something?)

Who's "we" then?

Anyone involved in helping to do this kind of work - it seems like the right answer is to add those flags to the Makefile.mingw - here's what I recently added to pidgin-otr to improve compile and linking hardening:

CC_HARDENING_OPTIONS ?= -fstack-protector-all -fPIE -Wstack-protector -fwrapv --param ssp-buffer-size=1
LD_HARDENING_OPTIONS ?= --dynamicbase --nxcompat -pie

Later, I just added those two variables into the proper CC and LDFLAGS:

override CFLAGS += -g -O2 -Wall $(CC_HARDENING_OPTIONS)
LDFLAGS = -Wl,--enable-auto-image-base $(LD_HARDENING_OPTIONS) -lssp

-lssp is required to get the stack smashing protection libraries linked in properly.

DrWhax? - if you build with those flags, what is missing?

The full flags used in pidgin-otr are here:

# Compiling with -fPIE and linking with -pie causes the plugin to crash
# on load, so we'll skip those.
CC_HARDENING_OPTIONS ?= -fstack-protector-all -Wstack-protector -fwrapv \
	--param ssp-buffer-size=1 -fno-strict-overflow -Wall -Wextra \
	-Wno-unused-parameter -Wno-missing-field-initializers -Wformat-security
# In theory, we'd also like the following:
# LD_HARDENING_OPTIONS ?= -dynamicbase --nxcompat -pie -z relro -z now
LD_HARDENING_OPTIONS ?= --dynamicbase --nxcompat

And we then set things up accordingly:

LDFLAGS = -Wl,--enable-auto-image-base $(LD_HARDENING_OPTIONS)
LDLIBS = -lssp
override CFLAGS += -g -O2 -Wall $(CC_HARDENING_OPTIONS)

If any of that works, I think it should be trivial to generate a patch that adds those hardening options.

comment:3 Changed 4 years ago by DrWhax

That looks good, doesn't seem we are missing out on hardening options. I'll give it a try on a machine somewhere today I hope and report back in this ticket.

comment:4 Changed 4 years ago by datallah

  • Component changed from unclassified to winpidgin (gtk)
  • Owner changed from rekkanoryo to datallah

Do those flags actually work for pidgin-otr?

I needed to use the following so that the flags actually were passed to the linker to get it to work in Pidgin:

LD_HARDENING_OPTIONS ?= -Wl,--dynamicbase -Wl,--nxcompat

There were no compiler errors, ASLR just wasn't enabled for the process when it was launched.

Also, it seems like -fPIE and -pie don't make sense on windows since stuff is relocatable anyway - the compiler prints out a message to that effect if you use -fPIE.

I've got Pidgin building with various options enabled and it seems to work ok (patch attached). I haven't yet figured the right way to ship the ssp dll, I manually copied it from the the gcc bin directory to test this out; once that's resolved and this gets a bit more testing, I can commit it.

Changed 4 years ago by datallah

Changed 4 years ago by datallah

comment:5 Changed 4 years ago by datallah

I uploaded an updated patch that links libssp statically into both pidgin.exe and libpurple.dll - everything else is linked to libpurple.dll, so that's all that is necessary. Ideally we'd ship libssp-0.dll and link it dynamically, but I don't see an easy way to grab the dll during the packaging process.

It isn't clear to me what the benefit of using -fwrapv is, and I don't even see -fno-strict-overflow documented in the gcc documentation at all.

comment:6 Changed 4 years ago by datallah

I was able to find some information about why -fwrapv and -fno-strict-overflow are suggested here. http://www.airs.com/blog/archives/120 and http://gcc.gnu.org/ml/gcc-help/2011-07/msg00227.html are interesting.

It's odd that-fno-strict-overflow isn't listed in the gcc documentation, but it is mentioned in the description of -fstrict-overflow in the optimization options page. I guess that's just an oversight.

For reference (partially so I can find it easily) #13879 is the corresponding ticket to this one that applies to the autoconfiscated build.

comment:7 Changed 4 years ago by Daniel Atallah <datallah@…>

(In [d9ebf57cd361]):
Enable DEP and ASLR flags as well as some other security related compiler flags for win32 builds. Refs #15290

Thanks goes to Jacob Appelbaum for pointing at the related patch he came up with for pidgin-otr.

comment:8 Changed 4 years ago by Daniel Atallah <datallah@…>

(In [3d82cc32b7a2]):
Use SetProcessDEPPolicy to permanently enable DEP during startup on supported versions of Windows. Refs #15290

comment:9 Changed 4 years ago by Daniel Atallah <datallah@…>

  • Milestone set to 2.10.7
  • Resolution set to fixed
  • Status changed from new to closed

(In [75dbccde9fc7]):
Enable the GCC Stack-Smashing Protection functionality (-fstack-protector-all) Thanks again to Jacob Appelbaum for pointing at his pidgin-otr patch.

Fixes #15290

comment:10 Changed 4 years ago by datallah

Ticket #15209 has been marked as a duplicate of this ticket.

comment:11 in reply to: ↑ 2 Changed 4 years ago by noloader

Replying to ioerror:

Replying to Robby:

Replying to DrWhax:

I hope we can come up with a secure build sequence which will guarantee, NX, DEP, ASLR, /GS, SafeSEH(am I missing something?)

Who's "we" then?

Anyone involved in helping to do this kind of work - it seems like the right answer is to add those flags to the Makefile.mingw - here's what I recently added to pidgin-otr to improve compile and linking hardening:

CC_HARDENING_OPTIONS ?= -fstack-protector-all -fPIE -Wstack-protector -fwrapv --param ssp-buffer-size=1
LD_HARDENING_OPTIONS ?= --dynamicbase --nxcompat -pie

Later, I just added those two variables into the proper CC and LDFLAGS:

override CFLAGS += -g -O2 -Wall $(CC_HARDENING_OPTIONS)
LDFLAGS = -Wl,--enable-auto-image-base $(LD_HARDENING_OPTIONS) -lssp

-lssp is required to get the stack smashing protection libraries linked in properly.

DrWhax? - if you build with those flags, what is missing?

The full flags used in pidgin-otr are here:

# Compiling with -fPIE and linking with -pie causes the plugin to crash
# on load, so we'll skip those.
CC_HARDENING_OPTIONS ?= -fstack-protector-all -Wstack-protector -fwrapv \
	--param ssp-buffer-size=1 -fno-strict-overflow -Wall -Wextra \
	-Wno-unused-parameter -Wno-missing-field-initializers -Wformat-security
# In theory, we'd also like the following:
# LD_HARDENING_OPTIONS ?= -dynamicbase --nxcompat -pie -z relro -z now
LD_HARDENING_OPTIONS ?= --dynamicbase --nxcompat

And we then set things up accordingly:

LDFLAGS = -Wl,--enable-auto-image-base $(LD_HARDENING_OPTIONS)
LDLIBS = -lssp
override CFLAGS += -g -O2 -Wall $(CC_HARDENING_OPTIONS)

If any of that works, I think it should be trivial to generate a patch that adds those hardening options.

Be careful of <tt>-fwrapv</tt>. Its used as a crutch to make illegal programs work. Its better to fix the problems.

The project was given a harden set of flags for Linux at https://developer.pidgin.im/ticket/15209. In addition to the GCC flags in ticket 15209, it should also be using the following warnings: -Wall -Wextra -Wconversion -Wformat=2 -Wformat-security -Wstrict-overflow. GCC has good static analysis capabilities, and the feature can be used to find a number of defects. The sources have a number of defects when compiled with elevated warnings (truncations errors, conversion errors, and overflow/wrap).

comment:12 follow-up: Changed 4 years ago by datallah

These changes make our crash reporting stack traces useless (I assume it is the ASLR that is problematic). I'm not sure if there's going to be an easy way to fix that.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 4 years ago by noloader

Replying to datallah:

These changes make our crash reporting stack traces useless (I assume it is the ASLR that is problematic). I'm not sure if there's going to be an easy way to fix that.

Hmmm... I've never heard that one (I'm not being a smart ass). Optimizations and stripping symbols usually causes this problem. What is Pidgin using for crash reporting?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by datallah

Replying to noloader:

Replying to datallah:

These changes make our crash reporting stack traces useless (I assume it is the ASLR that is problematic). I'm not sure if there's going to be an easy way to fix that.

Hmmm... I've never heard that one (I'm not being a smart ass). Optimizations and stripping symbols usually causes this problem. What is Pidgin using for crash reporting?

The only compiler changes are the the additions to CC_HARDENING_OPTIONS and the linker changes are LD_HARDENING_OPTIONS in [d9ebf57cd361]. The way things are being stripped hasn't changed either.

We're using a forked version of the drmingw exchndl.dll which uses the binutils bfd library to get at the debug symbols.

If there were something better, we'd be happy to use it.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by noloader

Replying to datallah:

Replying to noloader:

Replying to datallah:

These changes make our crash reporting stack traces useless (I assume it is the ASLR that is problematic). I'm not sure if there's going to be an easy way to fix that.

Hmmm... I've never heard that one (I'm not being a smart ass). Optimizations and stripping symbols usually causes this problem. What is Pidgin using for crash reporting?

The only compiler changes are the the additions to CC_HARDENING_OPTIONS and the linker changes are LD_HARDENING_OPTIONS in [d9ebf57cd361]. The way things are being stripped hasn't changed either.

We're using a forked version of the drmingw exchndl.dll which uses the binutils bfd library to get at the debug symbols.

OK, I had a chance to take a brief look at exchndl.dll.

I think exchndl.dll is doing too much in DllMain? (via OnStartup?), and I would expect Pidgin to see some intermittent problems. I don't see where its safe to call SetUnhandledExceptionFilter? in DllMain?. "DllMain? : a horror story," http://blogs.msdn.com/b/oleglv/archive/2003/10/28/56142.aspx; "DllMain? and life before birth," http://blogs.msdn.com/b/oleglv/archive/2003/10/24/56141.aspx.

By calling SetUnhandledExceptionFilter? on the thread that calls DllMain?, the library is actually installing the exception handler on a thread created by the loader. The thread dies after loading. http://www.oldschoolhack.de/tutorials/seh.pdf.

exchndl.dll is also manually duplicating a lot of functionality offered in Debug Help. Debug Help is written by the folks who write WinDebug? (and they are they folks who write the OS). I've had a lot of success over the years with Debug Help and its fully redistributable (the only drag is you have to download the entered SDK to get the library). http://msdn.microsoft.com/en-us/library/windows/desktop/ms679309(v=vs.85).aspx.

If there were something better, we'd be happy to use it.

You are in a sticky situation. As a project maintainer, you are bringing in components that you are responsible for (the project is responsible for its Supply Chain and outside code falls within those bounds). You can file bug reports upstream, but its no consolation when Pidgin takes a bug because upstream does not suffer Pidgin bugs. Personally, I have aversions to the "let's use library X" because I don't want to be responsible for library X (the Boost fan club is especially loud).

With that said, have you considered google-breakpad (http://code.google.com/p/google-breakpad/). It claims to be an open-source multi-platform crash reporting system. Its under a BSD style license, which is fairly permissive. I know the code has a few issues from a quick scan of some of the sources (for example, DLL Hijacking or Binary Planting), but the issues should be fixed soon.

You also have Windows Error Reporting (WER) on the Windows platform (http://msdn.microsoft.com/en-us/library/windows/hardware/gg487440.aspx).

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by datallah

Replying to noloader:

I think exchndl.dll is doing too much in DllMain? (via OnStartup?), and I would expect Pidgin to see some intermittent problems. I don't see where its safe to call SetUnhandledExceptionFilter? in DllMain?. "DllMain? : a horror story," http://blogs.msdn.com/b/oleglv/archive/2003/10/28/56142.aspx; "DllMain? and life before birth," http://blogs.msdn.com/b/oleglv/archive/2003/10/24/56141.aspx.

By calling SetUnhandledExceptionFilter? on the thread that calls DllMain?, the library is actually installing the exception handler on a thread created by the loader. The thread dies after loading. http://www.oldschoolhack.de/tutorials/seh.pdf.

Thanks for the interesting links, there's certainly some information there that I wasn't aware of. I think we may have been fortunate in how we use exchndl.dll so that we're not impacted by some of these (or maybe this is the reason that the upstream instructions are to use it the way we're doing it). We don't actually link to exchndl - instead it is loaded dynamically using LoadLibrary by pidgin.exe. I think this prevents the threading initialization problems as well as the ambituity about load order because it ends up being invoked in our thread instead of the loader's thread.

There may still be issues with calling SetUnhandledExceptionFilter in DllMain, but it is a kernel32 function, so it may be ok according the "DllMain and life before birth" article. At any rate, it would be relatively trivial to change that so that we call an initialization function explicitly, so perhaps I'll do that (on the other hand we've been using this a long time and I'm not aware of any weird issues).

exchndl.dll is also manually duplicating a lot of functionality offered in Debug Help. Debug Help is written by the folks who write WinDebug? (and they are they folks who write the OS). I've had a lot of success over the years with Debug Help and its fully redistributable (the only drag is you have to download the entered SDK to get the library). http://msdn.microsoft.com/en-us/library/windows/desktop/ms679309(v=vs.85).aspx.

Are you referring to the IMGHELP.DLL stuff or other functionality that bfd is being used for? I had briefly looked at dbghelp when cleaning up exchndl and found that apparently [https://code.google.com/p/jrfonseca/wiki/DrMingw#The_IMAGEHLP.DLL_Saga modern IMGHELP.DLLs use dbghelp. I didn't look too much at it because I didn't have the include files handy without using the Platform SDK.

We actually couldn't redistribute it (or anything else that's not GPL compatible) - it would be ok to use it if it were a system library (which it actually appears to be), but if it isn't, it would be a violation of our license to use it.

If there were something better, we'd be happy to use it.

You are in a sticky situation. As a project maintainer, you are bringing in components that you are responsible for (the project is responsible for its Supply Chain and outside code falls within those bounds). You can file bug reports upstream, but its no consolation when Pidgin takes a bug because upstream does not suffer Pidgin bugs. Personally, I have aversions to the "let's use library X" because I don't want to be responsible for library X (the Boost fan club is especially loud).

Well, unless you write everything from scratch, which I don't think is reasonable, there aren't other options than to use other people's code :). I'm not really into reinventing the wheel and I'm certainly already deeper into exchndl.dll already than I'd prefer to be.

With that said, have you considered google-breakpad (http://code.google.com/p/google-breakpad/). It claims to be an open-source multi-platform crash reporting system. Its under a BSD style license, which is fairly permissive. I know the code has a few issues from a quick scan of some of the sources (for example, DLL Hijacking or Binary Planting), but the issues should be fixed soon.

Breakpad has been on my radar for a while and I have briefly looked at it. The licensing appears to be ok. Unfortunately, it doesn't seem to work with gcc debug symbols on windows - there are a number of tickets about mingw issues.

You also have Windows Error Reporting (WER) on the Windows platform (http://msdn.microsoft.com/en-us/library/windows/hardware/gg487440.aspx).

Again, this isn't going to be able to handle gcc debug symbols :(

comment:17 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by noloader

Replying to datallah:

Replying to noloader:

exchndl.dll is also manually duplicating a lot of functionality offered in Debug Help. Debug Help is written by the folks who write WinDebug? (and they are they folks who write the OS). I've had a lot of success over the years with Debug Help and its fully redistributable (the only drag is you have to download the entered SDK to get the library). http://msdn.microsoft.com/en-us/library/windows/desktop/ms679309(v=vs.85).aspx.

Are you referring to the IMGHELP.DLL stuff or other functionality that bfd is being used for? I had briefly looked at dbghelp when cleaning up exchndl and found that apparently [https://code.google.com/p/jrfonseca/wiki/DrMingw#The_IMAGEHLP.DLL_Saga modern IMGHELP.DLLs use dbghelp. I didn't look too much at it because I didn't have the include files handy without using the Platform SDK.

We actually couldn't redistribute it (or anything else that's not GPL compatible) - it would be ok to use it if it were a system library (which it actually appears to be), but if it isn't, it would be a violation of our license to use it.

DbgHelp?.dll is a different library from Image Help. I think DbgHelp?.dll has everything you need already written by the folks who know the most about the Windows operating system. "Enumerating Symbol Files," "Retrieving Symbols by Address," "Retrieving Symbols by Symbol Name," "Retrieving Line Numbers by Address," and "Retrieving Line Numbers by Symbol Name" :http://msdn.microsoft.com/en-us/library/windows/desktop/ms679345(v=vs.85).aspx.

comment:18 in reply to: ↑ 17 Changed 4 years ago by datallah

Replying to noloader:

DbgHelp?.dll is a different library from Image Help. I think DbgHelp?.dll has everything you need already written by the folks who know the most about the Windows operating system. "Enumerating Symbol Files," "Retrieving Symbols by Address," "Retrieving Symbols by Symbol Name," "Retrieving Line Numbers by Address," and "Retrieving Line Numbers by Symbol Name" :http://msdn.microsoft.com/en-us/library/windows/desktop/ms679345(v=vs.85).aspx.

Unfortunately, I don't see how this would be helpful to us except for replacing the imghelp stuff which exchndl now uses to process MSVC compiled code in the stack. My googling found a bunch of references which indicate it isn't able to handle the debug symbols that gcc generates.

Interestingly there is a dbghelp clone that the Wine folks have written that does handle gcc debug symbols (there's a mingw port, but that hasn't been updated in 3 years, which isn't promising); perhaps that warrants some investigation.

I also just found the dbg library which is a new library that sounds like it might be able to do some of the exchndl dirty work.

Unfortunately these all still like they'd be a significant amount of coding on our end to get working - I'd really like a drop-in thing that just works. It doesn't seem like that would be too much to ask, but apparently it is.

Last edited 4 years ago by datallah (previous) (diff)
Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!