-
Notifications
You must be signed in to change notification settings - Fork 939
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
Anomalies scanner #570
Anomalies scanner #570
Conversation
const PeCoffSection *lastSec = (nSecs) ? getPeSection(nSecs - 1) : nullptr; | ||
if (epSec == lastSec) | ||
{ | ||
anomalies.emplace_back(std::make_pair<std::string, std::string>("epInLastSec", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two remarks concerning the use of std::make_pair()
and emplace_back()
:
- When using
std::make_pair
, you are not supposed to specify the template parameters (see e.g. this SO answer). The whole point ofstd::make_pair()
is that you don't have to specify the template parameters. Otherwise, you could just as well createstd::pair
directly. - There is no benefit of using
emplace_back()
with r-values (you could call justpush_back()
). The benefit ofemplace_back()
comes from the fact that you can pass just the arguments to the constructor of the objects that a vector holds, which both simplifies the code and possibly makes it faster, as the object is created in-place.
Thus, instead of
anomalies.emplace_back(std::make_pair<std::string, std::string>("x", "y"));
write just
anomalies.emplace_back("x", "y");
void PeFormat::scanForImportAnomalies() | ||
{ | ||
// scan for import stretched over multiple sections | ||
for(auto&& impRange : formatParser->getImportDirectoryOccupiedAddresses()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a forwarding reference in this case? Wouldn't const auto&
suffice?
for(const auto& impRange : formatParser->getImportDirectoryOccupiedAddresses())
As Howard Hinnant puts it, liberate use of auto&&
results in so-called confuscated code: code that unnecessarily confuses people.
Similar piece of code appears in PeFormat::scanForExportAnomalies()
.
void scanForResourceAnomalies(); | ||
void scanForImportAnomalies(); | ||
void scanForExportAnomalies(); | ||
void scanForOptHeaderAnomalies(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TABs
{ | ||
if (std::find(duplSections.begin(), duplSections.end(), name) == duplSections.end()) | ||
{ | ||
anomalies.emplace_back("unusualSecName", "Unusual section name: " + name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape section names because you might end up with gibberish output.
{ | ||
if (std::find(duplSections.begin(), duplSections.end(), name) == duplSections.end()) | ||
{ | ||
anomalies.emplace_back("packedSecName", "Packer section name: " + name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape section names because you might end up with gibberish output.
auto characIt = usualSectionCharacteristics.find(name); | ||
if (characIt != usualSectionCharacteristics.end() && characIt->second != flags) | ||
{ | ||
anomalies.emplace_back("unusualSecChar", "Section " + name + " has unusual characteristics"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape section names because you might end up with gibberish output.
// scan size over 100MB | ||
if (sec->getSizeInFile() >= 100000000UL) | ||
{ | ||
anomalies.emplace_back("largeSec", "Section " + msgName + " has size over 100MB"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape section names because you might end up with gibberish output.
{ | ||
if (std::find(duplSections.begin(), duplSections.end(), name) == duplSections.end()) | ||
{ | ||
anomalies.emplace_back("duplSecNames", "Multiple sections with name " + name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape section names because you might end up with gibberish output.
(cmpSecStart <= secStart && secStart < cmpSecEnd)) | ||
{ | ||
const std::string cmpMsgName = (cmpName.empty()) ? numToStr(cmpSec->getIndex()) : cmpName; | ||
anomalies.emplace_back("overlappingSecs", "Sections " + msgName + " and " + cmpMsgName + " overlap"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape section names because you might end up with gibberish output.
} | ||
} | ||
|
||
anomalies.emplace_back("stretchedImp", "Import " + msgName + " is stretched over multiple sections"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape import names because you might end up with gibberish output.
} | ||
} | ||
|
||
anomalies.emplace_back("stretchedExp", "Export " + msgName + " is stretched over multiple sections"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape export names because you might end up with gibberish output.
void PeFormat::scanForObsoleteCharacteristics() | ||
{ | ||
std::size_t flags = getFileFlags(); | ||
std::size_t obsoleteFlags = PELIB_IMAGE_FILE_LINE_NUMS_STRIPPED | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you take this anomaly from? I am just curious whether each of this flag is really obsolete. https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-_image_file_header says only few of the ones you 've listed are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have adopted it directly from source codes of PortEx. The link you posted seems to ommit the fact that the flags are indeed obsolete (see flags documentation referenced by PE documentation ). I checked whether the Obsolete Flag
anomaly pops on binary I have created with VB6 compiler and it does. Consider neglecting the flags that turned obsolete "recently".
#415
I've implemented PE scanner for format anomalies.
PE format is scanned by: