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
Code Audit of Ghidra #382
Comments
Thank you for running static analyzer. If you want to help, could you please to filter all errors and warnings (remove false positives, styles errors, for example). Also if it's not a hassle for you, could you attach more usable format of report? |
I have re-submitted as comments - with lines for reference in each file that is noted as an error or warning in our custom made java security scanner. Only errors and warnings are submitted thus far. If you want more info, contact me. |
Can you please wrap the text in '```' so that it doesn't get wrapped as badly |
So perfect to read it . |
@mlarmie-g2, are you understand, that you are doing a disservice? I repeat my sentence: if you really want to help the project, then could you please to filter output (I myself can remove all lines by regexp too) of your custom made java security scanner. There are many security scanners (including open source), which I can configure and run more precisely and get more clear output without your help. |
‘GPL\DemanglerGnu\src\demangler_gnu\c\cp-demangle.c,5414,warning,Obsolete function 'alloca' called. In C99 and later it is recommended to use a variable length array instead.’ ‘GPL\DemanglerGnu\src\demangler_gnu\c\cp-demangle.c,5415,warning,Obsolete function 'alloca' called. In C99 and later it is recommended to use a variable length array instead.’ |
If that is the case, this process should have been done far before this code was even made public. We are helping to figure out why there is a lot of code that isn't even being used, or redundant, and reducing the attack surface since anything open source can be used both offensive and defensive and some code can be used to weaponize other tools. |
Thanks for the input. Running software evaluation tools can be helpful but frustrating without access to the particular tool as issues are resolved. I've summarized the issues I see, without a judgement of which are the most important to fix. It appears from my quick perusal the majority of the issues are: The variables not initialized are normally not a concern in JAVA, however I tend to initialize them by default. I'm not normally a C++ developer. In your evaluation, which are the most egregious or worth the fix. |
I made an effort to go through these. The scanner seemed to be following all possible conditional compilation paths and picked up code that doesn't make it into the build. The alloca() code in particular is only used if the compiler doesn't provide variable length arrays. The errors described as out of bounds shifting seem to be due to the scanner mistakenly thinking uint8 is a 32-bit integer. The uninitialized member warnings are good general advice but ignore the established initialization patterns for the various objects. We do do tests for code execution that depends an uninitialized values via valgrind, so I'm setting these aside. That leaves the following:
The PcodeInjectLibrarySleigh warning is a good find, it looks like an oversight during a refactor of the class. All of these are all reasonable suggestions (none of them are bugs), so I've gone ahead and made changes to address them. |
Changes committed to master. |
Thanks for addressing these issues, I was not sure how to log them but I'm glad your team took the time to go through the findings! |
Code Review audit of the source code of Ghidra. Attached is the report with the file issues, and lines of code, with brief description of the issues seen.
ghidra.pdf
The text was updated successfully, but these errors were encountered: