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

Security Audit #458

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sayamkanwar
Copy link

@sayamkanwar sayamkanwar commented Nov 17, 2018

Hi,
I have done several tests on Fabrik and I have compiled the results.
The tests are located in 'security_audit/TESTS.md', please have a look at it I have found several vulnerabilities in Fabrik.
Thanks

@Ram81 @PalashTanejaPro @utsavgarg

@PalashTanejaPro
Copy link
Contributor

Impressive work on the report, it's quite extensive and makes it possible for developers to fix the highlighted problems.

Just a few things

  1. Does the long string in buffer overflow cause the server to crash and provide an empty response? If so then this might need immediate fixing as fabrik.cloudcv.org will be vulnerable to server crashes.

  2. I don't understand what the CSRF vulnerability is doing? Are you able to guess the tokens without using inspect element? If so then that might also be a high priority fix.

I'll give you a 1-day extension to clarify this but otherwise, this looks good to me :)

@Ram81 @utsavgarg Should the student create this .md file with the vulns or should we take the creation of issues related to them as the submission? I think issues would be more suitable for this purpose.

@sayamkanwar
Copy link
Author

sayamkanwar commented Nov 17, 2018

Thanks @PalashTanejaPro,

  1. Yes, I tried to play around with the API with different inputs and in other cases it was giving a valid JSON response whereas when I passed an arbitrarily large string, it resulted in buffer overflow. I also tried this in the browser. It returned the error 'The page isn't working' whereas it was working fine for smaller length inputs.

screenshot 2018-11-17 at 8 59 58 pm

  1. i) So basically I inspected the source code of 0.0.0.0:8000/accounts/login/. I found a hidden input value with the name 'csrfmiddlewaretoken' which is being generated by the server and is dynamic and it's obvious that it's there to prevent login requests from other origins but I believe the fault here is that it's being rendered in the HTML which makes it easy for hackers to exploit the login. In my opinion, values like these should be maintained at the backend only and should be passed to the server from backend itself instead of rendering into the web allowing login request from other origins.
    ii) Another fault: this login page contains a cookie named 'csrftoken', so I opened the console and typed console.log(document.cookie) and it returned _ga=GA1.1.860592203.1541516212; _gid=GA1.1.1409113918.1542232998; csrftoken=u9BRRc8f2Uyg9EfLpO0nLaSgR113w7Pbg63GZ9RKy0wxgRZ4TXfvfdmyaF1ZdqSC. So the problem here is the csrftoken cookie is not set to HTTPOnly which is why it is accessible through Javascript and hence it can be easily fetched with one line of code. If it is set to HTTPOnly like the session cookie then this problem will be resolved.

@Ram81
Copy link
Member

Ram81 commented Nov 17, 2018

@PalashTanejaPro I agree with you, we have to create issues and get the fixes done asap.

@Ram81
Copy link
Member

Ram81 commented Nov 17, 2018

@sayamkanwar nice work pointing this out, we are still working on login feature and it's not complete yet though. We have to get the fixes for these issues done as fast as possible.

@sayamkanwar
Copy link
Author

Thank you @Ram81! :)
Let me know how do I have to submit this task then. If I have to create the issues, I'll do it.

@PalashTanejaPro
Copy link
Contributor

@sayamkanwar great! You can mention this stuff in the issues you create then. Just create the issues and we'll approve the task for you :)

@sayamkanwar
Copy link
Author

So I have to create the issues for each vulnerability or just 1 issue for all?

@Ram81
Copy link
Member

Ram81 commented Nov 17, 2018

@sayamkanwar You can mention all of them in a single issue.

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

@sayamkanwar
Copy link
Author

Okay thank you, I'll create one!

@sayamkanwar
Copy link
Author

Created the issue: #459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants