1 / 46

Code Reviews

Code Reviews. James Walden Northern Kentucky University. Topics. Types of Reviews Code Review Process Checklists Prioritizing Code to Review. Code Reviews. Inspection of source code by one or more people who aren’t the author of the code. Goal: Identify defects for later removal.

ciqala
Download Presentation

Code Reviews

An Image/Link below is provided (as is) to download presentation Download Policy: Content on the Website is provided to you AS IS for your information and personal use and may not be sold / licensed / shared on other websites without getting consent from its author. Content is provided to you AS IS for your information and personal use only. Download presentation by click this link. While downloading, if for some reason you are not able to download a presentation, the publisher may have deleted the file from their server. During download, if you can't get a presentation, the file might be deleted by the publisher.

E N D

Presentation Transcript

  1. Code Reviews James Walden Northern Kentucky University

  2. Topics • Types of Reviews • Code Review Process • Checklists • Prioritizing Code to Review CSC 666: Secure Software Engineering

  3. Code Reviews Inspection of source code by one or more people who aren’t the author of the code. • Goal: Identify defects for later removal. • People: Moderator, reviewers. • Scope: Module or small set of classes. • Time: 1-2 hours; <1kloc CSC 666: Secure Software Engineering

  4. Benefits of Code Reviews • Find defects sooner in the lifecycle. (IBM finds 82% of defects before testing.) • Find defects with less effort than testing. (IBM—rev: 3.5 hrs/defect, test: 15-25 hrs/defect.) • Find different defects than testing. (Can identify design and requirements problems too.) • Educate developers about security bugs. (Developers frequently make the same mistakes.) CSC 666: Secure Software Engineering

  5. PCI DSS 6.3.7.b Obtain and review policies to confirm that all custom application code changes for web applications must be reviewed (using either manual or automated processes) as follows: ƒ Code changes are reviewed by individuals other then the originating code author, and by individuals who are knowledgeable in code revie techniques and secure coding practices. ƒ Code reviews ensure code is developed accordin to secure coding guidelines such as the Open Web Security Project Guide (see PCI DSS Requirement 6.5). ƒ Appropriate corrections are implemented prior to release. ƒ Code review results are reviewed and approved by management prior to release. CSC 666: Secure Software Engineering

  6. Inspections • Most formal process. • Thorough coverage with separated roles. • Use checklists to focus on specified goals. • Collect metrics to track defects. • Determine whether further inspections of revised software needed at end of meeting. • Extensive documentation of effectiveness. • One study found 16-20 defects/kloc with inspections compared with 3 defects/kloc in informal walkthrough. CSC 666: Secure Software Engineering

  7. Roles in Reviews • Moderator Manages meeting; follows up on issues. • Reader Paraphrases code during meeting. Not the author. • Recorder Records bugs discovered. • Author Provides context for code; answers questions. Makes corrections after code review. CSC 666: Secure Software Engineering

  8. Walkthroughs • Less formal process. • Author leads meeting and describes code. • Focus on author needs, not quality goals. • No checklists used or metrics gathered. • Quality varies widely. • Walkthrough quality depends solely on author. • Useful for educating developers about code, provide high level view of design and defects. CSC 666: Secure Software Engineering

  9. Follow-up Planning Meeting Rework Prep Author Moderator Author Moderator Everyone Author Everyone Code Review Process CSC 666: Secure Software Engineering

  10. Follow-up Planning Meeting Rework Prep Author Moderator Author Moderator Everyone Author Everyone Planning • Author initiates Planning once code ready. • A Moderator is assigned to inspection. • Author and Moderator assemble inspection pkg. • Moderator identifies other participants. CSC 666: Secure Software Engineering

  11. Follow-up Planning Meeting Rework Prep Author Moderator Author Moderator Everyone Author Everyone Preparation • Reviewers examine inspection package. • Reviewers use checklists and analysis tools. • Reviewers mark bugs found. CSC 666: Secure Software Engineering

  12. Follow-up Planning Meeting Rework Prep Author Moderator Author Moderator Everyone Author Everyone Meeting • Reader describes code in own words. • Reviewers comment and ask questions. • Recorder notes all potential bugs, suggestions. • Team appraises code at meeting conclusion. CSC 666: Secure Software Engineering

  13. Follow-up Planning Meeting Rework Prep Author Moderator Author Moderator Everyone Author Everyone Rework Author addresses issues recorded at meeting. CSC 666: Secure Software Engineering

  14. Follow-up Planning Meeting Rework Prep Author Moderator Author Moderator Everyone Author Everyone Follow-up • Moderator meets with Author about rework. • Moderator verifies all changes made correctly. • Author checks in corrected code. CSC 666: Secure Software Engineering

  15. Formality Spectrum CSC 666: Secure Software Engineering

  16. Code Review Tips • Know your limits. Typical review speed is 150-200 lines/hour. Limit meeting length to 1-2 hours. • Know what bugs to look for. Checklists Static analysis tools • Use tools. Simple tools: grep, findstr Code viewers Static analysis tools • Require preparation before the meeting. CSC 666: Secure Software Engineering

  17. Checklists Security reviews should include checklists of common problems, including: • SQL injection • Cross-site scripting • Input validation bugs • Checking return values • Resource name canonicalization • Race conditions CSC 666: Secure Software Engineering

  18. Code Review Problems • Requires substantial expertise in area of programming and security to be effective. • Human readers are fallible and will miss mistakes. • Code reviews are slow. Unreviewed legacy code will take time to review. CSC 666: Secure Software Engineering

  19. Prioritizing Code If you can’t review everything, review • Code that runs with privileged mode. • Code that listens on globally accessible sockets. • Code that is accessible w/o authentication. • Code with a history of vulnerabilities. • Code that handles sensitive data. • Complex code. • Code that changes frequently. CSC 666: Secure Software Engineering

  20. Reviewing for SQL Injection CSC 666: Secure Software Engineering

  21. Ex: Zune infinite loop on 12/31/08 year = 1980;while (days > 365) {   if (IsLeapYear(year))  {       if (days > 366)  {           days -= 366;           year += 1;       }    } else {       days -= 365;       year += 1;   }} CSC 666: Secure Software Engineering

  22. Key Points • Roles • Moderator • Reader • Recorder • Author • Process • Planning • Preparation • Meeting • Re-work • Followup CSC 666: Secure Software Engineering

  23. References • Brian Chess and Jacob West, Secure Programming with Static Analysis, Addison-Wesley, 2007. • Michael Howard, “A Process for Performing Security Code Reviews.” IEEE Security & Privacy, July 2006. • Eoin Keary et. al., OWASP Code Review Guide 1.1, http://www.owasp.org/index.php/Category:OWASP_Code_Review_Project, 2008. • Steve McConnell, Code Complete, 2/e, Microsoft Press, 2004. • Gary McGraw, Software Security, Addison-Wesley, 2006. • PCI Security Standards Council, PCI DSS Requirements and Security Assessment Procedures, v1.2, 2008. • Karl Wiegers, Peer Reviews in Software, Addison-Wesley, 2002. CSC 666: Secure Software Engineering

  24. Static Analysis James Walden Northern Kentucky University

  25. Topics • Why Static Analysis? • False Positives and Negatives • Static Analysis Internals • Using the Tools CSC 666: Secure Software Engineering

  26. What is Static Analysis? Static = without program execution • Includes everything except testing. • Usually used to refer to compiler type tools. Examples • Static type checking • Vulnerability detection tools • Formal methods CSC 666: Secure Software Engineering

  27. Why Static Analysis? • Code reviews require substantial expertise in secure programming. • Human readers are fallible and will miss mistakes. • Code reviews are slow. Unreviewed legacy code will take time to review. CSC 666: Secure Software Engineering

  28. Verification Techniques Formal Verification Static Analysis Assurance Code Review Security Testing Penetration Testing Cost CSC 666: Secure Software Engineering

  29. False Negatives and Positives False Positives • Tool reports bugs in code that aren’t there. • Complex control or data flow can confuse tools. False Negatives • Tool fails to discover bugs that are there. • Code complexity or lack of rules to check. CSC 666: Secure Software Engineering

  30. False Negatives and Positives False Positives Mistakes False Negatives Check Heuristics CSC 666: Secure Software Engineering

  31. Static Analyis Approaches • Standard compiler warnings and type checking. • Lexing source checkers that look for bad names like strcpy() and gets(). • Parsing source code checkers. • Parsing checkers with annotations. • Formal proof based program verification. CSC 666: Secure Software Engineering

  32. Static Analysis Internals • Parser • Model Builder • Analysis Engine CSC 666: Secure Software Engineering

  33. Parser • Convert programming language to AST. • Must have a parser for each language that tool supports. Abstract Syntax Tree CSC 666: Secure Software Engineering

  34. Control Flow Graph if(a > b) nConsec = 0 s1 = getHexChar(a) s2 = getHexChar(b) return nConsec CSC 666: Secure Software Engineering

  35. Data Flow with SSA Source Code: if (bytesRead < 8) { tail = (byte) bytesRead; } SSA Form: if (bytesRead1 < 8) { tail2 = (byte) bytesRead1; } tail3 = φ(tail1, tail2); CSC 666: Secure Software Engineering

  36. Taint Propagation Track flow of data from source to sink. • Source: where data comes into program. • Sink: function that consumes the data. Vulnerabilities reported if • Data comes from an untrusted source. • Data consumed by a dangerous sink. • No function between source and sink makes the data safe. CSC 666: Secure Software Engineering

  37. Tainting SQL Injection Example $link = mysql_connect($DB_HOST, $DB_USERNAME, $DB_PASSWORD) or die ("Couldn't connect: " . mysql_error()); mysql_select_db($DB_DATABASE); $username = $_GET[‘username’]; $password = $_GET[‘password’]; $query = "select count(*) from users where username = '$username' and password = '$password'"; $result = mysql_query($query); Source Source Sink CSC 666: Secure Software Engineering

  38. Local vs. Global Analysis Local Analysis: Analysis of an individual function, a.k.a. intraprocedural analysis. Global Analysis: Follows control and data flow between functions, a.k.a. interprocedural analysis. CSC 666: Secure Software Engineering

  39. Rules Security knowledge base for tool. • Identify data sources. • Identify data sinks. • Model behavior of validation functions. • Check for dangerous configurations. • Check control flow (i.e. every lock released.) Customize for process + project • Check coding style is obeyed. • Check for custom functions, standards. CSC 666: Secure Software Engineering

  40. Static Analysis Tools Simple search (lexing) tools • Flawfinder • ITS4 • RATS Parsing Tools • Fortify Source Code Analyzer • Coverity Prevent • Klocwork K7 Suite • FindBugs • splint CSC 666: Secure Software Engineering

  41. Using the Tools Who runs the tools? • Developers • Security team When do you run the tool? • While code is being written (IDE integration) • Before code check-in • After each build • After major milestones What do you do with the results? • Support code review process. • Support security metrics. • Use to decide if project should be released. CSC 666: Secure Software Engineering

  42. Review Results Run Tool Fix Bugs Update Rules Code Reviews Review Code CSC 666: Secure Software Engineering

  43. Static Analysis Metrics • Vulnerability density (vulns/KLOC) • Vulnerabilities divided by severity • Critical, high, medium, low • Vulnerability types • Injection, XSS, race conditions, etc. • Vulnerability dwell • How long bug remains in code after detection. • Audit coverage • Percentage of code covered by reviews. CSC 666: Secure Software Engineering

  44. Evolution of a Single Project CSC 666: Secure Software Engineering

  45. Key Points • Static Analysis • Source code needed, but not execution. • Fast, repeatable, objective analysis. • Sources and Sinks • Malicious input enters program via sources. • Exploitation occurs at the sink. • Mistakes • False positives • False negatives CSC 666: Secure Software Engineering

  46. References • Brian Chess and Jacob West, Secure Programming with Static Analysis, Addison-Wesley, 2007. • Eoin Keary et. al., OWASP Code Review Guide 1.1, http://www.owasp.org/index.php/Category:OWASP_Code_Review_Project, 2008. • Gary McGraw, Software Security, Addison-Wesley, 2006. • PCI Security Standards Council, PCI DSS Requirements and Security Assessment Procedures, v1.2, 2008. • Karl Wiegers, Peer Reviews in Software, Addison-Wesley, 2002. CSC 666: Secure Software Engineering

More Related