search

restic / rest-server

Rest Server is a high performance HTTP server that implements restic's REST backend API.
BSD 2-Clause "Simplified" License
944 stars 140 forks source link

Security: Prevent loading of usernames containing a slash #132

Closed juergenhoetzel closed 3 years ago

juergenhoetzel commented 3 years ago

"/" is valid char in HTTP authorization headers, but are also used in rest-server to map usernames to private repos.

This commit prevents loading maliciously composed usernames like "/foo/config".

Issue: #131

Checklist

gurshafriri commented 3 years ago

👋 @juergenhoetzel can you elaborate what is the impact of this issue? if it's a valid security issue we (at snyk) would like to add it to our vulnerability database.

juergenhoetzel commented 3 years ago

@juergenhoetzel can you elaborate what is the impact of this issue?

This possible exploit comes to my mind:

In a shared rest-server setup where users can sign up for backup space (web registration form) and choose a malicious "username" they can overwrite other users config files as shown in the issue #131 .

rawtaz commented 3 years ago

I think it's rather inaccurate to consider this a security vulnerability in rest-server.

As an analogy, consider PHP's include() directive. Imagine a user of this language, the developer in this case, writing include('subpages/' . $_GET['page']) for the purpose of dynamically including some subpage on the website. Since they don't sanitize the $_GET['page'] input, an attacker can set a GET parameter like page=../../config.ini or similar to escape the public folder of the website and access the config file having sensitive information in it. Now, is this a security vulnerability in PHP? Of course not, it's a developer error and security issue introduced by the developer, in their use of the software.

Same here - if you e.g. build the web form @juergenhoetzel suggested, and actually regardless of what you build, you are expected to sanitize user input - otherwise you don't control what goes into the password command or password file in the first place. The rest-server can't take into account all the wrongdoings that can be done.

In particular, regarding the example in #131, the documentation states "you may use the --private-repos flag which grants access only when a subdirectory with the same name as the user is specified in the repository URL". In other words, if you specify, in the URL, a subdirectory with the same name as the user, access is granted. This menas that if you specify, in the URL, the path foo/config, this matches the user foo/config, and as the documentation states, when these two match, access is granted.

Hence, this works exactly as specified in the documentation. If you then develop e.g. a web form letting users input a username like "foo" and "foo/config", and they run the curl command in the issue, then you will have opened up for the result mentioned in the issue. But that's not a security vulnerability in the software, it's a security issue introduced by how you use of the software in a larger context.

To be clear; the problem in this (#131) example case is that you are actively letting the second user access a part of the first user's repository. There's nothing rest-server can do to prevent you from that (it would be the same in any other software or filesystem), besides perhaps warn you about it or actively prevent the use of usernames that have the same prefix. It would have to be done in the loading of the password file, as rest-server doesn't control the password file, obviously. Regardless that's a separate thing than a security vulnerability in this case - it's more of a matter of adding more checks to prevent mistakes :)

All that said, I think the PR itself isn't a bad idea. I don't see anyone intentionally wanting to specify usernames that (with the --private-repos option in use) gives one user access to another user's repository, but there's indeed a chance that someone e.g. builds a web form that doesn't sanitize input properly, and then (just like with a regular filesystem) an adversary user may be unintentionally given access to parts of the first user's repository. Since there's hardly a use case for the "foo" and "foo/config" usernames, I'm all for implementing this PR :)

Thanks!

rawtaz commented 3 years ago

Second thought: Should we perhaps instead of disallowing / go even further and only accept [a-zA-Z0-9@.-] in usernames? Not sure what other characters people would actually need in their usernames.

gurshafriri commented 3 years ago

@rawtaz thanks for your elaborated explanation!

Nevertheless, I beg to defer with you on the classification of this as not a security issue :)

In your PHP analogy, the attacker uses the attribute as intended by PHP. Here, the attacker can use an attribute defined as a username (auth) to access files. I would even argue that the isUserPath function: https://github.com/restic/rest-server/blob/9313f194415a90f06c122053c35252ae23112686/handlers.go#L151-L159

