Skip to content

Issue #82 Adds -d and -V flags, Add unsupported RFG notice#83

Open
bonafideduck wants to merge 2 commits intotrailofbits:masterfrom
bonafideduck:issue-82
Open

Issue #82 Adds -d and -V flags, Add unsupported RFG notice#83
bonafideduck wants to merge 2 commits intotrailofbits:masterfrom
bonafideduck:issue-82

Conversation

@bonafideduck
Copy link

@bonafideduck bonafideduck commented Oct 14, 2020

#82

  • Adds a -d flag to the output for those that don't want to pipe through a JSON formatter.
  • Documents the -V flag.
  • This adds a warning to the RFG test that Microsoft never released this capability.

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@woodruffw woodruffw self-requested a review October 14, 2020 22:10
@woodruffw
Copy link
Contributor

Thanks @bonafideduck! I'll take a look at this in a bit.

@bonafideduck
Copy link
Author

It seems to be failing in the tests. I did my work on linux, so perhaps there is a difference in the compilers. My c++ is not strong, so perhaps it will be obvious to you.

constexpr const char kRFGDescription[] =
"Binaries with RFG enabled have additional return-oriented-programming "
"protections.";
// https://www.techrepublic.com/article/windows-10-security-how-the-shadow-stack-will-help-to-keep-the-hackers-at-bay/
Copy link
Contributor

Choose a reason for hiding this comment

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

This link is pretty light on details, consider replacing it with this one: https://xlab.tencent.com/en/2016/11/02/return-flow-guard/

Also, please put it above the variable declaration 🙂

"Binaries with RFG enabled have additional return-oriented-programming "
"protections.";
// https://www.techrepublic.com/article/windows-10-security-how-the-shadow-stack-will-help-to-keep-the-hackers-at-bay/
"(This was never released by Microsoft). Binaries with RFG enabled have "
Copy link
Contributor

Choose a reason for hiding this comment

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

NIt: Remove the parens and put this sentence at the end of the description.

};
}

const std::string Checksec::detailedReport() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we currently want this functionality: it'll be a little unwieldy to maintain (since we don't currently have a nice way to just enumerate all of our checks), and its existence on the main Checksec class is another place where we fail to isolate with CLI from the C++ public API.

I'm happy to merge the rest of this PR, though.


const std::string Checksec::detailedReport() const {
json j = toJson();
std::string s = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you mentioned not being experienced with C++: the idiomatic way to do this kind of function is with a std::stringstream, which you can then feed chunks with the stream-style << operator. You can then pull the ultimate std::string out of the stream with std::stringstream::str().

@woodruffw
Copy link
Contributor

Thanks for submitting this. I think we're going to pass on the -d functionality for now, at least until Winchecksec provides lower-maintenance APIs for implementing it. Otherwise, your changes look good!

@bonafideduck
Copy link
Author

It is your tool. So I wont push hard.

I wish you would reconsider. Although visible in the json, statements like something not being possible on 64 bits in an easily readable format saves a lot of time.

@woodruffw
Copy link
Contributor

I wish you would reconsider. Although visible in the json, statements like something not being possible on 64 bits in an easily readable format saves a lot of time.

Yeah, I'm sympathetic. I do think this is a feature we'd like in the medium-term, but the current Checksec API is misdesigned in a few places that make me wary of adding it as-is.

To sum them up:

  • JSON generation is currently part of the API/ABI for the Winchecksec library, which means that library consumers have a public ABI dependency on our vendored version of nlohmann::json. It should really just be part of the CLI (i.e., completely encapsulated in main.cpp).

  • We currently don't have a clean way to enumerate/iterate over all Checksec checks, since each is implemented as a member function. We should really replace the current operator json() weirdness with something that returns an STL container for simple iteration (probably something like std::vector<std::tuple<std::string, MitigationReport>>). From there, a "detailed" output mode would be pretty simple.

@bonafideduck
Copy link
Author

Hi, I apologize for not updating this PR. I was waiting for a response from Zoom Legal about the Contributor License Agreement. They said that this was a bit too broad of an agreement in that I wasn't just signing for this action, but the actions of anyone in Zoom. Legal said they'd be willing to discuss a less broad license.

@woodruffw
Copy link
Contributor

@bonafideduck No problem! I'll raise this internally today and see if we can come up with a solution.

@dguido
Copy link
Member

dguido commented Oct 26, 2020

@bonafideduck I think there may be some confusion here. This CLA should only apply to you, individually, not your entire company. Can you have them email me @ dan@trailofbits.com and I can sort it out? Thanks!

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