A review looks for what tests miss
A secure code review is reading code, or a change to it, to look for security faults before it ships. It matters because tests check what the code should do, and a review asks what the code could be made to do.
A colleague adds a small feature on Friday afternoon. Nobody reads it closely. A reviewer with a short checklist would have found the fault in five minutes.
By the end of this lesson
Note
I can review code for security faults.
Key words
| Word | What it means |
|---|---|
| Code review | reading code to find faults before it ships |
| Checklist | a short list of questions asked of every change |
| Diff | the lines a change removes and the lines it adds |
| Finding | one fault a reviewer reports, with where and why |
| Hard-coded | written into the code instead of being given to it |
| Information leak | a message or page that tells an attacker too much |
The cells students fill in are left blank in this preview.
The one rule
Warning
The lab is a shop that is insecure on purpose. Read your own private copy and nothing else. It has no way out to the internet. Never review or probe a system that is not yours without written permission. In the UK that is a crime under the Computer Misuse Act 1990.
A checklist keeps every review the same
Reviewers miss things when they read without a plan. A checklist gives the review a shape: the same few questions asked of every change, in the same order.
It does not replace thinking. It stops a tired reviewer skipping the boring question that matters, such as who is allowed to do this.
Worked example: a six-question checklist
| Area | Ask | A red flag |
|---|---|---|
| Input | Where does outside data come in? | Used before any check |
| Queries | Is any query built from text? | A plus sign or a format string in a query |
| Secrets | Is anything private written in the code? | A password or key in a string |
| Access | Who is allowed to do this? | No check before a change or a read |
| Errors and logs | What does a failure show and record? | Internal words shown, nothing logged |
| Libraries | Is a new dependency pinned and needed? | No version, or an unknown source |
The cells students fill in are left blank in this preview.
Reading a diff, line by line
Most reviews read a diff. A line that starts with a minus sign was removed. A line that starts with a plus sign was added. Lines with neither are unchanged context.
Read the added lines first. Then ask what the removed lines used to do. A fix that removes a check can be worse than the fault it solves.
A diff that fixes a query
- rows = db.execute("SELECT name FROM books WHERE title = '" + word + "'")
+ rows = db.execute("SELECT name FROM books WHERE title = ?", (word,))
db.close()
return rows
Worked example: questions about that diff
| Question | Answer for this diff |
|---|---|
| What changed? | The word moved out of the query text into a parameter |
| Is the fix complete? | Only if no other query in the file joins text |
| Is there a test? | Ask for one that sends a quote mark |
| Did it remove a check? | No, it removed a risky join |
The cells students fill in are left blank in this preview.
A common mistake
Warning
Many people think that if all the tests pass, the code is secure. It is not. Tests check what the code should do. A review asks what the code could be made to do, with input nobody wrote a test for.
A finding says where, what, why and fix
Report each fault as a finding. A reader who was not in the review must be able to act on it.
Say where it is. Say what the fault is. Say why it matters, in one sentence about what an attacker gains. Say how to fix it. Mark how serious it is, so the team fixes the worst first.
Worked example: one finding written well
| Part | Example |
|---|---|
| Where | shop.py, line 60 |
| What | The database's own error text is sent to the visitor |
| Why | It shows an attacker how the shop is built |
| Fix | Send a plain message and write the detail to a log |
| Seriousness | Medium: it helps an attack, it is not one |
The cells students fill in are left blank in this preview.
Review three lines of the shop
Your lab shop is a small program. The next three lines print one line each that a reviewer would question. They are not the search itself. Read each and decide: is it a fault, or a choice that needs a reason?
Which address does the shop listen on?
docker exec app grep -n "0.0.0.0" /tmp/app/app.pyShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
What does the shop write to a log?
docker exec app grep -n -A1 "def log_message" /tmp/app/app.pyShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
What does the shop say when it fails?
docker exec app grep -n "database error" /tmp/app/app.pyShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Three lines, three findings to weigh
The first prints line 68. The address 0.0.0.0 means every network interface. In a container that is needed, so a reviewer writes a note, not an alarm.
The second prints lines 63 and 64: a function for logging whose body is pass. The shop writes no record at all. The third prints line 60, which sends the database's own words back. That is an information leak.