My code review journey as a Web Security Engineer
Tags: security, webdev, agile
Published: 2021-10-01 11:30:11

# Summary
- [Introduction](#introduction)
- [Objectives](#objectives)
- [How to work effectively?](#how-to-work-effectively-)
- [Input](#input)
- [Output](#output)
- [Steps](#steps)
- [High-level process flow](#high-level-process-flow)
- [RC sample code review report](#rc-sample-code-review-report)
- [Conclusion](#conclusion)

## Introduction
Code review is a part of regular development practices for any corporation. However, adding security elements to the code review is the most effective measure in preventing vulnerabilities, even before the first commit. Additionally, the code review process provides itself with sharing security best practices amongst a development team. Finally, it produces 'lessons' that we can learn to prevent future bugs.

The primary purpose of security engineers is to work collaboratively with the rest of the team to figure out a way not to reproduce the same vulnerabilities or at least implement a mechanism to minimize them.

## Objectives
By executing the steps in this guide, you will be capable to:
* Identify specific security-related flaws within the code.
* Generate a list of security issues found in the code that we should prioritize for mitigation.

## How to work effectively?

* Set a time frame for your code reviews.

To not lose track of the higher-level security vulnerabilities you are looking for, Set a reasonable time frame on your reviews and use it to keep yourself from getting stuck. If you find yourself spending too much time on one place, mark it for later review and move to the next one.


* Set clear objectives for your review.

A focused review is a helpful review. Spend time at the beginning of your review to understand the RC (Release candidate) tickets, and after that, check the bugs that are possible in the code you are reviewing.


* Review small and manageable chunks of code.
* Understand well the inputs and outputs for the code you are reviewing.
* Review only for security issues.
* Ask for help from the engineering team in case of missing comments and documentation.

## Input
The following inputs are crucial to perform an efficient code review:

* **Architecture diagram**

Use this to understand the high-level functioning of the application to help you identify possible security flaws at the very beginning and mitigate them before starting the development process.

* **Usage scenarios**

Before jumping directly to the code, you must understand the usage and the purpose by reading the content of the tickets on the RC page. Otherwise, the code will be ambiguous.

* **Data flow**

Any external inputs consider dangerous & untrusted; you should always trace the data flow from the _source_ to the _sink_.

* **Inputs and outputs**

Performing _dataflow analysis_ is necessary to know each type of input and output the codebase has; you must use static analysis tools like [SonarQube](https://sonarqube.com/) because it will be time-consuming doing this task manually.

* **Expert help**

If you find it challenging to understand the business logic or the technical solution, go to the ticket and ask for more clarification.

## Output
The code review aims to generate a list of bugs that we can fix to enhance the platform's security. 

For more visibility, your list should contain:
* Ticket ID
* The vulnerable code snippet.
* Proof of concept including exploitation and the impacts of the vulnerability.
* Recommendations that help shorten the time to remediate vulnerabilities.

![Flow](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/r23szqrathepjjkw8s2o.png)

## Steps

1. **Static Analysis Scan (SAST)**
In this step, use a static analysis tool like [SonarQube] (https://www.sonarqube.org/) to analyze the codebase, looking for flaws in these codes that may compromise security.

2. **Threat modeling**
The threat modeling process will help you understand the application and how it interacts with external entities. It includes creating use-cases to know how the application is working, identifying entry points to see where a potential intruder could interact with the application.

3. **Identify code review objectives.**
Code review objectives are a set of vulnerability types you will be looking for in the application based on its architecture and identified threats. For instance, it is not essential to look for SQL injection bugs if the application has no interactions with a database.
Examples of code review objectives:

* Ensure that all untrusted external inputs are passed to a validation routine before being used.
* Ensure that the application is built to handle all possible errors gracefully. When errors occur, the site should respond with a mainly designed result that is helpful to the user without revealing unnecessary internal details.
* Check cryptographic algorithms to ensure secrets are cleared quickly.
* Check the application routes to see how does user input maps to the application.
* Search for sensitive Keywords, i.e., token, password, select, encode, decode, sanitize, etc.
* Check every result from the SAST (SonarQube), which runs against the target codebase.
* Once you find a valid vulnerability, _perform search queries on the codebase for more issues of the same type_.

4. **Look for an entry point.**

This step will take a detailed look at the code to find as many security vulnerabilities as possible. You should use the set of goals we developed in stages 1,2 and 3 for guidance.

We should also have the following handy:
* List all the Hotspots from step 1.
* Review all the security Hotspots to identify which of the vulnerabilities discovered in the codebase require mitigation steps and which can be treated as "_false positives._" Among those requiring attention, you then need to prioritize the urgency of each vulnerability and plan the implementation of mitigation steps.
* Perform dataflow analysis.

What should you focus on analyzing the report generated from SAST?

* **Inputs** - Find all the list of inputs and then pair this up to the code you need to review. For example, you should mark all the public interfaces, UI, database interaction, socket interaction, file IO, and other areas where your application can accept data as critical for review.
* **Hard-coded strings** - Look for any hard-coded sensitive data, such as a password, cryptographic key, outbound communication to external components, or encryption of internal data, etc.
* **Error handling code** - Look for all the error handling and see if they are handled securely. Because this may expose sensitive information – sometimes leading to vulnerabilities.
* **Session Management** - Look for any weak session identifier generation, session replay, session fixation, etc.
* **Logical Attacks** - Understand the business logic of the code because the SAST can't detect things like abuse of functionality, workflow bypass ..etc.
* **Code Quality.**
* **Application Resource Handling** - LFI, XXE, L/RFI, RCE


## High-level process flow

![High Level Process Flow](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/tmfvb0l3pteywb5okm0h.png)

## RC sample code review report

- Ticket Link: [T-link]
- Release Version: [R-version]
- Assignee: [Name]

**Overview**


| #NO      | Affected URL/File | Vulnerability    | Risk Level    | Ticket ID |
| :----:       |    :----:   |         :----: |     :----:    |:----:  |
|    1   | https://codebor.com/index.php?id=       | SQL INJECTION  | <span style="color:red;font-weight: bold;">CRITICAL</span> | TICK-001|              
| 2   | https://codebor.com/page.php?c=        | XSS     | <span style="color:orange;font-weight: bold;">MEDIUM</span> |  TICK-002 |           


| #NO      | Vulnerability Class | Item Tested    | Status  |
| :----:       |    :----   |         :---- |     :----:    |
|    1   | Access Control       | <ul><li>Inadequate Auditing Controls.</li><li>Unlimited Login Attempts.</li><li>Password Complexity Policy</li></ul>    |       <span style="color:green;font-weight: bold;">PASSED</span>
| 2   | Session Management       | <ul><li>Weak Session Identifier Generation.</li><li>Session Replay.</li><li>Session Fixation</li><li>Insufficient Session Expiration.</li></ul>     | <span style="color:green;font-weight: bold;">PASSED</span>|
| 3   | Data Validation     | <ul><li><span style="color:red;">Improper Input Validation.</span></li><li><span style="color:red;">Dynamic SQL Commands.</span></li><li>Improper Output Encoding.</li><li>Format Strings</li></ul>     | <span style="color:red;font-weight: bold;">FAILED</span>|
| 4   | Application Resource Handling       | <ul><li>Path Traversal.</li><li>Predictable Object Identifiers.</li><li>XML Entity Expansion.</li><li>Local & Remote File Inclusion.</li><li>Shell command execution.</li></ul>     | <span style="color:green;font-weight: bold;">PASSED</span> |
| 5  |Cryptography       | <ul><li>Weak Algorithms.</li><li>Poor Key Management.</li><li>Insecure Data Storage.</li></ul>     | <span style="color:green;font-weight: bold;">PASSED</span> |
| 6   | Logical Attacks       | <ul><li>Abuse of Functionality.</li><li>Workflow Bypass.</li></ul>     | <span style="color:green;font-weight: bold;">PASSED</span> |
| 7   | “Hidden” Functionality     | <ul><li>Debugging Interfaces.</li><li>Undocumented Inputs.</li></ul>      | <span style="color:green;font-weight: bold;">PASSED</span> |
| 8   | Code Quality      | <ul><li>Verbose Error Messages.</li><li>Unused / Dead Code.</li><li>Improper Exception / Error Handling</li><li>Inconsistent Logging.</li></ul>     | <span style="color:green;font-weight: bold;">PASSED</span> |

## Conclusion

> "It's much more useful to think of security as being a vector to follow rather than a point to be reached. Vectors have size and direction, and you should think about the direction you want to go in pursuit of security and how fast you'd like to chase it. However it's path you will continue to walk forever."

---

You can reach me out on LinkedIn if you have questions [@Bour Abdelhadi](https://www.linkedin.com/in/in/bohr)

Do you want to support me? > [💲](https://www.paypal.com/paypalme/bohrhadi) Thanks :D