This site is the archived OWASP Foundation Wiki and is no longer accepting Account Requests.
To view the new OWASP Foundation website, please visit https://owasp.org

Difference between revisions of "How to Write an Application Code Review Finding"

From OWASP
Jump to: navigation, search
 
m (Added navigation to facilitate sequential reading online)
 
(19 intermediate revisions by 9 users not shown)
Line 1: Line 1:
−
> Personally, I find all of these lists (the SANS Top 20, the old Top 
+
{{LinkBar
−
> 10, the old Guide, etc) very negative - which is the way they were 
+
  | useprev=PrevLink | prev=Reviewing Web Services | lblprev=
−
> designed. Even the chapter headings in the new book from Howard and
+
  | usemain=MainLink | main=OWASP Code Review Guide Table of Contents | lblmain=Table of Contents
−
> LeBlanc are negative.
+
  | usenext=NextLink | next=Automated Code Review | lblnext=
−
>
+
}}
−
> A few years ago, that's how I thought, too. However, I've moved on.   
+
__TOC__
−
> Sure we need to tell people, don't do X when it is necessary, but I  
+
 
−
> think human nature works better when ideas are framed in a positive  
+
An application security "finding" is how an application security team communicates information to a software development organization. Findings may be vulnerabilities, architectural problems, organization problems, failure to follow best practices or standards, or "good" practices that deserve recognition.
−
> way. Certainly with business types who don't (yet) understand risk  
+
 
−
> properly. Read this:
+
==Choose a Great Title==
−
>  
+
When writing an application security finding, you should choose a title that captures the issue clearly, succinctly, and convincingly for the intended audience. In general, it's best to phrase the title in a positive way, such as “Add access control to business logic” or “Encode output to prevent  [[Cross-site Scripting (XSS)]]."
−
> http://www.asktog.com/columns/047HowToWriteAReport.html
+
 
−
>  
+
==Identify the Location of the Vulnerability==
−
> I write many reports which occasionally detail pretty bad news for  
+
The finding should be as specific as possible about the location in both the code and as a URL. If the finding represents a pervasive problem, then the location should provide many examples of actual instances of the problem.
−
> the recipients. Typically, they are not technical people (nor 
+
 
−
> necessarily should they be - well-written reports should be 
+
==Detail the vulnerability==
−
> understandable by lay people). Tog's essay was an eye opener for me 
+
The finding should provide enough detail about the problem that anyone can:
−
> and I wish I'd read it sooner. With my more positive approach, I'm 
+
* understand the vulnerability
−
> getting greater traction and things are getting fixed. Before, they'd 
+
* understand possible attack scenarios
−
> often go "it's all too hard, we accept this risk, next!"
+
* know the key factors driving likelihood and impact
−
>  
+
 
−
> I strongly believe we are here to enable secure business, not get in 
+
==Discuss the Risk==
−
> the way. Too many security folks* forget that we exist to make sure 
+
There is value in both assigning a qualitative value to each finding and further discussing why this value was assigned. Some possible risk ratings are:
−
> that ordinary folks don't lose money, don't see their details lost to 
+
* Critical
−
> identity thieves, and don't lose privacy. "Thou Shalt Not ..." lists 
+
* High
−
> don't really work in this "enable secure business" ideology.
+
* Moderate
−
>  
+
* Low
−
> That's why the Guide has moved from negative titles to positive or 
+
 
−
> neutral titles. I've tried as hard as I can do phrase the issue in 
+
Justifying the assigned risk ratings is very important. This will allow stakeholders (especially non-technical ones) to gain more of an understanding of the issue at hand. Two key points to identify are:
−
> terms of "This is the business reason why we check for this issue. 
+
* Likelihood (ease of discovery and execution)
−
> Check X. Do Y", rather than say "Faulty authorization. Don't do X. 
+
* Business/Technical impact
−
> It's bad. M'kay?".
+
 
