Transcription
09/02/2021Secure Code ReviewsDr. Michaela GreilerIncreasing your Code Review Superpower@mgreilerMore information at michaelagreiler.comhi@michaelagreiler.com1Many companies invest in Code Reviews21
09/02/2021Motivations for Code Reviews @Microsoft3Code reviews correlate with areduction of defects.Code ReviewsDo Find BugsUnreviewed code is 2X morelikely to introduce defects thanreviewed code.At Google, 80% of code reviewslead to code improvements.Sources:Fagan, Kemerer and Paulk, Tanaka et al., Bavota and Russo, Thongtanunam et al., Bacchelli and Sadowski.42
09/02/2021Not all code review feedback is equal!ReadabilitySource: Characteristics of useful code reviews: an empirical study at Microsoft, Bosu, Greiler, Bird5Code Review FeedbackBest:OK:No-go:Functional defects,Documentation,Alternatives without benefits,missing corner cases or validation,coding style & conventions,existing tech debt and refactoring,Api usage, best practicesspelling mistakesplanning and future work63
09/02/2021What to focus on duringcode reviews Is the Code Correct?Does the code do what it's supposed to? Does it handle edgecases? Is it adequately tested to make sure that it stays correcteven when other engineers modify it? Is it performant enoughfor this use case? Is the Code Secure?Does the code have vulnerabilities? Is the data stored safely? Ispersonal identification information (PII) handled correctly?Could the code be used to induce a DOS? Is input validationcomprehensive enough? Is the Code Readable?Is the code easy to read and comprehend? Does it make clearwhat the business requirements are (code is written to be readby a human, not by a computer)? Are tests concise enough? Arevariables, functions and classes named appropriately? Do thedomain models cleanly map the real world to reduce cognitiveload? Does it use consistent coding convention? Is the Code Elegant?Does the code leverage well-known patterns? Does it achievewhat it needs to do without sacrificing simplicity andconciseness? Would you be excited to work in this code? Wouldyou be proud of this code? Is the Code Inspiring?Does the code leave the codebase better than what it was?Does it inspire other engineers to improve their code as well? Isit cleaning up unused code, improving documentation,introducing better patterns through small-scale omments/2wau2x/maslows pyramid of code review/7Code Reviews Are NotOnly About Bugs15% of the commentsare about defects84
09/02/2021What arecode reviewsabout?Sources: Bosu, Greiler, Bird: Characteristics of Useful Code Reviews: An Empirical Study at Microsoft 2015 Mäntylä and Lassenius. What Types of Defects Are Really Discovered in Code Reviews? 20099Source: OWASP Code Review Guide 2.0105
09/02/2021Code Reviewsare so muchmore11126
09/02/2021Main Pain PointsSlow Turn-AroundTimeLow ReviewQuality13147
09/02/2021How do we make sure wefind important issues?15Knowcommonsecurityvulnerabilities OWASP Top 10 – Web app centric: https://owasp.org/wwwproject-top-ten/ Injection Broken Authentication Sensitive Data Exposure XML External Entities (XXE) Broken Access Control Security Misconfiguration Cross-Site Scripting (XSS) Insecure Deserialization Using Components with Known Vulnerabilities Insufficient Logging & Monitoring Code review guide: https://owasp.org/www-pdfarchive/OWASP Code Review Guide v2.pdf Common Weaknesses by MITRE: http://cwe.mitre.org/data/ 40 categories comprising 418 weaknesses Top 25 Most Dangerous Software Weaknesses168
09/02/2021Source: OWASP Code Review Guide 2.017Use a CodeReview eview-checklist189
09/02/2021Security and Data PrivacyWhat security vulnerabilities is this code susceptible to?Are authorization and authentication handled in the right way?Is (user) input validated, sanitized, and escaped to prevent security attacks such as cross-sitescripting, SQL injection?Is sensitive data like user data, credit card information securely handled and stored?Does this code change reveal some secret information like keys, passwords, or usernames?Is data retrieved from external APIs or libraries checked accordingly?Is the right encryption used?19Web SecurityPrinciplesAuthenticationConfirm something is authentic. Example:confirming the identity of a user.AuthorizationSpecify access rights to resources.Example: only Joe can view Joe's accountbalance.ConfidentialityPrevent the disclosure of information tounauthorized individuals or systems.Example: message encryption.Data / Message IntegrityData cannot be modified or corruptedwithout detection.AvailabilityWeb sites need to be available and fast.Example: many websites can boast 99.99%uptime.AccountabilityWhen a person or system accesses orchanges data their actions should betraceable. Example: loggingNon-repudiationThe ability to prove that a transactiontook place. Example: electronic receipts.2310
09/02/2021Experience32Findingvulnerabilitiesis hardSource: An Empirical Study on the Effectiveness of Security Code Review, Edmundson et. al3311
09/02/202134Understandingthe Context3512
09/02/2021What does this change accomplish?Focus onWHAT andWHY, notHOW!Why was this change necessary?Why did you come up with this solution?Have you considered alternative solutions?Why did you decide against them?36What is the entry point of your solution?Which order of files makes most sense for thereviewer?Give Contextand Lead TheReviewWould a gif, video or screenshot help understandthe change?Add comments to get a reviewer's attentionAdd comments to ask for specific feedback3713
09/02/2021I need more informationto understand the codeand give valuablefeedback.For the change I worked on,there is no need for a lengthyreview description. The contextand code are clear enough.Perspective38Change Size3914
09/02/2021Code Review Size – Feedback Quality% of useful CommentsVisual StudioOfficeWindowsNumber of Files in Change Set40Code Review Size – Feedback Quality200-400 LOC are themaximum a developer caneffectively processSource: Best Kept Secrete of Code Reviews astudy of Cisco’s code review4115
09/02/2021Large Pull Requests42Review Time4316
09/02/202144AutomationLet the tool point out issues so people don’t have to.4517
09/02/2021Linter and other tools should bepart of an automated loopIntegratingLinting on theDEVOPs cycleLinting can take time. Design toolchain to reduce interruptions andwaiting times.But make sure problems arereported before merge.46Problems reported aftermerge don’t get fixed4718
09/02/2021Automatic ScanningStrengthWeaknesses Can runs continuously with CI Finds buffer overflows, SQLInjection Flaws Helps pinpoint developers toproblematic files Isn’t good in findingauthentication, access control,or cryptography problems Reports many false positives Can only scan code (i.e., configcan be problematic if notpresent in code) Code must be compile/runnable48How to make sure we find important issues?Use a code reviewchecklistLearn about securityissuesInclude people withthe rightexpertise/experience on the reviewSet enough timeaside for a reviewReview small,incrementalchangesLearn, learn,learn4919
09/02/2021Let’s do acode review50Focus of a secure code reviewData ValidationError nticationCryptography5120
09/02/2021Data & Input Validation All data from users needs to be considered untrusted. Best practices: Exact match validator „Known good” approach (allowed list) “Known bad” approach (block list). Input data: not only user data but also HTTP headers, input fields, hidden fields, drop down lists,and other web components Check: type, length, characters. Do contextual escaping, instead of replacment Always validate on the server side (again) Use parameterized queries52Improper input validation can lead to Cross-site scripting (XSS) (CWE-79) attack SQL injection (CWE-89). Carriage Return Line Feed (CRLF) Injection (CWE-93) Argument Injection (CWE-88) Command Injection (CWE-77) Learn 5321
09/02/2021SQL Injection query "SELECT * FROM users WHERE name '{ name}'"Input: Michaela; DROP TABLE users;54Authentication Can admin accounts log-in via the web?Are failure messages for invalid usernames or passwords leak information?Are invalid passwords logged (which can leak sensitive pwd & username combis)?Are the pwd requirements (lengths/complexity) appropriated?Are invalid login attempts correctly handled with lockouts, and rate limit?Does the "forgot pwd" routine leak information, is vulnerable to spamming, or is the pwdsend in plain text via email? How and where are pwd and usernames stored, and are appropriate mechanisms suchas hashing, salts, encryption in place? More info: hentication Cheat Sheet.html5722
09/02/2021Experience the problems58Password resetroutine Notice that the resetpassword email is sent tothe email addresssupplied in the request,but not the one retrievedfrom the database.5923
09/02/2021Give awayinfo forexploitation606124
09/02/2021Learn: Language Agnostic and SpecificSecurity Code Review Guide: https://owasp.org/www-pdfarchive/OWASP Code Review Guide v2.pdf Ruby on Rails: https://rails-sqli.org/ Rails Security Guide: https://guides.rubyonrails.org/security.html Great resources: Secure Coding Practices Checklist: https://owasp.org/www-pdfarchive/OWASP SCP Quick Reference Guide v2.pdf Cheat ets/SQL Injection Prevention Cheat Sheet.html Input Validation: estions & DiscussionGet Dr. Greiler’sCode Review E-Bookincluding Code Review comhi@michaelagreiler.com6325
Use a code review checklist Learn about security issues Include people with the right expertise/experienc e on the review Set enough time aside for a review Review small, incremental changes Learn, learn, learn 48 49. 09/02/2021 20 Let's do a code review Focus of a secure code review
