Potential security issue

oomb commented 3 years ago

Hello,

A potential security issue was found in this repository and was reported via huntr.

Kindly look into the issue here and validate it.

This link is private and will be only accessible to the project maintainers.

Thanks!

ciur commented 3 years ago

@oomb, thank you for opening this issue. I will have a look.

ciur commented 3 years ago

Here is how to reproduce the vulnerability issue:

Login as as Papermerge administrative user (may work for other papermerge users as well)
The naive Papermerge admin user receives a phishing email (see below for email example) from another source which asks to download THEIR document from THEIR webpage and upload to THEIR webpage as if that is the way of confirming THEIR service
The naive Papermerge administrative user, clicks the link, but instead it download HIS (or one of HIS user's document, because of predictable URL and lack of CSRF token) and uploads HIS document to THEIR webpage.

Example of phishing email:

<html>
<body>
To verify that you are a human, upload the file that has been downloaded from our website now.
<a href="https://demo.papermerge.com/node/11/download/">Download Test File</a>
</body>
</html>

To mitigate the problem either use less predictable document ID's (e.g. UUID) or use csrf tokens.

l4rm4nd commented 3 years ago

@ciur: All requests within the Papermerge web application that trigger a sensitive action use a csrf token (either within POST data or as X-CSRFToken HTTP header), so I assume the CSRF middleware is correctly enabled and the application not susceptible to CSRF.

E.g. creating a new application user:

An attacker without knowledge about the csrfmiddlewaretoken is not able to conduct a CSRF attack.

So the outlined problem is not really Cross-Site Request Forgery (CSRF) nor Insecure Direct Object Reference (IDOR).

Downloading attachements is only possible as authenticated user. You have to be logged into the web application.
Downloading attachements is only possible as authorized user. You must have permissions to access the file.
Downloading files do not trigger a state changing action on the application and is therefore done by the HTTP method GET. GET requests are usually not worth being protected by a csrf token. See OWASP
Guessing the ID of an attachement does not really have an impact, since authorization and access controls are in place, which only allow the owner of a document to download it. So even if you have another valid user account within your papermerge application, this user won't be able to download the attachements without proper permissions, even if he can guess the correct ID of another user's document.

So what is the impact of this:

An attacker is able to trick you into downloading your own attachement from your own papermerge web application. Further, you will have to be already logged into the application during the attack. Otherwise, the download won't work due to missing authentication and you will see your papermerge's login panel. Moreover, browsers usually display a popup before saving files. This popup displays the filename, not under control of the attacker as well as the domain/server of which the file will be downloaded. Many hints to let you know what is really happening, reducing the likelihood of an attack.
After a successfull "attack", an attacker would further need to trick you into uploading your downloaded attachement to another application under control of the attacker. Otherwise, the attacker won't get any information by tricking you into downloading the file. The Same-Origin-Policy (SOP) hereby protects against unauthorized data access (read) during cross-site requests from another domain. So an attacker never sees any server response with file data by tricking you into downloading files.

So imho this is not really an issue at all.

Recommendation

You might introduce UUIDs instead of iterable IDs, but due to authorization in place, this is optional.
You might change the HTTP method for downloading files and implement a CSRF token if you really want to protect users against the outlined phishing attack, where you are tricked into downloading your own files from your own papermerge server.

ciur commented 3 years ago

@l4rm4nd, thank you a lot for your detailed explanation. I will take care of the issue.

ciur / papermerge

Potential security issue #390