−
>  
+
You should have a standard methodology for rating risks in your organization. The  [[How to value the real risk|OWASP Risk Rating Methodology]] is a comprehensive method that you can tailor for your organization's priorities.
−
> Only a few times I resorted to "don't do X" when it was truly 
+
 
−
> unavoidable and that's a few times too many. Hopefully, by Guide 2.1  
+
==Suggest Remediations==
−
> I can make it even more positive.
+
* alternatives
−
>
+
* include effort required
 +
* discuss residual risk
 +
 
 +
==Include References==
 +
* Important note: if you use OWASP materials for any reason, you must follow the terms of our license
 +
 
 +
==Sample Report==
 +
 
 +
Below is a sample format for a finding report resulting from a secure code review.
 +
 
 +
<!--
 +
/* Font Definitions */
 +
@font-face
 +
{font-family:"Microsoft Sans Serif";
 +
panose-1:2 11 6 4 2 2 2 2 2 4;}
 +
/* Style Definitions */
 +
p.MsoNormal, li.MsoNormal, div.MsoNormal
 +
{margin:0cm;
 +
margin-bottom:.0001pt;
 +
font-size:12.0pt;
 +
font-family:"Times New Roman";}
 +
@page Section1
 +
{size:595.3pt 841.9pt;
 +
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
 +
div.Section1
 +
{page:Section1;}
 +
-->
 +
 
 +
 
 +
 
 +
 
 +
 
 +
<div class=Section1>
 +
 
 +
<table class=MsoTableGrid border=1 cellspacing=0 cellpadding=0 width=631
 +
style='width:473.4pt;border-collapse:collapse;border:none'>
 +
  <tr>
 +
  <td width=631 colspan=4 valign=top style='width:473.4pt;border:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>Review /Engagement
 +
  reference:</span></p>
 +
  </td>
 +
  </tr>
 +
<tr>
 +
  <td width=631 colspan=4 valign=top style='width:473.4pt;border:solid windowtext 1.0pt;
 +
  border-top:none;padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>Package/Component/Class
 +
  Name</span></p>
 +
  </td>
 +
  </tr>
 +
<tr>
 +
  <td width=631 colspan=4 valign=top style='width:473.4pt;border:solid windowtext 1.0pt;
 +
  border-top:none;padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  </td>
 +
  </tr>
 +
<tr>
 +
  <td width=227 valign=top style='width:169.95pt;border:solid windowtext 1.0pt;
 +
  border-top:none;padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>Finding
 +
  description</span></p>
 +
  </td>
 +
  <td width=131 valign=top style='width:98.45pt;border-top:none;border-left:
 +
  none;border-bottom:solid windowtext 1.0pt;border-right:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>Location(S)</span></p>
 +
  </td>
 +
  <td width=117 valign=top style='width:88.0pt;border-top:none;border-left:
 +
  none;border-bottom:solid windowtext 1.0pt;border-right:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>Severity</span></p>
 +
  </td>
 +
  <td width=156 valign=top style='width:117.0pt;border-top:none;border-left:
 +
  none;border-bottom:solid windowtext 1.0pt;border-right:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>Recommendation</span></p>
 +
  </td>
 +
  </tr>
 +
<tr>
 +
  <td width=227 valign=top style='width:169.95pt;border:solid windowtext 1.0pt;
 +
  border-top:none;padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>No input
 +
  validation of the <b>HTTPRequest object.getID()</b> function. </span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>Lack of
 +
  input validation may make the application vulnerable to many types of
 +
  injection</span></p>
 +
  </td>
 +
  <td width=131 valign=top style='width:98.45pt;border-top:none;border-left:
 +
  none;border-bottom:solid windowtext 1.0pt;border-right:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>com.inc.dostuff.java</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>Lines 20,
 +
  55,106</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>com.inc.main.java</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>Lines 34,
 +
  99</span></p>
 +
  </td>
 +
  <td width=117 valign=top style='width:88.0pt;border-top:none;border-left:
 +
  none;border-bottom:solid windowtext 1.0pt;border-right:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>Critical
 +
  </span><b><span style='font-size:11.0pt;font-family:"Microsoft Sans Serif"'>&#9642;</span></b></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>Required
 +
  &#9633;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>Recommended
 +
  &#9633;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:"Microsoft Sans Serif"'>Informational
 +
  &#9633;</span></p>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  </td>
 +
  <td width=156 valign=top style='width:117.0pt;border-top:none;border-left:
 +
  none;border-bottom:solid windowtext 1.0pt;border-right:solid windowtext 1.0pt;
 +
  padding:0cm 5.4pt 0cm 5.4pt'>
 +
  <p class=MsoNormal><span style='font-family:"Microsoft Sans Serif"'>&nbsp;</span></p>
 +
  <p class=MsoNormal><span style='font-size:9.0pt;font-family:Arial'>It is critical
 +
  that this be addressed prior to deployment to production</span></p>
 +
  </td>
 +
  </tr>
 +
</table>
 +
 
 +
<p class=MsoNormal>&nbsp;</p>
 +
 
 +
</div>
 +
 
 +
 
 +
{{LinkBar
 +
  | useprev=PrevLink | prev=Reviewing Web Services | lblprev=
 +
  | usemain=MainLink | main=OWASP Code Review Guide Table of Contents | lblmain=Table of Contents
 +
  | usenext=NextLink | next=Automated Code Review | lblnext=
 +
}}
 +
 
 +
[[Category:How To]]
 +
[[Category:OWASP Code Review Project]]

Latest revision as of 16:55, 9 September 2010

«««« Main
(Table of Contents) 		»»»»

An application security "finding" is how an application security team communicates information to a software development organization. Findings may be vulnerabilities, architectural problems, organization problems, failure to follow best practices or standards, or "good" practices that deserve recognition.

Choose a Great Title

When writing an application security finding, you should choose a title that captures the issue clearly, succinctly, and convincingly for the intended audience. In general, it's best to phrase the title in a positive way, such as “Add access control to business logic” or “Encode output to prevent Cross-site Scripting (XSS)."

Identify the Location of the Vulnerability

The finding should be as specific as possible about the location in both the code and as a URL. If the finding represents a pervasive problem, then the location should provide many examples of actual instances of the problem.

Detail the vulnerability

The finding should provide enough detail about the problem that anyone can:

  • understand the vulnerability
  • understand possible attack scenarios
  • know the key factors driving likelihood and impact

Discuss the Risk

There is value in both assigning a qualitative value to each finding and further discussing why this value was assigned. Some possible risk ratings are:

  • Critical
  • High
  • Moderate
  • Low

Justifying the assigned risk ratings is very important. This will allow stakeholders (especially non-technical ones) to gain more of an understanding of the issue at hand. Two key points to identify are:

  • Likelihood (ease of discovery and execution)
  • Business/Technical impact

You should have a standard methodology for rating risks in your organization. The OWASP Risk Rating Methodology is a comprehensive method that you can tailor for your organization's priorities.

Suggest Remediations

  • alternatives
  • include effort required
  • discuss residual risk

Include References

  • Important note: if you use OWASP materials for any reason, you must follow the terms of our license

Sample Report

Below is a sample format for a finding report resulting from a secure code review.




Review /Engagement reference:

Package/Component/Class Name

 

Finding description

Location(S)

Severity

Recommendation

 

No input validation of the HTTPRequest object.getID() function.

 

Lack of input validation may make the application vulnerable to many types of injection

 

com.inc.dostuff.java

Lines 20, 55,106

 

com.inc.main.java

Lines 34, 99

 

Critical ▪

 

Required □

 

Recommended □

 

Informational □

 

 

It is critical that this be addressed prior to deployment to production

 


«««« Main
(Table of Contents) 		»»»»
Retrieved from "https://wiki.owasp.org/index.php?title=How_to_Write_an_Application_Code_Review_Finding&oldid=89007"