-
Notifications
You must be signed in to change notification settings - Fork 165
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
Patches: vulnerable code that can lead to RCE #357
Conversation
Hello @Aju100! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-01-11 04:37:49 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Aju100,
Thank you for trying to improve the security.
Your change is breaking the whole codebase and the tests are not passing anymore.
Can you look deeper into the problem? I recommend you to run the tests locally. You can check the FURY documentation to see how to run them. it will help you to debug.
Thank you
@@ -40,6 +40,7 @@ | |||
# Functions | |||
# ---------------------------------------------------------------------------- | |||
|
|||
req = urllib.request.Request('http://www.url.com') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'http://www.url.com'
: Can you explain what is this url?- Could you explain why
req
should be a global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it has broken out the whole codebase. First of all, req must be a global variable to look at our URL through which we can fetch or use it. It will be better to know about the URL link which we are using it. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see here, Request
does not to be global for doing this fix.
I still do not understand what is http://www.url.com
. Why this URL? why pointing to this place? when going to this url there is nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Url should be this one https://api.github.com/repos/{0}/stats/contributors. I think we can disclose or reject this PR for this issue. But in order to know the issue behind it, you can still use the Bandit tool to find out the vulnerabilities on the fury repository. Here is the link.
@@ -19,7 +19,7 @@ | |||
from datetime import datetime, timedelta | |||
from subprocess import check_output | |||
|
|||
from urllib.request import urlopen, Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why you remove urlopen
? The link that you provide does not do it and I do not understand your change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mention it's a vulnerable calling function that can lead to open up the file too instead of URL only.
Hi, there in this new PR, I have removed the vulnerable functions call used in the code which can allow leading Remote Code execution. Instead of using urrlib, we can use Requests for it which can solve the issue for it. For more info, you can look through bandit official docs and StackOverflow.
But one thing is we have to harder URL for there in order to request it. Thank you