Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ASLR & DEP #123

Merged
merged 3 commits into from
Jan 22, 2017
Merged

Enable ASLR & DEP #123

merged 3 commits into from
Jan 22, 2017

Conversation

Chocobo1
Copy link
Contributor

@Chocobo1 Chocobo1 commented Jan 18, 2017

@mattock
Copy link
Member

mattock commented Jan 18, 2017

Not sure if this was a coincidence: OpenVPN/openvpn-build#79

With that change OpenVPN GUI also gets ASLR/DEP, but only when built using openvpn-build, which is not the only option. So this PR also makes sense I think.

@Chocobo1
Copy link
Contributor Author

Not sure if this was a coincidence: OpenVPN/openvpn-build#79

Sometimes happens ;)


I was hoping the CI could check my PR, because I'm not sure "Use high entropy ASLR" will work on older mingw version, mine is gcc version 6.3.0 (Rev1, Built by MSYS2 project)

@Chocobo1
Copy link
Contributor Author

Now I see the CI check failed, should I remove the --high-entropy-va part?

@chipitsine
Copy link
Contributor

the best would be "modify configure.ac to check whether that option is supported..."
current CI is xenial mingw

there's also "building openvpn" guide on https://community.openvpn.net/openvpn/wiki/BuildingOpenVPN-GUI

if you want to add some option which is not widely supported, it should be written there

@chipitsine
Copy link
Contributor

or you can make another PR to update CI settings/repo/mingw :-)

@Chocobo1
Copy link
Contributor Author

Chocobo1 commented Jan 18, 2017

Alright, I added a test to see if the linker supports --high-entropy-va flag, now it pass the CI check.

@@ -57,7 +57,7 @@ case "$host" in
*-mingw*)
CPPFLAGS="${CPPFLAGS} -DWIN32_LEAN_AND_MEAN"
CPPFLAGS="${CPPFLAGS} -D_WIN32_WINNT=NTDDI_WINXP"
LDFLAGS="${LDFLAGS} -Wl,--nxcompat"
LDFLAGS="${LDFLAGS} -Wl,--nxcompat,--dynamicbase,--export-all-symbols"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this hack of adding --export-all-symbols still needed in mingw? There has to be a better solution than exporting all symbols

Copy link
Contributor Author

@Chocobo1 Chocobo1 Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly yes, I checked in vmmap & procexp.

I also notice the shipped openssl dll doesn't enable aslr.

Another thought, why this project doesn't support building in MSVC? It support all these security features flawlessly.

@cron2
Copy link
Contributor

cron2 commented Jan 18, 2017 via email

@selvanair
Copy link
Collaborator

selvanair commented Jan 18, 2017

To add to that we are not MSVC compliant right now -- our resource files need a lot of changes to satisfy Windows resource compiler which is pretty "primitive" in handling line continuation using "", preprocessor macros in stringtable etc.

@selvanair selvanair closed this Jan 18, 2017
@selvanair selvanair reopened this Jan 18, 2017
@selvanair
Copy link
Collaborator

selvanair commented Jan 18, 2017

Oops -- pressed the wrong button!

We are not MSVC compliant right now -- I had spent some time with R Morris and we did get it compile with a few tweaks except for the resource file which required a lot of edits so we ported only one language file. MS resource compiler is rather primitive in handling line continuation using "\" that we use extensively, preprocessor macros in stringtable etc. Nothing too complicated to change, but not enough motivation to do so..

@selvanair
Copy link
Collaborator

I did some tests on enabling ASLR using mingw. As pointed out by @Chocobo1, using only -dynamicbase is not enough. In fact it has no effect except that the ASLR flag gets set in the executable[1]. But --export-all-symbols is not required, just exporting one symbol is enough.

Until mingw/binutils fixes this for good[2], we could add a dummy global like __declspec(dllexport) char aslr_workaround; and then -Wl,--dynamicbase will work.

Notes:
[1] Atleast on Win7 64 bit, with or without -dynamicbase I find both heap and env block are always randomized, code and stack remains fixed unless reloc section is present. Setting the flag alone is enough for DLLs as reloc is already there.
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=17321

@Chocobo1
Copy link
Contributor Author

Chocobo1 commented Jan 22, 2017

But --export-all-symbols is not required, just exporting one symbol is enough.

I adopted the workaround and gave credit to you in commit msg.

There is still a thing that bothers me, the image base address isn't randomized.
MS linker have a RandomizedBaseAddress option[1] for this, but I don't find anything on gcc/mingw side. Update: rechecked, not true.

[1]: https://msdn.microsoft.com/en-us/library/microsoft.visualstudio.vcprojectengine.vclinkertool.randomizedbaseaddress.aspx

@selvanair
Copy link
Collaborator

selvanair commented Jan 22, 2017

I think setting this property in VS is the dumbed-down way of saying pass /dynamicbase to the linker. See item 5 in the msdn docs for /dynamicbase (https://msdn.microsoft.com/en-us/library/bb384887.aspx)

EDIT: I do not use VS so just guessing..

@Chocobo1
Copy link
Contributor Author

Yes you're right.
And in my post above I got my mind mixed up, so it's not true.
So is this PR eligible for merging?

@selvanair
Copy link
Collaborator

DEP appears enabled by default for 64bit programs (tested Win7, Win10), yet the flag -nxcompat may help other platforms.
For ASLR, the --high-entropy-va flag is not supported on 32bit, the check in configure takes care of that. The feature itself appears to work well on Win10 (apparently supported in Win8 as well).
Merging.

@selvanair selvanair merged commit f35308d into OpenVPN:master Jan 22, 2017
@Chocobo1 Chocobo1 deleted the ldflags branch January 23, 2017 03:32
@Chocobo1
Copy link
Contributor Author

Thanks to everyone involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants