Build Git with Stack Smashing Protector enabled #501
Conversation
We can easily add that flag for Git for Windows only. And once we can demonstrate that it does not hurt, but helps things instead, we can suggest it to upstream. (Those are my thoughts on the matter 😺 ) |
|
So I turned this into a Pull Request. All of the regression test suite passes with this. |
|
Git for Windows » git #29 SUCCESS |
|
@shiftkey could you have a look? |
|
@dscho ERR_CONNECTION_TIMED_OUT ? |
|
Does it have any visible effect on performance? |
Well, this is disappointing. I really think that all my work resulted in a project that empowers people to answer such questions themselves instead of putting even more load onto my shoulders. The answer can be easily obtained by using the SDK yourself, compiling Git with and without the flag, and running a decently-long-running task several times. So instead of letting me research whether |
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes git-for-windows#501 Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
stack-protector
branch
from
f0f23e5 to
6fc6a9d
Compare
dscho
changed the title
Build Git with Stack Smashing Protector enabled
|
@dscho sorry, my question was generally addressed to @shiftkey because he made this PR and most likely has more means and understanding to answer it. There is no need to overreact, I was just wondering. I don't have dev environment installed and not familiar with benchmarking git to test it myself. But the fact that git is very high quality code that it would be less prone to this kind of attack raised my concern about if the benefit is really worth it. |
|
Let's start from the beginning:
I'd started running the test suite (this is actually easy to do, I recommend trying it out when you have a spare moment) after talking with @dscho about the proposal and around the time of opening this issue.
This adds code before and after specific methods which are vulnerable to overflowing the stack, so that you can verify at runtime that nothing unexpected has occurred. Again, this isn't added throughout the entire codebase (although we can enable that), but to address specific known risks. I mostly wanted to focus on impacting tests (that is, finding something now breaks with the flag set). I didn't have any hard data to provide at the time, but given how easy it was to enable I'm happy to listen to concerns if people spot things which are impacted. This was raised to me by a security researcher, and as we've seen in that past Git is not immune to these sorts of issues, I'd like to see us be more proactive and defensive about security risks.
Please be mindful that Git for Windows is a complex project that a lot of people use and there's only a small team of contributors available. When a question or issue is clear, concise and contains enough details, it's usually very easy for the maintainer (or even other contributors) to follow up. However, when a question or issue is unclear, it takes time and energy from the maintainer to follow up and understand the details - which means they have less time for the other work they need to do each day. I appreciate the questions and curiosity, but how questions are phrased can lead to very different reactions from the reader, because we lack the nuance of verbal communication when we're working in a textual medium like this. |
|
@dscho thanks for merging this. I couldn't get to the CI build artefacts but I'll have a look at the test suite this week and see if I have anything further to add. |
|
@shiftkey sorry for barging in, but IMO the question was concise, clear, and legitimate, considering the potential scope of the impact from this change. So the bottom line is that the performance impact was not evaluated, but it might if users complain about performance regressions? Maybe off topic: is there a performance test suite for git or for git for windows? |
|
@avih I don't believe the question was directed correctly, as @dscho replied instead of me. That's where the confusion started.
There's a number of tests in Git core which are focused on performance: https://github.com/git-for-windows/git/tree/master/t/perf Here's me running the Git for Windows test suite on my underwhelming Surface Pro 2 which has Windows 10 64-bit Build 10240 installed. You can do the same if you're in Git Bash (which should give you all the tools to build Git from scratch): Which returns these numbers - each line is the classic Unix
Partial Bitmap seems to be the only exceptional change here. /me shrugs |
|
That's good info. I agree that other than the partial bitmap thing, the rest seem minor at most IMO, which is very good. Worth keeping in mind then that the partial bitmap thingy regressed ~3000% (30x slower), which is far from negligible on its own. I am, however, unable to assess how important is it to keep this specific number low, if at all. |
|
I'm going to have a look at whether that partial bitmap test is consistently slower |
@danilaml why did you not ask for that? I could easily have helped you. |
@avih nobody contested that the question was concise, clear or legitimate. Nor the scope. The issue was that some people feel completely happy to load even more tasks on others than they already do. You, for example, could have easily followed the link @shiftkey provided how to run the test suite. Instead, you chose to "barge in" and demonstrate that you are willing to ask, but not to do anything about answering the question. That would be forgivable. If I had not worked that hard to make it easy for you to answer the question. More than half my time is spent on making it easy to contribute to Git for Windows, be it new code, patches, or (as here) contributions that answer an important question. I mean, you can totally be that type of user who is complacent about their problems and expects others to solve them for you. Or you could take up the challenge, use the already formidable tools in place to work on the problems, and actually get a cozy and warm feeling that you became an Open Sorcerer. Your choice 😜 |
@avih please keep in mind that such a judgement, after having done nothing to help move this ticket along, elicits quite a few bad feelings in me. Quite a few. |
Not only this wasn't judgment, I even explicitly stated that I'm unable to judge or evaluate the importance of this value. Regardless, please accept my apology if I hurt someone's feelings. |
@avih now, if you want to give some of the time back you took from @shiftkey and myself, you will go and install the SDK, build Git, run the perf test @shiftkey pointed out, and if you can replicate that terrible slowdown then investigate what causes it. Helping that way would be the only apology I need. |
|
I don't have the capacity for that any time soon. |
|
@avih well, thank you so much. |
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
To reduce Git for Windows' attack surface, we started using the Address Space Layout Randomization and Data Execution Prevention features in ce6a158 (mingw: enable DEP and ASLR, 2019-05-08). To remove yet another attack vector, let's make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because on Windows: The latter appears to strike a better balance between the performance impact and the provided safety. In a non-scientific test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s, i.e. the performance impact was *well* lost in the noise. This fixes git-for-windows#501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
To reduce Git for Windows' attack surface, we started using the Address Space Layout Randomization and Data Execution Prevention features in ce6a158 (mingw: enable DEP and ASLR, 2019-05-08). To remove yet another attack vector, let's make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because on Windows: The latter appears to strike a better balance between the performance impact and the provided safety. In a non-scientific test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s, i.e. the performance impact was *well* lost in the noise. This fixes git-for-windows#501 Signed-off-by: Johannes Schindelin <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
As suggested privately to Brendan Forster by some unnamed person (suggestion for the future: use the public mailing list, or even the public GitHub issue tracker, that is a much better place to offer such suggestions), we should make use of gcc's stack smashing protector that helps detect stack buffer overruns early. Rather than using -fstack-protector, we use -fstack-protector-strong because it strikes a better balance between how much code is affected and the performance impact. In a local test (time git log --grep=is -p), best of 5 timings went from 23.009s to 22.997s (i.e. the performance impact was *well* lost in the noise). This fixes #501 Signed-off-by: Johannes Schindelin <[email protected]>
Reference: http://wiki.osdev.org/Stack_Smashing_Protector
This was raised to me privately recently as a suggestion to harden Git from buffer overflows. From the link above:
I've been looking into building the latest version of Git for Windows with this flag enabled, and am running through the test suite with these parameters:
This injects a stack smashing protector into "functions with vulnerable objects". There's also some tougher options for GCC that can be used here
-fstack-protector-strongand-fstack-protector-allbut I think the first one is Good Enough™.I'm not sure how upstream feels about this, but we can deal with that later.
Thoughts?