DEV Community: Mohamed AboElKheir

How Reachability Analysis 🔎 can help with open source vulnerabilities mess (Coana as an example)

Mohamed AboElKheir — Wed, 22 Jan 2025 17:30:13 +0000

If you are a security engineer or a developer, you probably already know the pain of having to deal with the vulnerabilities affecting the open-source packages (e.g. npm, pip, maven, .. etc) used by your application. In today's story, we discuss "Reachability analysis", a feature that promises to ease this pain by eliminating 70-80% of these alerts using Coana, as an example. But first, let's dig into what is wrong with pen-source vulnerability scanning.

The open-source vulnerabilities mess

Software is built using many building blocks, and for any application to be secure all the building blocks need to be secure. One of the most important building blocks is open-source packages (e.g. npm, pip, maven, .. etc) which is estimated to constitute 70-90% of modern applications according to some studies.

That is why it is important to continuously scan open source packages to check if the used versions are affected by known vulnerabilities, as these vulnerabilities could potentially lead to exploits even if the code of the application doesn't have any security issues.

Most organizations use SCA tools (e.g. Snyk, Dependabot, .. etc) to perform such scans, However, they usually run into multiple issues in practice:

The number of findings is huge and unmanageable, this is mainly because as mentioned earlier open source can be up to 90% of the actual codebase (e.g. a simple hello world application that uses Express can have more than 100 npm packages if you count the child dependencies as shown below, and this grows pretty quickly as the application gets more complex).

$ cat package.json
{
  "name": "test",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "test": "echo \\"Error: no test specified\\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "description": "",
  "dependencies": {
    "escape-html": "^1.0.3",
    "express": "^4.19.2",
    "lodash": "^4.17.20"
  }
}

$ npm list --all
test@1.0.0
├── escape-html@1.0.3
├─┬ express@4.19.2
│ ├─┬ accepts@1.3.8
│ │ ├─┬ mime-types@2.1.35
│ │ │ └── mime-db@1.52.0
│ │ └── negotiator@0.6.3
...

# This simple application has 120 npm packages
$ npm list --all | wc -l
     120

Fixing the vulnerabilities is not as straightforward as it seems for multiple reasons, e.g.:
1. The package with the vulnerability may not have a patched version yet.
2. The vulnerability affects a child package (dependency of a dependency), and the parent package doesn't have a version that uses the patched version of the child package yet.
3. The patched version of the affected package could introduce some breaking changes that need some code refactoring.
4. Even if all these issues are not present, in many cases the application usually doesn't have enough testing coverage, which means manual testing is needed to apply the fix.
These issues, along with the huge number of findings, put the development team in front of a difficult choice: either spend an unreasonable amount of time fixing and testing the findings, slowing down the development process or ignore the findings (or fix them in batches in long periods of time), which is what many teams end up doing.
As a result of this mess, some teams decide to only prioritize findings of high or critical severities, but this is usually not enough to bring down the load to a reasonable level.

Severity is not everything

Now, let's look at things from a different angle. A developer could think that if my application has 100+ critical and high open source vulnerabilities, but hasn't been hacked yet, this probably means that the severity of these vulnerabilities is not really critical or high, and they wouldn't be entirely wrong.

This discrepancy between finding severity and actual impact stems from the fact that open-source vulnerability scanners don't answer the question "Are these vulnerabilities exploitable in the context of my application?". As we are going to see shortly the answer for the vast majority of findings is "no", and this means two things:

We spend a lot of time fixing and testing vulnerabilities classified as Critical or High, but they are actually not exploitable, so they don't have any real impact.
We don't know which of these vulnerabilities are actually exploitable, which means that they may end up being ignored or at least the fix would take too long, which significantly increases the risk for our application.

Is this vulnerability exploitable?

Let's take the below application as an example:

const express = require('express');
const _ = require('lodash');

const app = express();
const port = 3000;

// Sample data
let users = [
    { id: 1, name: 'Alice', age: 25 },
    { id: 2, name: 'Bob', age: 30 },
    { id: 3, name: 'Charlie', age: 35 },
];

// Middleware to parse JSON bodies
app.use(express.json());

// Route to get all users
app.get('/users', (req, res) => {
    res.json(users);
});

// Route to get a user by id
app.get('/users/:id', (req, res) => {
    const userId = parseInt(req.params.id);
    const user = _.find(users, { id: userId });

    if (user) {
        res.json(user);
    } else {
        res.status(404).json({ error: 'User not found' });
    }
});

// Route to add a new user
app.post('/users', (req, res) => {
    const newUser = req.body;

    // Use lodash to assign an incremental id and add to users list
    newUser.id = _.maxBy(users, 'id').id + 1;
    users = _.concat(users, newUser);

    res.status(201).json(newUser);
});

// Route to update a user
app.put('/users/:id', (req, res) => {
    const userId = parseInt(req.params.id);
    const updatedData = req.body;
    const userIndex = _.findIndex(users, { id: userId });

    if (userIndex >= 0) {
        // Use lodash to merge the updated data with the existing user data
        users[userIndex] = _.merge(users[userIndex], updatedData);
        res.json(users[userIndex]);
    } else {
        res.status(404).json({ error: 'User not found' });
    }
});

