Your browser doesn't support the features required by impress.js, so you are presented with a simplified version of this presentation.

For the best experience please use the latest Chrome , Safari or Firefox browser.

Code Review

Krisztián Schäffer, HACKTIVITY 2012
Code Review in general
Quality Assurance Activity
Common practice on big projects
"White crow" in the enterprise
Mature tools and processes
Every change is reviewed before commit
 Security Code Review
CVE 2012-2122
MySQL/MariaDB auth bypass
$ for i in `seq 1 1000`;
do mysql -u root
2>/dev/null; done
typedef char my_bool;

my_bool check_scramble(const char *scramble_arg,
            const char *message,
            const uint8 *hash_stage2)
  SHA1_CONTEXT sha1_context;
  uint8 buf[SHA1_HASH_SIZE];
  uint8 hash_stage2_reassured[SHA1_HASH_SIZE];

  .... Hashing stuff ...

  return memcmp(hash_stage2,
typedef char my_bool;
static my_bool check_pwd(char *given_hash,
                      char *stored_hash)
  return memcmp(given_hash, stored_hash, 8);
int main()
  char *given =  "\x00 xxxxaaa";
  char *stored = "\xff aaaaaaa";
  printf("memcmp: %d\n",
      memcmp(given, stored, 8));
  printf("check : %d\n",
      (int)check_pwd(given, stored));
  return 0;
$ gcc -Wall -pedantic
Clang says nothing, but
Splint 3.1.2
mysqlproba.c: (in function
mysqlproba.c:7:10: Return
value type int does not match
declared type my_bool:
memcmp(given_hash, stored_hash, 8)
Types are incompatible.
- my_bool schould be defined as int.
- But the main problem is the mental transformation between two 'equivalent' expressions:
memcmp(a,b) <=> memcmp(a,b) != 0
This is 'C style', but the problem is universal.
* CVE-2012-2122 checker, Joshua J. Drake
int main(void) {
  while (1) {
    one = rand();
    two = rand();
    ret = memcmp(&one, &two, sizeof(int));
    if (ret < -128 || ret > 127)
    ... abort if enough time spent ...
  printf("Vulnerable! memcmp returned: %d\n",
∃ a,b: memcmp(a,b) < -128
 || memcmp(a,b) > 127
∃ a,b: (char)memcmp(a,b) == 0
       && memcmp(a,b) != 0
(char)-129 == 127
(char)-130 == 126
(char) 128 == -127
(char) 129 == -126
Why unexplored so long?
Research goal is to find vulnerabilities, not weaknesses.
"There is no such thing as crash or DoS, there are vulns YOU cannot exploit."
Code Audit vs. b/g box
Attack simulation
Vulnerability types
-> Hybridize
Focus on
- Most critical vulns
- Easy to find vulns
- Code coverage
Time/money, business needs matter
Easy to forget it during the work
and run for the interesting vulns.
Threat model will
help focusing
Ask for it or DIY
- Attack surface
- Threats
- Assets
Attack tree, DFD, ...
- Fagan: Heavyweight
- Timeboxed w or w/o
dev support
- Face to face: ideal
for low budget
- Fully automated: Just like automated hacking
- Control flow sensitive
- Data flow sensitive
- Top-down
Starting points
- Input
- Candidate points:
grep, static analyser, security functionality,
trust boundary etc.
- Top of the file
What is a finding?
Vuln: You have to prove
Weakness: Too many?
You as auditor are an
expensive resource so
don't waste your time!
public void serialize(ProtocolObject object,
 Writer output) throws ProtocolException
try {
 AuthorizationRequest authorizationRequest = ...
 output.write("<?xml version=\"1.0\" ?>");
 output.write("<authz-request version=\"0.1\">");
 output.write("   <subject>");
 output.write("      <subject-id>"+
 output.write("   </subject>");
 output.write("   <resource>");
 output.write("      <resource-id>"+
string key = username +
SHA512 sha = SHA512.Create();
string authToken = Convert.ToBase64String(
Regular review is part
of good QA - do it!
Develop security culture
Findig a weakness is a victory, not shame on the developer
Identify, separate and audit high risk components
Use tools to have data and metrics
- Inspection rate (LOC/hour)
- Defect rate (defect/hour)
- Defect density (defect/kLOC)
These tell more about the review than the code!
static analysis
Have a secure coding codex and review checklist per language
Buy one or start with a naked platform codex and tune it in iterations
public static Class<?> findClass(String name) ...
 try {
  ClassLoader loader = Thread.currentThread().
  if (loader == null) {
   // can be null in IE (see 6204697)
   loader = ClassLoader.getSystemClassLoader();
  if (loader != null) {
   return Class.forName(name, false, loader);
 } catch (ClassNotFoundException exception) {
  // use current class loader instead
 } catch (SecurityException exception) {
  // use current class loader instead
 return Class.forName(name);
crypto k.o$ cat README
#@+node:xxx.20060802163624:@thin README
This is an experiment to use Literate Programming in the X project. All files in the crypto package are generated from the single crypto.leo file. You are encouraged to edit this file with Leo, instead of editing the derived java files.
#@-node:xxx.20060802163624:@thin README
Thank you!
Krisztián Schäffer
Made with impress.js
if (orgUnit == null || template == null ||
      !(currentWorker.isOwner() ||
      currentWorker.isSecurity() ||
      currentWorker.isResolver(orgUnit) ||
      currentWorker.isTopResolver() ||
      currentWorker.isLeader()) ||
      rightApplication == null)
    <%@ include file="/include/err_authz.jsp"%>