Code Reviews in the Real World: Code Reviews in the Real World Adam Shostack adam@homeport.org


Overview: Overview Some Real World Experience Holding a review Focusing a review What to look for Training and Automation (Ewww) Software engineering Conclusions


The Real World: The Real World Reviews can be done in a time to market driven organization Cover from 500-5000 lines of code in 2 hours Reviews do provide Security wins Reliability wins


Holding a Review: Holding a Review Basic Meeting Management Have a goal. Have a moderator Distribute code in advance Bring paper copies Produce Minutes


Holding a Review (2): Holding a Review (2) Advanced Meeting management Comfy chairs Munchies STOP after two hours Comments are directed at the code, not at the coder


Targeting a Review: Targeting a Review Architectural Overview Startup Code User Interface Code Signals Logging Comments


Architectural Overview: Architectural Overview Look at the design plan first Think about how to attack Userids, passwords, encryption User interaction What is exposed by security breach


Startup Code: Startup Code Clean the environment Set it to something sane Check your location Check your permissions Parse your arguments carefully This blocks most buffer overflows


User Interface Code: User Interface Code Limited number of inputs Expected Values of inputs Don’t look for malicious, look for safe Expect the unexpected


Signals: Signals wu-ftpd bug Know how your program should handle signals Race conditions Core files


Logging: Logging Do Log Use system log facilities Log unusual happenings '/usr/sbin/sendmail 12:30 PM: bozo invoked with argument -c \055\013\330…'


Comments: Comments More than this, hopefully


What to look for: What to look for Buffer Overflows Data Parsing Race Conditions Authentication/Authorization System() Self tests


Buffer Overflows: Buffer Overflows Unbounded string functions Poor parsing of input Lead to Stack being smashed Return pointer shifts Attacker code runs locally Server or local code problem


Race Conditions: Race Conditions Usually local file issues (setuid code) Open file A, get file B open without path /tmp links stat(file); open(file) vs fopen(file), fstat(fd)


Lack of authentication/Authorization: Lack of authentication/Authorization Does the code do something where it should be handling access control? Web code is a good example Password to get in Password stored in URL (Referrer logs) Account # stored in a cookie without HMAC


Data Parsing: Data Parsing Basic paranoia about environment buffer overflows, misconfigurations input: command line environment stdin file system (what does your program do under chroot?)


System(): System() (and exec*p, exec*e, and popen) Environment path ;, andamp;andamp;, \n andamp;c Look for what you allow, deny all else.


Evidence of Tools: Evidence of Tools Makefile gcc -wall andamp; perl -w Purify andamp; electric fence lint test cases or regression testing software


Training and Automation: Training and Automation Size Revision Control Software Common Mistakes Software engineering sendmail Guidelines at http://www.homeport.org/~adam/review.html


Conclusions: Conclusions Can do in a high pressure place Excellent investment of effort