Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vulnerabilities discovered by Scrutinizer-ci #41

Closed
rugk opened this issue Jul 18, 2016 · 6 comments
Closed

Fix vulnerabilities discovered by Scrutinizer-ci #41

rugk opened this issue Jul 18, 2016 · 6 comments

Comments

@rugk
Copy link
Member

rugk commented Jul 18, 2016

Scrutinizer-ci detected 4 potential vulnerabilities:
vulnerabilities

There is one XSS marked as "critical", one file exposure and two variable injections.

The "Variable Injection" vulnerabilities show detailed paths, so at the first glance they look serious. It is about the language selection. I don't see any escaping done. As Scrutinizer-ci recommend whitelisting would be good.

BTW thanks to @scrutinizer-ci. They offer this security analysis for free for non-commercial open-source projects.

@rugk rugk added this to the Next release milestone Jul 18, 2016
@rugk
Copy link
Member Author

rugk commented Jul 18, 2016

So I think the file exposure is intentional, but whitelisting should also be used here. Maybe one can set a language of ../../[...]/etc/passwd...

@elrido
Copy link
Contributor

elrido commented Jul 18, 2016

The language given in the cookie is compared against the whitelist of the available languages:
https://github.com/PrivateBin/PrivateBin/blob/master/lib/i18n.php#L142

This is IMHO a false positive and not a real security issue. The automatic test is interpreting the code wrong. If it bugs you, just rewrite the code so that the $match is not filled from the cookie, but from the $availableLanguages array instead.

@rugk rugk changed the title Fix vulnerabilities discovered by Fix vulnerabilities discovered by Scrutinizer-ci Jul 18, 2016
rugk added a commit that referenced this issue Jul 18, 2016
#41

Not tested locally, let's say what Travis says... 😄
@rugk
Copy link
Member Author

rugk commented Jul 18, 2016

Yes, all right. So what about the other issues?

@elrido
Copy link
Contributor

elrido commented Jul 18, 2016

RainTPL will soon be replaced and the others are warning because we suppress warnings / errors when we chmod/rmdir/unlink files. There are checks where it matters, but I suppress log to webserver if it doesn't.

Regarding the jsonld: I see no danger in replacing the URLs in the returned jsonld paths with a user defined REQUEST_URI. It would simply break the API for that user if it would point to a wrong URL. The jsonld files would be used by the paste manager described in #2 to validate that a given privatebin is compatible to the paste manager in question and supports the properties needed. We could run a htmlspecialchars() on it for safe measure.

@elrido
Copy link
Contributor

elrido commented Jul 18, 2016

Just commited e7dde4d which should address the jsonld one.

@rugk
Copy link
Member Author

rugk commented Jul 18, 2016

Yeah, I just wanted to explain that the content submitted by the user does not have to be a correct URL or so... (It might be difficult as this is a server-generated URL, but well...)

So as all security issues are addressed, we can close this issue.

@rugk rugk closed this as completed Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants