Bugzilla – Full Text Bug Listing
| Summary: | A codan checker for detecting format string vulnerabilities | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Meisam <meisam.fathi> |
| Component: | cdt-codan | Assignee: | Elena Laskavaia <elaskavaia.cdt> |
| Status: | RESOLVED FIXED | QA Contact: | Elena Laskavaia <elaskavaia.cdt> |
| Severity: | normal | ||
| Priority: | P3 | CC: | angvoz.dev, cdtdoug, vivkong |
| Version: | 7.0 | ||
| Target Milestone: | 8.0 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 174571 [details]
A checker for detecting format string vulnerabilities (+ test cases)
Created attachment 174572 [details]
A checker for detecting format string vulnerabilities (+ test cases)
Created attachment 174573 [details]
A checker for detecting format string vulnerabilities (+ test cases)
Some comments about the patch:
1) ArgumentParser should be probably CFormatStringParser
Btw did you write it yourself 100%?
2) In this code
private static String[] VULNERABLE_FUNCTIONS = {//
// list of all format string vulnerable functions
"scanf", //$NON-NLS-1$
"fscanf", //$NON-NLS-1$
"fwscanf", //$NON-NLS-1$
"wscanf", //$NON-NLS-1$
"swscanf", //$NON-NLS-1$
"sscanf" //$NON-NLS-1$
};
private static String[] FUNCTIONS_WITHOUT_BUFFER = {//
// list of all format string vulnerable functions that has one argument
"scanf", //$NON-NLS-1$
"wscanf" //$NON-NLS-1$
};
private static String[] FUNCTIONS_WITH_BUFFER = {//
// list of all format string vulnerable functions that has two
// arguments
"fscanf", //$NON-NLS-1$
"fwscanf", //$NON-NLS-1$
"swscanf", //$NON-NLS-1$
"sscanf" //$NON-NLS-1$
};
2.1) You should never duplicate stuff like this - it is prone to errors,
if you need you can always merge two arrays in one programmatically.
2.2) I am not sure what do you mean by "FUNCTIONS_WITH_BUFFER" vs "FUNCTIONS_WITHOUT_BUFFER". Difference between them is only placement
of the format argument - 0 vs 1. Not the buffer/no buffer.
2.3) It is better to make this checker parametrized - where this is entered
by user. For this purpose it is better to use combine name/argument string
such as "scanf:0", "fscanf:1" - where number indicates argument position.
There are checkers already that implement list as custom argument (CatchByReferenceChecker)
3) This code does not make much sense
if (isVulnerableFunctionExprepression(callExpression)
&& hasVulnerablaArguments(callExpression)) {
// reportProblem(ER_ID, expression,
// expression.getRawSignature());
}
I assume hasVulnerablaArguments prints the errors? Code has to be rewriten in
this case or/and functional name changed
4) There is debug printing (sysout)
5) There is TODO comment (not sure why you trowing exception there)
6) Can you please write more comments on what checkers exactly does? (In class header)
Alena, thanks for reviewing and commenting. > 1) ArgumentParser should be probably CFormatStringParser I Agree. > Btw did you write it yourself 100%? Nope, Eclipse and I did it together :) I'm working on a project similar to cdt-codan. But our C/C++ parser is buggy, so we are moving to cdt-codan. I hope to contribute more checkers in near future. > 2) In this code > private static String[] VULNERABLE_FUNCTIONS = {// > ... > ... > ... > 2.1) You should never duplicate stuff like this - it is prone to errors, > if you need you can always merge two arrays in one programmatically. Again, I agree :) > 2.2) I am not sure what do you mean by "FUNCTIONS_WITH_BUFFER" vs > "FUNCTIONS_WITHOUT_BUFFER". Difference between them is only placement > of the format argument - 0 vs 1. Not the buffer/no buffer. The difference, as you said, is the placement of format string argument. > 2.3) It is better to make this checker parametrized - where this is entered > by user. For this purpose it is better to use combine name/argument string > such as "scanf:0", "fscanf:1" - where number indicates argument position. > There are checkers already that implement list as custom argument > (CatchByReferenceChecker) I'll take a look at CatchByReferenceChecker. > 3) This code does not make much sense > if (isVulnerableFunctionExprepression(callExpression) > && hasVulnerablaArguments(callExpression)) { > // reportProblem(ER_ID, expression, > // expression.getRawSignature()); > } > I assume hasVulnerablaArguments prints the errors? Code has to be rewritten in > this case or/and functional name changed I'll fix it. > 4) There is debug printing (sysout) I'll fix it. > 5) There is TODO comment (not sure why you throwing exception there) This TODO is there because I simply don't know how you externalize strings in eclipse/cdt. Should I put them in "bundle.properties"? This TODO has nothing to do with the thrown exception. I'm throwing an exception because when hasVulnerablaArguments is invoked, either the "if" or the "else if" statement should be executed. The final "else" should never happen. I could have replaced it with a return statement. I don't know whether you (eclipse folks) have a policy for cases like this or not. Personally, I prefer to be informed when something unexpected happens, but if you have a policy to ignore cases like this, I'll adhere to it. > 6) Can you please write more comments on what checkers exactly does? (In class > header) Sure. Hopefully in the next patch. Created attachment 175464 [details]
The patch that adds a checker to codan for detecting format string vulnerability
I revised the previous patch and applied recommendations that Alena made.
Created attachment 175467 [details]
Testcases for the previous patch
This checker should be renamed to reflect that it is only for Scanf (there are other vulnerabilities related to format strings which is not scanf related) I suggest ScanfFormatStringSecurityChecker Also it should probably go to Security Vulnerabilities category which needs to be created I applied the patch with the changes from the comment above, also I turned it off by default because security is special case *** cdt cvs genie on behalf of elaskavaia *** Bug 320187 - A codan checker for detecting format string vulnerabilities [*] plugin.xml 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/plugin.xml?root=Tools_Project&r1=1.16&r2=1.17 [*] bundle.properties 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/OSGI-INF/l10n/bundle.properties?root=Tools_Project&r1=1.4&r2=1.5 [*] AutomatedIntegrationSuite.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/AutomatedIntegrationSuite.java?root=Tools_Project&r1=1.14&r2=1.15 [+] FormatStringCheckerTest.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/FormatStringCheckerTest.java?root=Tools_Project&revision=1.1&view=markup [*] plugin.xml 1.33 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/plugin.xml?root=Tools_Project&r1=1.32&r2=1.33 [+] VulnerableFormatStringArgument.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/fs/VulnerableFormatStringArgument.java?root=Tools_Project&revision=1.1&view=markup [+] CFormatStringParser.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/fs/CFormatStringParser.java?root=Tools_Project&revision=1.1&view=markup [+] ScanfFormatStringSecurityChecker.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/fs/ScanfFormatStringSecurityChecker.java?root=Tools_Project&revision=1.1&view=markup [*] bundle.properties 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties?root=Tools_Project&r1=1.11&r2=1.12 I get the following compile error in the org.eclipse.cdt.codan.core.test.AutomatedIntegrationSuite: FormatStringCheckerTest cannot be resolved to a type AutomatedIntegrationSuite.java /org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test line 58 Java Problem It seems that the second patch that contains this Testcase is not applied. Hi Meisam, I got a question from translation for this line: Context: Finds statements lead to format string vulnerability File: eclipse\plugins\org.eclipse.cdt.codan.checkers\OSGI-INF\l10n\bundle.properties What is a "Finds statement"? Should this be translated? (In reply to comment #12) > Hi Meisam, > I got a question from translation for this line: > > Context: Finds statements lead to format string vulnerability > File: > eclipse\plugins\org.eclipse.cdt.codan.checkers\OSGI-INF\l10n\bundle.properties > > What is a "Finds statement"? Should this be translated? Hi Vivan, I'm not sure that I fully understand your question. But this might be a better one: "This checker finds statements that lead to format string vulnerability. I'm sorry if I made confusion. Thanks Meisam. |
This is a patch that adds a checker to codan for detecting formatstring vulnerabilities caused by scanf like functions. e.g. int f(){ char inputstr[5]; scanf("%s", inputstr); // here return 0; }