is exactly intended to check that the username in the beginning of the path is the same as the one requesting it - but it missed the case of "/foo/config" to differ between users, giving 2 different usernames - foo and foo/config the access to this private repo. If flagging a repo as a "private-repo" doesn't block access for other users due to unwanted characters in a username - it seems something I would like to alert users to update when fixed. Don't you agree?

I will wait until you (or any other maintainer) will response before making an advisory out of it so I can get your opinion before - and also happy to wait for a fix if it will be pushed in the near future.

rawtaz commented 3 years ago

@gurshafriri It seems to me that you don't fully understand how this feature in rest-server is designed. I am however on the road right now so I'll reply more in a couple of days. But for now, please actually read the rest of what I wrote as well, it seems like you only read the PHP part I wrote.

juergenhoetzel commented 3 years ago

Quoting/highlighting the documentation:

To prevent your users from accessing each others' repositories, you may use the --private-repos flag which grants access only when a subdirectory with the same name as the user is specified in the repository URL.

I interpret a subdirectory as a single subdirectory and not a path which has the same prefix as the username. Otherwise rest-server can not prevent your users from accessing each others' repositories in the case shown in issue #131.

fd0 commented 3 years ago

Thanks for the report! In my opinion, it's not a vulnerability in the rest-server, but it becomes one when the rest-server is embedded in a larger system in some way. We should IMHO keep the Security: prefix in the changelog file so people using the rest-server recognize this issue as potentially security-relevant.

I agreed this is issue is undesirable and the code does not cover usernames containing slashes. Let's not pick on the (not so great) documentation, but rather move forward with correcting the issue.

Likely backslashes at least on Windows will need to be handled the same way, so I'd opt for a rather strict list of allowed characters (like @rawtaz's proposal [a-zA-Z0-9@.-]). @juergenhoetzel do you need help implementing this?

juergenhoetzel commented 3 years ago

characters (like @rawtaz's proposal [a-zA-Z0-9@.-]). @juergenhoetzel do you need help implementing this?

Hi @fd0: I amended to commit to restict the valid characters using the regexp.

But I choose to use the unicode character class to do not discriminate non-ASCII people ;-)

gurshafriri commented 3 years ago

@gurshafriri It seems to me that you don't fully understand how this feature in rest-server is designed. I am however on the road right now so I'll reply more in a couple of days. But for now, please actually read the rest of what I wrote as well, it seems like you only read the PHP part I wrote.

@rawtaz I swear that I did - could be that I got it wrong.

Please remember that discussions around security issues are important to the open-source community at large. You cannot know how projects that use (or will use) this project implements it, let alone projects that use projects (that use projects...) that use this one. The way to secure open-source would be to pin-point the core issue, fix it and prompt developers to update their packages accordingly.

Discussions on wether a certain root cause lies in one package or another are welcome though. In this case I tend to feel that this is the issue's core. I also ran it through another one of our security personas to get a 2nd opinion. If it was a wanted behaviour, I would argue that it is common to add a warning to it in some way - but since it is not I don't think it is relevant in this case.

@fd0 there is still the open question of wether you would want to prompt users of this library to update it due to this fix. We respect your context and views on wether this kind of behaviour is under the responsibility of this package, but feel free to get our help with assigning a CVE and creating an advisory out of it if you change your mind.

fd0 commented 3 years ago

@gurshafriri Maybe we have a misunderstanding here: Although it can be imported as a library, the rest-server is a standalone program.

From that viewpoint, the htpasswd file is considered a configuration file, and only administrative users should be able to change it. If you assume that attackers have that kind of access (in a usual deployment), then they can just give themselves all access privileges they need, even without this issue.

Hypothetically, people using this project as a library would need to make sure that they validate the where the user information comes from. As far as I can see nobody besides us (well, and a fork of rest-server) imports it as a library, which matches my expectation based on the exchanges I had with users.

I agree that the described behaviour was unwanted, we corrected it (thanks again @juergenhoetzel) and the next release will have a changelog entry about it labelled Security, here. We've done this with other (more severe vulnerabilities) before, see the CHANGELOG file. New releases are announced in the blog and on Twitter.

We (as a project) take great care to write the CHANGELOG file for our users so that it contains all the information they need and it's not just a dump of git log or so. In my experience, this works great to make users read the CHANGELOG.

From our side I think that's enough. Thanks for your offer for help though!