Security Vulnerability Found

[porcupineyhairs]

Absolute Path Traversal due to incorrect use of send_file call

A path traversal attack (also known as directory traversal) aims to access files and directories that are stored outside the web root folder. By manipulating variables that reference files with “dot-dot-slash (../)” sequences and its variations or by using absolute file paths, it may be possible to access arbitrary files and directories stored on file system including application source code or configuration and critical system files. This attack is also known as “dot-dot-slash”, “directory traversal”, “directory climbing” and “backtracking”.

Common Weakness Enumeration category

CWE - 36

Root Cause Analysis

The os.path.join call is unsafe for use with untrusted input. When the os.path.join call encounters an absolute path, it ignores all the parameters it has encountered till that point and starts working with the new absolute path. Please see the example below.

>>> import os.path
>>> static = "path/to/mySafeStaticDir"
>>> malicious = "/../../../../../etc/passwd"
>>> os.path.join(t,malicious)
'/../../../../../etc/passwd'

Since the "malicious" parameter represents an absolute path, the result of os.path.join ignores the static directory completely. Hence, untrusted input is passed via the os.path.join call to flask.send_file can lead to path traversal attacks.

In this case, the problems occurs due to the following code :

opendiamond/opendiamond/dataretriever/augment_store.py

Line 164 in 7ded6b5

response = send_file(path,

Here, the obj_path parameter is attacker controlled. This parameter passes through the unsafe os.path.join call making the effective directory and filename passed to the send_file call attacker controlled. This leads to a path traversal attack.

Proof of Concept

The bug can be verified using a proof of concept similar to the one shown below.

curl --path-as-is 'http://<domain>/obj//../../../../etc/passwd"'

Remediation

This can be fixed by preventing flow of untrusted data to the vulnerable send_file function. In case the application logic necessiates this behaviour, one can either use the werkzeug.utils.safe_join to join untrusted paths or replace flask.send_file calls with flask.send_from_directory calls.

Common Vulnerability Scoring System Vector

The attack can be carried over the network. A complex non-standard configuration or a specialized condition is not required for the attack to be successfully conducted. There is no user interaction required for successful execution. The attack can affect components outside the scope of the target module. The attack can be used to gain access to confidential files like passwords, login credentials and other secrets. It cannot be directly used to affect a change on a system resource. Hence has limited to no impact on integrity. Using this attack vector a attacker may make multiple requests for accessing huge files such as a database. This can lead to a partial system denial service. However, the impact on availability is quite low in this case. Taking this account an appropriate CVSS v3.1 vector would be

(AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:N/A:L)[https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:N/A:L&version=3.1]

This gives it a base score of 9.3/10 and a severity rating of critical.

References

• OWASP Path Traversal
• Python : Flask Path Traversal Vulnerability github/securitylab#669

This bug was found using CodeQL by Github

[porcupineyhairs]

@jaharkes Can you please create a GHSA and request a CVE for this too?

[jaharkes]

As I commented on the pull request, I don't see an actual exploitable vulnerability here because of the way the Diamond system is architected and deployed. The dataretriever process exists only to hide the differences between local and remotely stored files for the Diamond search process.

The Diamond search scope and access validation happens in the diamondd process and as such the dataretriever is already treated as a non-trustworthy component in the chain. Because of this it only listens on the loopback interface, it runs as a non-privileged user, and the source of the object path that we normally see is obtained from the object index generated by the same dataretriever process and cannot be influenced by the (Diamond) application user.

So in the normal setup this would only provide a possible local attack vector for people who have already compromised the system on which it is deployed. The whole point of the dataretriever is to give access to data. So once the system is locally compromised, we have loss of confidentiality and whether we use os.path.join or safe_join doesn't change that.

[porcupineyhairs]

@jaharkes Did you see my comment #53 (comment) on the PR?

So in the normal setup this would only provide a possible local attack vector for people who have already compromised the system on which it is deployed. The whole point of the dataretriever is to give access to data. So once the system is locally compromised, we have loss of confidentiality and whether we use os.path.join or safe_join doesn't change that.

This is not necessarily true. Let's say you have some other service running on some other port which has an open redirect/host-only SSRF an attacker can club them both and still exploit this vulnerability. Even if the service is only exposed on local, there is still a security vulnerability albeit a bit less severe. CVSS v3.1 considers local as a attack vector too.

[jaharkes]

Look, we can argue back and forth, but ultimately there are quite a few broken assumptions and bad code. You don't have to look far to find much easier to exploit vulnerabilities in this code.

For one, we execute untrusted binaries sent by a client as "search filters", there is #21/#19 to remind us of that. In fact, there is your SSRF right there on the 'reexecute_filters' call.
But even if we fix 'safe_join' in the dataretriever here that doesn't really matter because the object loader accepts "file://" urls as a shortcut without knowing what DATAROOT was supposed to be because it doesn't check if the requested object is in the search scope to begin with (#20).

So what we're looking at here is a research project which tries to prove a concept, but wasn't really engineered to be a secure system to begin with.

[porcupineyhairs]

@jaharkes Sounds reasonable.
PS: You may want to add a note on the project's README stating that this is a research project.