Beginner Code Review(Part 1)

Adam C
6 min readApr 27, 2020

To improve my code review skills I decided to go over Pentesterlab’s free code review exercise. It can be found at: https://github.com/PentesterLab/cr

This will be part of a series where I describe the vulnerabilities I have found, how I found it and how to fix it. The benefit of using Pentesterlab is because it already lists the vulnerabilities and my goal was to find as many as I could then compare it against the list.

https://pentesterlab.com/exercises/codereview/course

Objective

The objective of this task was to perform a code review against a PHP application publicly available on Pentesterlab’s public repository. This review was done in order to practice a methodical approach in conducting code reviews to ensure all code is covered and inspected thoroughly.

High-Level Summary

I tasked myself with performing a code review of the PHP application. The focus of this test was to find potential vulnerabilities on the application without running the application. The overall objective was to identify potential vulnerabilities and report the findings in this blog.

When performing the code review, several critical vulnerabilities were identified. These compromise the confidentiality, integrity and availability of the server the web application is hosted on. The most important findings during this code review include:

  • Hard-coded credentials which reveal the database credentials and admin user hash.
  • Confidential files are exposed so it is possible to view the admin user hash as an external unauthenticated user.
  • Unrestricted file upload allows an attacker to upload malicious files. The sanitization on the server side is insufficient and it can potentially be exploited to gain access to the server.
  • Broken Logic on the authentication of the application. This includes two vulnerabilities that exploit the authentication tokens.

Vulnerabilities

This part of the blog will highlight the individual vulnerabilities. Not all of these may result in privilege escalation but they can still be useful to a malicious adversary.

  1. Unrestricted File Upload

Impact: 5/5
Instance: user.php, line 17
Proof of concept: file.pdf.php to bypass the regex.
Evidence:

addfile function in user.php

The regex only checks for the existence of the string “.pdf” so the filename “file.pdf.php” will be allowed.
Description: Unrestricted file uploads can allow a malicious adversary to upload a script that can be run by the server to gain remote access. This can affect the confidentiality, integrity and availability of the whole application.

2. Broken Authorization

Impact: 5/5
Instance: jwt.php, verify function, parse_json function, sign function
Proof of concept:
+ Can ignore the signature in the JWT and it can still be valid.
+ Can have a username will special characters and it will affect how the JWT is decoded.
Evidence:

jwt.php — when creating a jwt, special characters can affect the values of the token.
jwt.php — when verifying a jwt, if no signature if provided, the signature will not be checked.

There are two instances of broken authorization regarding the JWT. This can be done by having a user with special characters to adjust the values when it is decoded. For example a user with “ and , can create extra fields in the JWT.

  • See Appendix A for proof of concept code.

3. Lack of input sanitization

Impact: 5/5
Instance: Register a user with special characters.
Proof of concept: username of test”,”username”:”admin
Evidence
:

register function in user.php contains no sanitization

A user can have a username with non-alphanumerical characters. This can affect the JSON Web Tokens to allow user impersonation or directory listing.
Description: The lack of input sanitization in this instance affects other functions in the application. Namely the JWT encoding, decoding and getFiles function in user.php.

4. Exposed Confidential Files

Impact: 4/5
Instance: deploy.sql, db.php
Proof of concept: N/A
Evidence:

db.php contains the username and password.
deploy.sql contains the admin credentials hard coded.

Description: Exposing files with confidential information can result in elevation of privileges. In this case, if it was put on a live server and the database port was exposed it could be possible to connect and extract the user password hashes. As MD5 is used, it may be possible to retrieve the plaintext of the original password.

5. Hard-coded Credentials:

Impact: 4/5
Instance: deploy.sql, db.php
Proof of concept: N/A
Evidence: See the evidence for Exposed Confidential Files

6. Directory Listing

Impact: 4/5
Instance: user.php line 25–30
Proof of concept: The $user parameter can be adjusted to something malicious such as “../../../etc/”. The function will then return the files listed in “../../../etc”.
Evidence:

The function to get files in user.php
Proof of concept code.

Description: This vulnerability can allow an attacker to gain more information about the server. In addition, they may be able to enumerate places to upload a reverse shell due to the Unrestricted File Upload.

7. Cookies lacks security headers

Impact: 4/5
Instance: register.php line 9, login.php line 8
Proof of concept: Go to console and type document.cookie
Evidence:

register.php setting the cookies.
login.php setting the cookie.

Description: The secure and HTTPOnly security headers should be set. The HTTPOnly header will prevent Javascript accessing the JWT. The secure flag will prevent the JWT from being sent over HTTP.

8. Verbose Error Messages

Impact: 3/5
Instance: index.php line 24, login.php line 24
Proof of concept: N/A
Evidence:

index.php — there are no custom error messages so any errors will output excess information.

Description: Verbose error messages allow a user to enumerate how the application works. The more information given to an adversary, the easier it is for them to find points of entry.

9. Insecure password storage

Impact: 3/5
Instance: user.php, register function
Proof of concept: N/A
Evidence:

user.php — Md5 is used for storing the password when registering.

Description: The passwords are stored in MD5 without a salt. MD5 should never be used for storing passwords.

9 . Username Enumeration

Impact: 2/5
Instance: Register.php, line 12
Proof of concept: Register a user with the same username twice.
Evidence:

Register.php — Error messages for the user already existing.

Description: Usernames of the application can be enumerated by the registration functionality. This means it may be possible to enumerate existing users and then hijack these users through a mix of lack of input sanitization and broken authorization.

10. Lack of anti-automation

Impact: 2/5
Instance: login.php
Proof of concept: N/A
Evidence:

Login.php — The code for the login page.

Description:There is nothing preventing automated attacks against the application. This means it may be possible to perform brute force attacks to gain access to users.

11. HMAC is not used in creation of JWT

Impact: 2/5
Instance: jwt.php, line 17
Proof of concept: N/A
Evidence:

The signing function on jwt.php

Description: JSON Web Tokens use HMACs to sign the information so it can be trusted. Here, the data is signed through a hash and a consistent salt. It may be vulnerable to a length extension attack as well. This will be described in a future blog.

What’s next

In part 2 of this series will describe how a malicious adversary can combine these vulnerabilities to affect the confidentiality, integrity and availability of the application.

In part 3 I will describe how to fix these vulnerabilities to improve the security posture of the application.

Finally in part 4, I will describe the different methodologies that may be used to perform code reviews.

Thanks for reading!

Appendix A

To create these PoC files I extracted the relevant functions and modified them so they could work by themselves.

jwtverify.php
createjwt.php
No signature but the function has no returned an invalid signature

--

--