// Route to delete a user
app.delete('/users/:id', (req, res) => {
    const userId = parseInt(req.params.id);
    const user = _.remove(users, { id: userId });

    if (user.length) {
        res.json({ message: 'User deleted', user });
    } else {
        res.status(404).json({ error: 'User not found' });
    }
});

// Start the server
app.listen(port, () => {
    console.log(`Server running at <http://localhost>:${port}`);
});

This application has two main dependencies express and lodash (a very popular npm package with a variety of useful functions). In this case, we are using lodash to query and update the mock user database.

Let's assume this uses the below versions of these 2 packages as shown in the below package.json file:

{
  "name": "test",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "test": "echo \\"Error: no test specified\\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "description": "",
  "dependencies": {
    "express": "^4.19.2",
    "lodash": "^4.17.20"
  }
}

Let's run the Snyk scanner on this application to check the findings

$ snyk test

Tested 66 dependencies for known issues, found 3 issues, 3 vulnerable paths.

Issues to fix by upgrading:

  Upgrade express@4.21.1 to express@4.21.2 to fix
  ✗ Regular Expression Denial of Service (ReDoS) [Medium Severity][<https://security.snyk.io/vuln/SNYK-JS-PATHTOREGEXP-8482416>] in path-to-regexp@0.1.10
    introduced by express@4.21.1 > path-to-regexp@0.1.10

  Upgrade lodash@4.17.20 to lodash@4.17.21 to fix
  ✗ Regular Expression Denial of Service (ReDoS) [Medium Severity][<https://security.snyk.io/vuln/SNYK-JS-LODASH-1018905>] in lodash@4.17.20
    introduced by lodash@4.17.20
  ✗ Code Injection [High Severity][<https://security.snyk.io/vuln/SNYK-JS-LODASH-1040724>] in lodash@4.17.20
    introduced by lodash@4.17.20

Let's focus on the High severity finding CVE-2021-23337 affecting the lodash package (Code Injection [High Severity][<https://security.snyk.io/vuln/SNYK-JS-LODASH-1040724>] in lodash@4.17.20). If this is really a “High” severity vulnerability that causes Code Injection, this means that developers should leave everything and fix this as soon as possible. However, as security engineers, it is part of our job before asking the developers to leave everything to be sure the issue actually needs such urgent action.

This takes us to the question we need to answer "Is this vulnerability exploitable in the context of my application?". To be able to answer this question, let's have a closer look at the vulnerability. If we open the link mentioned in the Snyk scan https://security.snyk.io/vuln/SNYK-JS-LODASH-1040724 we will find the PoC (Proof of concept) code showing the payload to exploit the vulnerability:

var _ = require('lodash');

_.template('', { variable: '){console.log(process.env)}; with(obj' })()

For this PoC and from the overview, it is clear that this payload works when passed to the template() function and specifically to the templateOptions.variable argument. Having a quick look at our code, we can easily see that we are not using the template() function anywhere (we are only using the find, maxBy, concat, findIndex, merge, and remove lodash functions ). This means that this vulnerability although initially classified as "High" is not exploitable in our case, and hence can be safely ignored/de-prioritized, and definitely we shouldn't be asking developers to leave everything to fix this.

Reachability analysis

The above was an example of how we can manually review and triage a finding to determine whether it is exploitable. However, this doesn't scale well as the number of findings and the application complexity increase, it is not feasible to perform the same analysis for hundreds of findings. This is where automation could come to the rescue!

In the above example the answer to the question "Is the finding exploitable?" depended on another question "Is the vulnerable function used?". This question is easier to answer as we will see shortly, and this is basically what "Reachability" is about. If the vulnerable function is used then the vulnerability is considered "Reachable", otherwise it is not.

For example, in the below application packages 2 and 3 have vulnerabilities. However, as the application only uses the vulnerable function in package 2, only package 2's vulnerability is reachable, and package 3's vulnerability is unreachable, and can be safely ignored/de-prioritized.

Coana as an example

Let's take Coana as an example, an SCA that performs Reachability analysis, and at the time of writing this article is free to use on open-source projects. Coana creates a code property graph of your application and uses this graph to determine which functions in your dependencies (and child dependencies) are actually being called, hence automating the analysis we performed earlier.

Let's run a Coana scan on the sample application we analyzed earlier by following the steps in their documentation. As shown below, it reached the same conclusion about the lodash Code injection vulnerability that it is not reachable, and in the analysis details you can see the vulnerable functions it was looking for to determine reachability.

Let's try it for an exploitable vulnerability

Let's try that again but with the below application where the same lodash vulnerability is actually exploitable.

const express = require('express');
const _ = require('lodash');
const bodyParser = require('body-parser');

const app = express();
app.use(bodyParser.urlencoded({ extended: true }));

app.post('/generate-story', (req, res) => {
    const { name, meal, place, car, options } = req.body;

    const templateString = `
        <h2>Here is your random story:</h2>
        <p><%= name %> went to <%= place %> in their <%= car %> for a nice <%= meal %>.</p>
    `;

    let templateOptions = {};
    try {
        // Parse the hidden options field (this is the vulnerable part)
        if (options) {
            templateOptions = JSON.parse(options);
        }
    } catch (error) {
        return res.status(400).send('Invalid options JSON');
    }

    try {
        // compile template without user-supplied options
        const compiled = _.template(templateString, templateOptions);
        const story = compiled({ name: _.escape(name), meal: _.escape(meal), place: _.escape(place), car: _.escape(car) });

        res.send(story);
    } catch (error) {
        res.status(500).send('Error generating story');
    }
});

app.listen(3000, () => {
    console.log('Server running on <http://localhost:3000>');
});

This code uses the template() function and also takes the templateOptions from the request body of the /generate-stroy route. Hence, can exploited with the payload in the poc, e.g. we can inject code to expose all environmental variables as shown below:

$ curl -X POST <http://localhost:3000/generate-story>
      -H "Content-Type: application/x-www-form-urlencoded"
      --data-urlencode "name=Alice"
      --data-urlencode "meal=Pizza"
      --data-urlencode "place=New York"
      --data-urlencode "car=Tesla"
      --data-urlencode "options={\\"variable\\":\\"){return JSON.stringify(process.env)}; with(obj\\"}"

{
  "CLICOLOR": "1",
  "COLORFGBG": "7;0",
  "COLORTERM": "truecolor",
  "COMMAND_MODE": "unix2003",
  "EDITOR": "vim",
  "HISTFILESIZE": "2000000",
  "HISTSIZE": "1000000",
  "HISTTIMEFORMAT": "%F %T ",
  ....
}

Now let's use Coana to scan this vulnerable example, and as expected, now the same vulnerability is shown as "Reachable". Moreover, Coana will show us the lines of code where the vulnerable function is being used, this would help us plan and test the fix.

Reachability analysis in practice

In practice, Coana's reachability analysis is usually able to discard 70-80% of the vulnerabilities as unreachable, this significantly reduces the load on security and development teams and also helps the same teams focus on the vulnerabilities that are more likely to have actual impact on the application. This removes a lot of the mess we explained earlier in this story.

Reachable != Exploitable

One thing to note is that if a finding is reachable, it doesn't necessarily mean it is exploitable, as sometimes there other conditions that need to be met for the exploitation besides the vulnerable function being called.

For example, in the vulnerable code we used above, besides using the template() function, the templateOptions argument needed to be controlled by user input and passed to the function. Hence, If we removed the options parameter from the request body in the example, and didn't pass it to template() the example no longer becomes exploitable. In this case, Coana will still mark the finding as "Reachable", and manual triage is needed to complete the analysis and decide that it is not exploitable.

The below code is an updated version where the vulnerability is "Reachable" but not "Exploitable"

app.post('/generate-story', (req, res) => {
    const { name, meal, place, car } = req.body;

    const templateString = `
        <h2>Here is your random story:</h2>
        <p><%= name %> went to <%= place %> in their <%= car %> for a nice <%= meal %>.</p>
    `;

    try {
        // compile template without user-supplied options
        const compiled = _.template(templateString);
        const story = compiled({ name: _.escape(name), meal: _.escape(meal), place: _.escape(place), car: _.escape(car) });

        res.send(story);
    } catch (error) {
        res.status(500).send('Error generating story');
    }
});

That being said, Reachability analysis still adds a lot of value by excluding the unreachable findings. The point here, is that some manual triage could help discard even more vulnerabilities that won't have impact.

Conclusion

When using security tools such as SCA scanners, it is important to remember the initial goal we are using the tool for, which is eliminating risk and preventing negative impact on your application. Hence, it doesn't matter the number of findings we are getting from these tools, if we don't have enough confidence in the quality of these findings, and how much they actually represent risk and impact. That is why features like reachability analysis are useful, as they are able to eliminate a lot of the noise and give us much more confidence that the findings we are focusing on are the ones that represent probable risk and impact.

Lessons Learned #4: One error message could expose all your data (FileSender CVE-2024–45186)

Mohamed AboElKheir — Mon, 20 Jan 2025 14:48:54 +0000

Welcome to another story in the “Lessons Learned” series where we discuss real-world vulnerabilities from the perspective of an application security engineer focusing on the underlying root causes and the measures we can take to prevent similar issues in our applications.

In today’s story, we discuss a write-up by the security researcher Jonathan Bouman showing how an SSTI (Server-side template injection) vulnerability affecting an error message was used to expose all user data of an application. You can view the full write-up here.

Affected Application

Today’s vulnerability affected FileSender, a popular open-source application mainly used for file-sharing.

Impact of the vulnerability

Exposure of credentials (database and S3) potentially allowing an unauthenticated user to access and tamper with all customer files managed by the application.

What went wrong?

The security researcher explored the /download.php route which allows unauthenticated users to download files (FileSender allows authenticated users to share download links with unauthenticated users), and found that if the download link is expired the HTTP request is redirected to another unauthenticated route like /?s=exception&exception=eyJtZXNzYWdlIjoidHJhbnNmZXJfcHJlc3VtZWRfZXhwaXJlZCIsInVpZCI6IjY3MGE4N2M0YmQ0ODAiLCJkZXRhaWxzIjpudWxsfQ==

Obviously, the exception query parameter is base64 encoded, which implies the backend decodes this and uses it to apply some logic, specifically to generate the error message and display it as shown below:

When decoding the message we get the below JSON

$ echo "eyJtZXNzYWdlIjoidHJhbnNmZXJfcHJlc3VtZWRfZXhwaXJlZCIsInVpZCI6IjY3MGE4N2M0YmQ0ODAiLCJkZXRhaWxzIjpudWxsfQ==" | base64 -d  | jq  

{  
  "message": "transfer_presumed_expired",  
  "uid": "670a87c4bd480",  
  "details": null  
}

When changing the message in the JSON to something like xxxxx, re-encoding, and calling the same route, the new message was shown between curly brackets instead of the original message, e.g. encoding the below JSON to give a new base64 string, then replacing the value of the exception query parameter with this new base64 string shows the error message {xxxxx} as shown below.

$ echo '{"message":"xxxxx","uid":"670a87c4bd480","details":null}' | base64  
eyJtZXNzYWdlIjoieHh4eHgiLCJ1aWQiOiI2NzBhODdjNGJkNDgwIiwiZGV0YWlscyI6bnVsbH0K

Backend logic

When checking the code of the application to understand the logic of how the backend decodes and handles the exception query parameter, the security researcher came across the below function.

As shown this unserialize function starts with decoding the base64 string, and then extracting the fields from the JSON (line 362).
Then it replaces any {cfg:some_option} , {conf:some_option} , or {config:some_option} values in the exception with an empty string (line 365).
This suggests that these cfg/conf/config values would be parsed by a templating engine and substituted with the value of the config option, and it seems the goal of line 365 is to prevent this from happening.
However, this only replaces these cfg/conf/config values if they are enclosed in curly brackets (e.g. {cfg:some_option}).

Exploit

The security researcher checked the documentation of FileSender and found some potentially sensitive configuration options such as db_username and db_password which hold the database credentials and also cloud_s3_key and cloud_d3_secret which hold the AWS credentials that are used to access all customer files uploaded to FileSender.
The security researcher then tried modifying the message in the JSON as shown above (decode, modify, re-encode and then send in HTTP request) to cfg:db_username (without curly brackets), and the validation step in line 365 above didn’t replace this (as it doesn’t have a curly bracket), then as we have seen earlier a set of curly brackets was added in another place in the code, leading to the templating engine now replacing the config option with its value and exposing the database username as shown below:

This allowed exposing the values of all the config values mentioned above including the database and S3 credentials using this unauthenticated route, which then could be used to expose or tamper with all customer files directly on S3.

The Fix

FileSender fixed this issue by adding validation which only allows a list of allowed exception message identifiers in the message field such as the value transfer_presumed_expired used in the original decoded string.

Lessons Learned

Keep error messages generic: This vulnerability is a strong reminder to keep error messages in your web application as generic as possible, as any extra information while helpful for troubleshooting, gives more context to any potential attacker.
Secure Design: In this case, I would argue that using a templating engine to parse the error message is a design mistake. The developers tried to compensate for this design mistake through the validation applied in line 365 above, but using validation to compensate for design issues doesn’t always work as we have seen in this case. Your best bet to catch such design mistakes is integrating threat modeling in your SDLC process. For more about threat modeling you can check my series Threat Modeling Handbook.
Input validation: Another important missing control here was input validation, the message field here should be one of a list of expected values (exception identifiers such as transfer_presumed_expired), and if the backend validated that the passed message was in fact one of these allowed values before applying any logic, this would have prevented the issue (this was also the actual fix FileSender ended up using for this vulnerability as mentioned above). Generally, input validation is one of the most powerful security controls that is usually easy to implement and is effective against a wide range of security attacks (such as SSTI in this case). You can find more details about input validation in this story I’ve posted earlier.
Client-side encryption: If you are hosting customer files, it is always a good idea to allow your customers to encrypt their files with their own keys (client-side), in addition to any server-side encryption you apply, this gives extra protection in case the files do get exposed through a vulnerability like this one. In this case, FileSender did provide a client-side encryption feature, and customers using this feature had that extra protection.
Pentests and Bug Bounty: It seems from the validation in line 365 above that the developers did know that such issue could exist but thought that the validation they had was sufficient, this is why it is important to schedule periodic pentests and/or allow bug bounty hunters to test your application to verify whether your assumptions are correct.

Conclusion

The most innocent-looking features of your application such as error messages, coupled with suboptimal design decisions and insufficient validation could lead to severe security issues. That is why developers and security engineers need to understand and discuss the implications of the design and implementation decisions as early as possible in the SDLC process and perform proper testing to confirm their assumptions are correct.

Stay tuned for another story, and another set of Lessons Learned!

Lessons Learned #3: Is your random UUID really random? (Account takeover with the sandwich 🥪 attack)

Mohamed AboElKheir — Sun, 19 Jan 2025 22:17:14 +0000

Welcome to the third story in the “Lessons Learned” series where we discuss real-world vulnerabilities from the perspective of an application security engineer focusing on the underlying root causes and the measures we can take to prevent similar issues in our applications.

In today’s story, we discuss a very interesting bug-bounty write-up showing a 0-click ATO (account takeover) using a clever technique called the Sandwich 🥪 attack, you can find the full write-up here, credit to Lupin & Holmes.

Impact of the vulnerability

As this is a bug bounty write-up the application affected wasn’t disclosed, but the impact was a 0-click ATO which means one user of the application could take over another user’s account without the victim user having to do anything, the attacker only needs to know the username or email of the victim.

What went wrong?

Like the vast majority of web applications, the affected application had a “Reset Password” functionality to help users who forgot their passwords, and like most applications, the functionality worked by following the below sequence:

Take the email of the user who forgot the password.
Verify the email exists.
Generate a random Id that corresponds to the user, and store it in the application database.
Send an email to the user with a link to the reset password endpoint including the random Id in the query parameters. e.g. https://password.application.com/token=ae0010a2-a6ed-11ef-b2c2-d26f418147d3
When the reset password endpoint gets an HTTP request it verifies the random Id exists in the database, and if it does it persists the new password for the corresponding user. Note that the security of this feature depends on the fact that the reset password random Id is long enough and has enough entropy (randomness) and hence can’t be guessed by an attacker trying to reset the password of another user to take over their account.

UUID version 1

Well, turns out this is not entirely true. The security researcher found that the application used UUID version 1 to generate the random Id used for the reset password links, and UUIDv1 relies mainly on the MAC address of the device and the timestamp of generation instead of being random. That is why if you try generating multiple uuids using UUIDv1 on your device part of the uuid will always be the same (depends on the MAC address), and the part that is different is the hexadecimal representation of the timestamp of the uuid generation.

The uuids generated using UUIDv1 have the below structure:

First 3 parts are the hexadecimal representation of the timestamp of the uuid generation.
Last 2 parts depend on the MAC address and system information, hence will always be the same if the uuid is generated on the same device.

Let’s give that a try in Python’s implementation of UUIDv1

As you can see the first part of the uuid (in red) depends on the time stamp, that is why the last 2 parts are the same in all generated uuids as they were generated within a short period of time and have close timestamps, and the second part of the uuid (in yellow) is the same for all uuids as they were all generated on the same laptop. This isn’t very random, is it?

You can also convert the first part of the UUID to a readable timestamp with a function like the one below:

from uuid import UUID,uuid1
from datetime import datetime, timedelta
def uuid1_to_datetime(uuid1_str):
    # Parse the UUID string to a UUID object
    uuid_obj = UUID(uuid1_str)
    uuid_time = uuid_obj.time
    uuid_seconds = uuid_time / 1e7

    uuid_epoch = datetime(1582, 10, 15)
    unix_epoch = datetime(1970, 1, 1)
    epoch_offset = (unix_epoch - uuid_epoch).total_seconds()

    unix_time = uuid_seconds - epoch_offset
    readable_date = datetime.fromtimestamp(unix_time)

    return readable_date

# Example usage
uuid1_str = str(uuid1()) 
print(uuid1_str)
datetime_value = uuid1_to_datetime(uuid1_str)
print("Timestamp in UUID:", datetime_value)

Which gives the below output

$ python test.py
155da2e8-a75f-11ef-b3ee-d266e835d4e1
Timestamp in UUID: 2024-11-20 11:47:09.396452

$ python test.py
aa604f0e-a763-11ef-9e7f-d266e835d4e1
Timestamp in UUID: 2024-11-20 12:19:57.381403

The Sandwich 🥪 Attack

Now what remains is how the security researcher was able to exploit this un-random uuid issue, which was a very clever attack the worked in the below sequence:

Attacker requests a reset password link for their own account, they will get the link in their email, this is the first slice of bread 🍞.
Very quickly, the attacker also requests a reset password link for the victim’s account, the attacker won’t get this link of course, this is what’s inside the sandwich 🧀.
Also very quickly, the attacker requests another reset password link for their own account, and they will also get this link in their email, this is the second slice of bread 🍞.
Now the attacker has 2 uuids for the 2 links generated for their own account, and there is another uuid for the victim account we don’t know yet, this is the one we need to find to takeover the victim account.
As all of the uuids are generated using UUIDv1 the last part (in yellow in the example above) is always the same so we can get that from either of the 2 links the attacker already has.
What remains is the first part which depends on the timestamp (in red in the example above) which we don’t know, but we know it is a timestamp between the 2 timestamps in the links the attacker has (the 2 slices of bread). Hence, the security researcher created a script that generated a list of all timestamps between the 2 timestamps in the links in the attacker link, and used that to generate uuids and links then used these links to brute force the application until the correct id was found.
Once the correct id was found the attacker could use it to reset the password of the victim account and take over the account.

The Fix

This issue can be fixed by switching to UUID version 4 which relies on random number generation, making it impossible to guess the generated uuids.

Lessons Learned

In this case the issue is more related to the implementation than to the design of the reset password functionality, and there are multiple things we can do that could help avoid this issue and similar issues:

Rate limiting: This attack needed brute forcing to work, and this could have been prevented by rate limiting. In this case, as the route is not authenticated you can limit the number of requests per client IP per second or per minute.
Always review crypto usage with security: Poor choice of crypto algorithms could lead to different kinds of security issues, so it is always a good idea to review any usage of crypto with your security team. And yes, random number/id generation should be included in crypto operations. This is also a good topic to discuss during the threat modeling of your project.
Use SAST and Linters: This kind of implementation issue could be detected automatically using tools SAST and Linters as the vulnerable functions are known. For example, in this case I couldn’t find a SAST rule to detect the usage of UUIDv1, but I took advantage of Semgrep’s Custom rules feature to add a rule to detect the usage of UUIDv1 in Python in the Semgrep Open source Rule Registry. Here is the Pull Request I submitted to add the rule https://github.com/semgrep/semgrep-rules/pull/3517 Here’s an example of findings generated by the new rule I added:

$ semgrep -c insecure-uuid-version.yaml .

┌──── ○○○ ────┐
│ Semgrep CLI │
└─────────────┘

Scanning 180 files (only git-tracked) with 1 Code rule:

  CODE RULES
  Scanning 91 files.

  SUPPLY CHAIN RULES

  No rules to run.


  PROGRESS

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00                                                                                                                        


┌─────────────────┐
│ 3 Code Findings │
└─────────────────┘

    insecure-uuid-version.py
    ❯❱ insecure-uuid-version
          Using UUID version 1 for UUID generation can lead to predictable UUIDs based on system information
          (e.g., MAC address, timestamp). This may lead to security risks such as the sandwich attack. 
          Consider using `uuid.uuid4()` instead for better randomness and security.                                                                

           ▶▶┆ Autofix ▶ uuid.uuid4()
            4┆ uuid = uuid.uuid1()
            ⋮┆----------------------------------------
           ▶▶┆ Autofix ▶ uuid4()
            9┆ uuid = uuid1()
            ⋮┆----------------------------------------
           ▶▶┆ Autofix ▶ uuid4()
           14┆ uuid = uuid1()                

┌──────────────┐
│ Scan Summary │
└──────────────┘
Some files were skipped or only partially analyzed.
  Scan was limited to files tracked by git.

Ran 1 rule on 91 files: 3 findings.

⏫ A new version of Semgrep is available. See <https://semgrep.dev/docs/upgrading>

Side Challenges

💡 Use the above Semgrep custom rule as reference and submit a new Semgrep rule that detects the usage of UUIDv1 in another language such as Java or Javascript. Share with me the Pull Request the in comments if you do.

💡 Create a rule in any Linter you use (e.g. ESLint) that detects the usage of UUIDv1. Share the rule with me in the comments.

Conclusion

The fact that the usage of the uuid1() function instead of uuid4() could lead to an account takeover vulnerability just shows that for many security issues, the devil truly lies in the details. This is why your approach to application security should be multi-layered to cover both the design and the implementation. It is also useful to use automation to convert any lessons learned for past issues to rules and checks in your tools (like the Semgrep rule above). Hope you found this useful, have a great day ahead!

Lessons Learned #2: Your new feature could introduce a security vulnerability to your old feature (Clickhouse CVE-2024-22412)

Mohamed AboElKheir — Wed, 25 Sep 2024 17:28:54 +0000

This is the second story in the “Lessons Learned” series where we discuss real-world vulnerabilities from the eyes of an application security engineer, with a focus on the underlying root causes and the measures we can take to prevent similar issues in our applications.

In today’s story, we will discuss CVE-2024-22412 which affected ClickHouse a popular open-source column-oriented database management system typically used for online analytical processing (OLAP) in real-time. You can find the full write-up of the vulnerability here.

Impact of the vulnerability

This vulnerability could lead to authorization bypass under specific conditions, potentially leading to the exposure of sensitive data stored in the database.

What went wrong?

ClickHouse had previously introduced a feature that allowed role-based access control to any table based on the value of a column. For example you can create 2 roles, one that is only allowed to access to rows where user_id = 1 and another role is only allowed access to rows where user_id = 2 with the statements below.

CREATE ROLE user_role_1;
GRANT SELECT ON user_data TO user_role_1;
CREATE ROW POLICY user_policy_1 ON user_data
    FOR SELECT USING user_id = 1 TO user_role_1;   

CREATE ROLE user_role_2;
GRANT SELECT ON user_data TO user_role_2;
CREATE ROW POLICY user_policy_2 ON user_data
    FOR SELECT USING user_id = 2 TO user_role_2;

GRANT user_role_1, user_role_2 TO user;

INSERT INTO user_data (id, user_id) VALUES 
  (1, 1), (2, 2), (3, 1), (4, 2), (5, 1), (6, 1), (7, 1), (8, 1), (9, 2);

Now when selecting from the table using these 2 roles the results will vary based on the role as shown below.

This “Role-based access control” feature in itself was working fine until a new feature was introduced which is a “Query cache”. The goal of the new feature is to enhance performance by caching the results of queries, and returning the results from the cache if the same query is run.

Now as you may have already guessed, the issue here was related to how these 2 features played together. The “Query cache” didn’t add the user role in the identifier of the query, making the same query run by 2 different roles look the same for the query cache, and subsequently returning the results of user_role_1 if they are already cached when user_role_2 is used, allowing access to rows the role shouldn’t be authorized to access.

The Fix

ClickHouse fixed this by adding a patch incorporating current users and roles into the cache key, making the same query with 2 different roles have different cache keys.

Lessons Learned

Similar to the last story of this series, this is a business logic issue specific to ClickHouse, so no security scanning tool (SAST, DAST, IAST, .. etc) could have detected this issue. For this kind of issue, your best lines of defense are:

Threat modeling: Spending time during the design phase of any project to decide what could go wrong would be a good place to discuss business logic issues and what can be done to avoid them (the mitigations). For complex issues involving multiple features like this one, Threat modeling is the activity that is most likely to catch such issues before being pushed to production. For more about threat modeling you can check my series Threat Modeling Handbook.
Security tests: Covering the security properties of your features (e.g. the threat model mitigations) with unit or integration tests could also have helped with detecting new security issues being introduced after a feature is launched. That being said, note that issues related to caching are sometimes missed by unit and integration tests as they need a specific sequence of events to be reproducible.
Pentests and Bug Bounty: If you miss such issues in your threat model and security tests, then having regular pentests and/or a bug bounty program can act as your safety net. In this case, the issue was reported to ClickHouse’s Bug Bounty program, which put them in a much better place that if an actual attacker discovered the issue and tried to exploit it.

Conclusion

Software is complex, and while 2 features could be working well separately, they could introduce a security vulnerability when combined. Hence, it is always good to consciously consider the security implications of any new feature during the design phase (threat modeling) and to cover the security properties with tests to ensure they don’t get broken by future changes.

Lessons Learned #1: One line of code can make your application vulnerable (Pre-Auth RCE in Metabase CVE-2023–38646)

Mohamed AboElKheir — Tue, 03 Sep 2024 21:17:11 +0000

Welcome all to this new series “Lessons Learned”. In this series, I plan to share some real-world vulnerabilities from the eyes of an application security engineer. There are many resources where you can find vulnerability write-ups which mostly focus on the exploitation techniques used to discover and exploit the vulnerability. This is usually pretty cool but is more relevant to security researchers/pentesters/bug bounty hunters.

However, In this series, I will take a different angle that is more relevant to application security engineers and developers, so I will focus instead on the underlying root causes and the measures we can take to prevent similar issues in our applications.

Of course, I will leave links for the vulnerabilities write-up if you are also interested in the exploitation techniques.

So Let’s start with the first vulnerability!

Pre-Auth RCE in Metabase (CVE-2023–38646)

Metabase is a popular open-source business intelligence tool. A vulnerability was discovered around July 2023 and assigned CVE-2023–38646 (you can find the full write-up here) which had a devastating impact of pre-auth RCE (Remote code execution) which means an unauthenticated user with network access to any instance of the web application could run code on the servers the application is running on. So Let’s discover what went wrong!

Metabase

Issue #1: Authentication Bypass

Like many applications, Metabase uses a setup token during the initialization of the application. Once the application is initialized, the setup token should no longer be useable, and instead, the user should use the credentials created during the initialization process.

This was working as expected until one day a developer pushed a PR (pull request), and for some reason removed the line of code clearing the setup token after init is done, this wasn’t caught in the code review, and as a result for this version of Metabase and subsequent ones, the setup token was available after being in production for anyone that has network access.

Any attacker that can find the setup token, can use it to bypass authentication and login to the web application.

One line of code made the setup token available after the initialization

Issue #2: SQL Injection

As Metabase is a business intelligence application, it has to connect to multiple database, one of which is the H2 database. The security researchers in this case found a 0-day vulnerability (undiscovered vulnerability) affecting the H2 database driver allowing SQL injection.

You can see the SQL injection payload in the screenshot below, and you can find more details about how it works in the write-up itself. However, the thing to note here is that the reason this SQL injection was possible, is the fact that the application was taking the connection string itself as input which allowed the security researchers a lot of room to experiment and find ways to exploit.

SQL injection payload

Lessons Learned

Authentication bypass: The setup token issue is a business logic issue, very specific to Metabase. This means that no security scanning tool (SAST, DAST, IAST, .. etc) could have detected this issue. For this class of issues, your best bet is always:
- Threat modeling: Spending time during the design phase of any project to decide what could go wrong would be a good place to discuss business logic issues and what can be done to avoid them (the mitigations). For more about threat modeling you can check my series Threat Modeling Handbook.
- Security tests: It is equally important to make sure the mitigations discussed during threat modeling, and any security controls are being covered by tests. This could be a unit test, an integration test, or a security tool scan depending on the situation. For example, in this case, an integration test verifying the setup token was no longer available after initialization would have detected the issue before being pushed.
SQL injection: In this case, the vulnerability was not in the code itself, but in the library used for H2 database connection. As this was a 0-day, SCA tools (e.g. Dependabot) couldn’t have helped. However, two things could have helped here:

Not taking complex input such as the connection string: The exploit would have been much less likely to work if we only took the host, port, and other details needed for the connection and used them to build the connection string within the code, making it harder to provide the payload needed for SQL injection to work.
Input validation: besides not taking complex, input it is always important to perform input validation that doesn’t allow unneeded special characters, this could be in the form of a pattern constraint (regular expression), a list of allowed values, or a list of allowed characters. This also makes it nearly impossible to provide the payload needed for SQL injection to work. You can find more about input validation in my previous story How to make “Input validation” easy for your devs.

Conclusion

Always remember that even one line of code could make your application vulnerable, and while code reviews work, you can’t fully rely on them. You should also make sure that any security control is covered by a test that is running during build or periodically to make sure new code changes don’t break the control.

Stay tuned for the next episode of “Lessons Learned”!

How to make “Input validation” easy for your devs

Mohamed AboElKheir — Tue, 23 Jul 2024 14:34:46 +0000

What is Input validation

Input validation is one of the basic security controls that help protect against a wide range of web application attacks (e.g. SQL injection, Command injection, Directory traversal, .. etc). The goal of input validation is to discard any input that doesn’t look valid before it reaches a dangerous sink (e.g. an SQL statement).

What qualifies as input depends on the type of the application, e.g. for a typical REST API accepting HTTP requests, input parameters could come from:

Request headers
Cookies
GET parameters
Route parameters, e.g. for the route /api/resource/:resourceId the resource id is in the path/route e.g. for /api/resource/12345678 the resourceId is 12345678
Request body, e.g. fields in a JSON object

For each input parameter validation can take many forms but the most common ones are:

List of allowed values. e.g. for a type input parameter could only be one of the valid types (e.g. email, chat, or SMS).
A pattern. e.g. for a resource_id input parameter should be in the form of a UUID.
Maximum length with a list of allowed characters. e.g. for a description input parameter

Why Input validation is needed

This is effective because most attacks need some kind of weird-looking payload that includes special characters being passed as input (the source) and reaching a dangerous function (the sink). For example, an SQL injection payload may look like value'; DROP TABLE users; --. However, when input validation is applied these kinds of payloads are usually discarded before reaching the dangerous sink.

NOTE: While input validation is a useful security control, it doesn’t replace the need to replace dangerous sinks with safe ones. For e.g. for SQL injection you should always use parameterized queries to avoid SQL injection. Input validation should be used as a security-in-depth control just in case a dangerous sink is missed.

Paved road for Input validation

Despite the value and the relatively low complexity of input validation, many developers skip implementing it for their applications. This is because we usually follow the least friction path to our goal, and input validation is not generally necessary for the application to work as expected, hence developers ignore it or de-prioritize it to focus on the actual logic of the application.

This is why it is important for us application security engineers not to only focus on the importance of input validation, but also on how to make it easy for developers to implement it. Finding a way that makes it straightforward, easy to implement, and easy to audit a security control significantly reduces friction and increases adoption. In other words, we should find a “Paved road” for our developers to implement input validation.

For input validation, one way to do that is to bundle the input validation logic, and the behavior when input validation fails (e.g. return 400 status code with a custom error message) into some sort of middleware, and then use this middleware for our routes. This way developers only need to add this middleware to the routes and specify the expected parameters for each route with the corresponding type of validation.

Express as an example

As an example, let’s assume we are using Node.JS and Express framework for our backend. We can implement this middleware which uses the Joi validation library under the hood. The middleware can look something like:

const Joi = require('joi');
const validate = (schema) => {
  return (req, res, next) => {
    const sources = ['body', 'query', 'params', 'headers','cookies'];
    const validationResults = sources.map(source => {
      if (schema[source]) {
        return schema[source].validate(req[source], { abortEarly: false });
      }
      return { error: null };
    });
    const errors = validationResults.reduce((acc, result) => {
      if (result.error) {
        acc.push(...result.error.details);
      }
      return acc;
    }, []);
    if (errors.length > 0) {
      return res.status(400).json({ errors });
    }
    next();
  };
};
module.exports = validate;

Now, all what developers need to do is to specify a schema that defines the inputs coming from the request body, query parameters, path parameters, headers and cookies, e.g.

const createUserSchema = {
  body: Joi.object({
    username: Joi.string().email().required(), // only emails are accepted
    password: Joi.string().min(6).required(), // minimum length is 6 characters
  }),
  headers: Joi.object({
    'test1': Joi.string().pattern(new RegExp('^[a-zA-Z0-9]{3,30}$')).required(), // a regular expression
    'test2': Joi.string().valid('value1','value2','value3').optional(), // list of allowed values
  }).unknown(true), // Allow other headers
  cookies: Joi.object({
    'cookie1': Joi.uuid().required(), // only uuids are accepted
    'cookie2': Joi.string().valid('value1','value2','value3').optional(),// list of allowed values
  }),
  params: Joi.object(),
  query: Joi.object()
};

This scheme specifies the body parameters, headers, and cookies expected and their allowed values/patterns. Any other parameters or any invalid value will automatically return a 400 response code.

Then the developers just need to use the middleware with the schema when defining the route, e.g.:

app.post('/user', validate(createUserSchema), (req, res) => {
// Generate a unique ID for the user (for demonstration purposes)
  const userId = uuidv4();

  // Create a new user object from the request body
  const newUser = {
    userId: userId,
    username: req.body.username,
    password: req.body.password
  };
  // Add the new user to the users object using the generated ID
  users.push(newUser);
  // Respond with the newly created user object
  res.json(newUser);
});

Auditing the use of the middleware

Also, we can use a tool like Semgrep to audit the use of the middleware through all routes using a custom rule, to show a simple example we can use a rule like the below (Note this is just example which covers one way of defining routes in Express, for production the rule needs to be extended to include all other ways of defining routes).

rules:
  - id: express-route-without-validate-middleware
    patterns:
      - pattern: |
          router.route($ROUTE).$METHOD(...)
      - pattern-not-inside: |
          router.route($ROUTE).$METHOD(validate(...), $HANDLER)
    message: "Route declared without validate middleware"
    severity: WARNING
    languages: [javascript, typescript]
    metadata:
      category: security
      technology: express
    description: "Add validate middleware to this route"

When forgetting to add the middleware, this rule would generate a finding for the developers.

Conclusion

The above example is just one way to create a paved road to make input validation easier for your developers, obviously based on what languages and frameworks your developers are using you may need to adapt your approach. My recommendation is to have a discussion with your dev teams and security champions to decide on the best way to implement a solution that works for your organization, and make your goal for the agreed solution to be easy to use, and easy to audit as shown in the above example.