What is wrong with this code?
Why code reviews/scans are not enough
Code reviews and SAST scans are good tools to have in your toolbox as an application security engineer to detect and prevent vulnerabilities and security issues early on. But let me show you an example why they are not enough by themselves.
What is wrong with this code?
Let’s have a look at the below code snippet. This is a simple application that has 2 routes, /login which verifies a username and password and replaces that with a JWT token, and /protectedRoute which verifies the JWT token and gives back a welcome message if authentication is successful. Before moving to the next paragraph, give yourself a few minutes to review the code, and if you want you can even run a scan using your SAST tool of choice, and see if you can find any serious vulnerabilities.
const express = require('express');
const bcrypt = require('bcrypt');
const jwt = require('jwt-simple');
const mongoose = require('mongoose');
const mongoSanitize = require('express-mongo-sanitize');
const app = express();
app.use(express.json());
app.use(mongoSanitize());
const secretKey = process.env.JWT_SECRET;
// Sample MongoDB connection URI
const mongoURI = 'mongodb://localhost:27017/my_db';
// Connect to MongoDB using Mongoose
mongoose.connect(mongoURI, { useNewUrlParser: true, useUnifiedTopology: true });
const db = mongoose.connection;
// Create a user schema
const userSchema = new mongoose.Schema({
username: String,
password: String
});
// Create a user model
const User = mongoose.model('User', userSchema);
// Route for user login
app.post('/login', async (req, res) => {
const { username, password } = req.body;
try {
// Find user by username
const user = await User.findOne({ username });
if (!user) {
return res.status(401).json({ error: 'Authentication failed. User not found.' });
}
// Compare password with hashed password
const isPasswordValid = await bcrypt.compare(password, user.password);
if (!isPasswordValid) {
return res.status(401).json({ error: 'Authentication failed. Invalid password.' });
}
// Issue JWT token
const token = jwt.encode({ username }, secretKey,'HS256');
res.json({ token });
} catch (error) {
console.error('Error occurred during login:', error);
res.status(500).json({ error: 'Internal server error.' });
}
});
// Route that requires authentication
app.get('/protectedRoute', (req, res) => {
const token = req.headers.authorization;
if (!token) {
return res.status(401).json({ error: 'Unauthorized. Token missing.' });
}
try {
const decoded = jwt.decode(token, secretKey, 'HS256');
res.json({ message: `Hello ${decoded.username}` });
} catch (error) {
res.status(401).json({ error: 'Unauthorized. Invalid token.' });
}
});
const PORT = process.env.PORT || 3000;
app.listen(PORT, () => console.log(`Server running on port ${PORT}`));
The Vulnerability
Were you able to find the vulnerability in the above code? If so, great job! If not, then let me tell you that this code has a critical vulnerability that leads to full authentication bypass. How so?
The bug is in the below line in the /protectedRouteroute:
const decoded = jwt.decode(token, secretKey, 'HS256');The problem with this is that for the decode function in the jwt-simple library, the 3rd argument is expected to be the noVerify boolean value, not the algorithm, and the current behavior of this library is that if this noVerify argument is set to anything except true the token signature is NOT verified (this is not a good approach, as we should always opt to secure by default). Hence, in the above line 'HS256' is assigned to noVerify NOT to algorithm which leads to not verifying that the signature of the JWT is valid.
NOTE: This affects the current version of jwt-simple which is 0.5.6. Hopefully, this behavior will be fixed in a future version.
How to exploit
To exploit this all you need to do is create a JWT token that is properly encoded which can be generated without knowing the JWT secret used for signing and verification, using ANY key to sign the JWT token works!
# Generate an empty key
$ echo "any_secret_works" > test_key
# Use empty key to create a fake JWT token
$ FAKE_TOKEN=$(echo {\\"username\\":\\"test1\\"} | jwt -key test_key -alg HS256 -sign - )
# Use the fake token
$ curl -H "authorization: $FAKE_TOKEN" <http://localhost:3000/protectedRoute>
{"message":"Hello test1"}How to fix
Once you have completed the hard part which is finding the vulnerability, the fix is pretty straightforward. We just need to modify the above vulnerable line to the below, where we set the 3rd argument noVerify to false.
const decoded = jwt.decode(token, secretKey, false, 'HS256');How to detect similar vulnerabilities
Whether you were able to find the vulnerability or not, I assume you would agree that this kind of vulnerability can be very easily missed in a code review or by a SAST scan, which raises the question of the best way to detect this category of vulnerabilities, especially the ones affecting authentication, authorization, or security controls related to business logic.
The best way in this case in my opinion is dynamically testing the authentication, authorization, and business logic behavior (Note that DAST and Dynamic testing are two different things). This typically is performed by pentesters (internal or external).
Point-in-time vs Continuous detection
Pentests and dynamic testing are great, but they only provide point-in-time detection. However, it is not uncommon that such vulnerabilities are introduced later while developing new features or for bug fixes. For example, Imagine if the code started as:
const decoded = jwt.decode(token, secretKey);This line is not affected by the vulnerability. But imagine if a developer decided to use the more secure algorithm HS512, hence changing the above line to:
const decoded = jwt.decode(token, secretKey, 'HS512');The above line now becomes affected by the vulnerability, and as discussed before this is very hard to detect in a code review or by SAST scans.
To avoid such issues we need to have continuous tests covering the most important security controls such as authentication and authorization. For example, we can use a testing Framework such as Robot Framework to write tests covering the negative cases for authentication of the above application, then run them as integration tests in the pipeline. Here’s an example of how the tests could look like:
*** Settings ***
Library RequestsLibrary
Library Collections
*** Variables ***
${base_url} <http://localhost:3000>
${fake_secret} any_secret_works
*** Test Cases ***
Missing JWT Token Should Be Denied
${response}= GET ${base_url}/protectedRoute expected_status=anything
Status Should Be 401
Fake JWT Token Should Be Denied
${jwt_header}= Create Dictionary alg=HS256 typ=JWT
${payload}= Create Dictionary username=test1
${encoded}= Evaluate jwt.encode(${payload}, '${fake_secret}', algorithm='HS256')
${headers}= Create Dictionary authorization=${encoded}
${response}= GET ${base_url}/protectedRoute headers=${headers} expected_status=anything
Status Should Be 401This testing suite runs 2 test cases to:
- Verify that if the JWT token is missing, access is denied.
- Verify that if the JWT token is not signed with the correct secret access is denied.
Running this for the above vulnerable code would succeed for the first test and fail for the second test as expected, potentially blocking the build as a result.
$python -m robot test_authn.robot
==============================================================================
Test Authn
==============================================================================
Missing JWT Token Should Be Denied | PASS |
------------------------------------------------------------------------------
Fake JWT Token Should Be Denied | FAIL |
Url: <http://localhost:3000/protectedRoute> Expected status: 200 != 401
------------------------------------------------------------------------------
Test Authn | FAIL |
2 tests, 1 passed, 1 failed
==============================================================================Conclusion
To make sure your application is secure, you need to rely on different activities and use different tools at different steps of the SDLC process. Each one of these activities and tools does a good job detect and prevent specific types of vulnerabilities and security issues but may miss other types, and that is why these activities and tools need to work together.
In this story, we showed an example of a vulnerability that can be easily missed by code reviews, and SAST scans, but is detected through dynamic testing and/or integration or e2e tests to illustrate how these different activities and scans work together to detect and prevent different types of vulnerabilities.
