-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
git.exe doesn't seem to be ASLR/DEP enabled. #644
Comments
I get this: ./checksec.exe /c/Program\ Files/Git/mingw64/bin/git.exe
64-bit binary
Large address aware: Yes
ASLR: Yes
ASLR^2: No
DEP: Yes
TS Aware: No
/GS: No Please note that I inspected The latter is actually just a redirector (which we call "git-wrapper"). And yes, it is apparently not compiled with the correct flags. Let me check that. |
Indeed, /mingw64/bin/git.exe is fine. Sorry for the hassle and thanks for getting this out. |
This ensures that the Git wrapper is compiled with Adress Space Layout Randomization (ASLR) and Data Execution Prevention (DEP). The Git wrapper is still compiled without the stack protector, because that would require the executables to be linked against libssp, something we definitely do not want for e.g. git-bash.exe). Since the Git wrapper also serves as /bin/git redirector (to keep legacy callers working that expect git.exe to live in the top-level /bin/ directory), this fixes #644 partially (which pointed out that /bin/git.exe is compiled without ASLR/DEP). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Address Space Layout Randomization (ASLR) and Data Execution Prevention (DEP) are two powerful techniques to stave off exploits. Since both essentially come for free (performance-wise) it is wise to enable them. Together with Git for Windows' 5b24535 (fixup! mingw: Compile the Git wrapper, 2016-02-03) this concludes addressing the ticket git-for-windows/git#644. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@ismail Ah, I see you closed it already. These two commits were still needed to address the issue pointed out by you, though ;-) |
Aha! I am happy the bug report was not for nothing :) |
@ismail your report was definitely not for nothing. First of all, you already contributed actual code, which counts quite a bit in my book. Second: you raised a valid concern, and even if I would not have changed the Git wrapper, just having this ticket with the discussion on record is worth it. And lastly, you provided this neat little program to check whether ASLR/DEP are enabled. For future self's benefit, I will reproduce it here: // Original code belongs to Raymond Chen
// See https://blogs.msdn.microsoft.com/oldnewthing/20150518-00/?p=45581
#pragma comment(lib, "dbghelp.lib")
#include <windows.h>
#include <imagehlp.h>
#include <stdio.h> // horrors! mixing stdio and C++!
#include <stddef.h>
#define nullptr NULL
class MappedImage
{
public:
bool MapImage(const char* fileName);
void ProcessResults();
~MappedImage();
private:
WORD GetCharacteristics();
template<typename T>
WORD GetDllCharacteristics();
template<typename T>
bool HasSecurityCookie();
private:
HANDLE file_ = INVALID_HANDLE_VALUE;
HANDLE mapping_ = nullptr;
void *imageBase_ = nullptr;
IMAGE_NT_HEADERS* headers_ = nullptr;
int bitness_ = 0;
};
bool MappedImage::MapImage(const char* fileName)
{
file_ = CreateFile(fileName, GENERIC_READ,
FILE_SHARE_READ,
NULL,
OPEN_EXISTING,
0,
NULL);
if (file_ == INVALID_HANDLE_VALUE) return false;
mapping_ = CreateFileMapping(file_, NULL, PAGE_READONLY,
0, 0, NULL);
if (!mapping_) return false;
imageBase_ = MapViewOfFile(mapping_, FILE_MAP_READ, 0, 0, 0);
if (!imageBase_) return false;
headers_ = ImageNtHeader(imageBase_);
if (!headers_) return false;
if (headers_->Signature != IMAGE_NT_SIGNATURE) return false;
switch (headers_->OptionalHeader.Magic) {
case IMAGE_NT_OPTIONAL_HDR32_MAGIC: bitness_ = 32; break;
case IMAGE_NT_OPTIONAL_HDR64_MAGIC: bitness_ = 64; break;
default: return false;
}
return true;
}
MappedImage::~MappedImage()
{
if (imageBase_) UnmapViewOfFile(imageBase_);
if (mapping_) CloseHandle(mapping_);
if (file_ != INVALID_HANDLE_VALUE) CloseHandle(file_);
}
WORD MappedImage::GetCharacteristics()
{
return headers_->FileHeader.Characteristics;
}
template<typename T>
WORD MappedImage::GetDllCharacteristics()
{
return reinterpret_cast<T*>(headers_)->
OptionalHeader.DllCharacteristics;
}
template<typename T>
bool MappedImage::HasSecurityCookie()
{
ULONG size;
T *data = static_cast<T*>(ImageDirectoryEntryToDataEx(
imageBase_, TRUE, IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG,
&size, NULL));
if (!data) return false;
ULONG minSize = offsetof(T, SecurityCookie) +
sizeof(data->SecurityCookie);
if (size < minSize) return false;
if (data->Size < minSize) return false;
return data->SecurityCookie != 0;
}
void MappedImage::ProcessResults()
{
printf("%d-bit binary\n", bitness_);
auto Characteristics = GetCharacteristics();
printf("Large address aware: %s\n",
(Characteristics & IMAGE_FILE_LARGE_ADDRESS_AWARE)
? "Yes" : "No");
auto DllCharacteristics = bitness_ == 32
? GetDllCharacteristics<IMAGE_NT_HEADERS32>()
: GetDllCharacteristics<IMAGE_NT_HEADERS64>();
printf("ASLR: %s\n",
(DllCharacteristics & IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE)
? "Yes" : "No");
printf("ASLR^2: %s\n",
(DllCharacteristics & IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA)
? "Yes" : "No");
printf("DEP: %s\n",
(DllCharacteristics & IMAGE_DLLCHARACTERISTICS_NX_COMPAT)
? "Yes" : "No");
printf("TS Aware: %s\n",
(DllCharacteristics & IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE)
? "Yes" : "No");
bool hasSecurityCookie =
bitness_ == 32 ? HasSecurityCookie<IMAGE_LOAD_CONFIG_DIRECTORY32>()
: HasSecurityCookie<IMAGE_LOAD_CONFIG_DIRECTORY64>();
printf("/GS: %s\n", hasSecurityCookie
? "Yes" : "No");
}
int __cdecl main(int argc, char**argv)
{
MappedImage mappedImage;
if (mappedImage.MapImage(argv[1])) {
mappedImage.ProcessResults();
}
return 0;
} Compile it within Git for Windows' SDK using |
Using the test program from https://github.com/ismail/randomworks/blob/master/windows/checksec.cpp
Only tested the portable binary. I am afraid something has gone wrong. can you check the build log to see if ldflags are properly applied?
The text was updated successfully, but these errors were encountered: