-
Notifications
You must be signed in to change notification settings - Fork 168
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 the ssrf #287
fix the ssrf #287
Conversation
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.
well, I think my code is ok In my ubantu. But the error in travis CI makes me confused....
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.
Can you explain to me the difference between remote file inclusion and SSRF?
def download_file(self, url): | ||
injectd_url = self.extract_url(urllib2.unquote(url)) | ||
try: | ||
r = safe_request_url(injectd_url) |
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.
please make this optional
try: | ||
r = safe_request_url(injectd_url) | ||
except Exception as e: | ||
logger.info(e) |
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.
should be error
not info
except Exception as e: | ||
logger.info(e) | ||
file_name = None | ||
return file_name |
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.
return None
@@ -85,6 +134,7 @@ def download_file(self, url): | |||
|
|||
def handle(self, attack_event): | |||
if attack_event.http_request.command == 'GET': | |||
#pass |
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.
remove
@@ -54,9 +97,15 @@ def store_file(self, injected_file): | |||
with open(os.path.join(self.files_dir, file_name), 'w+') as local_file: | |||
local_file.write(injected_file) | |||
return file_name | |||
|
|||
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.
remove
ip_address = socket.getaddrinfo(hostname, 'http')[0][4][0] | ||
if is_inner_ipaddress(ip_address): | ||
logger.info("inner ip address attack") | ||
raise BaseException("inner ip address attack") |
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.
why is this bad?
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.
Because the others may attack the inner network, we must filter the url before we use our web server to request url.
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.
I actually like this. This should only happening when testing. Maybe instead of raising an exception give it a classification?
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.
Yeah, your are right. I will change it, but nowadays I am too busy, maybe I will send a pull request slowly.
|
||
import glastopf.sandbox.sandbox as sandbox | ||
from glastopf.modules.handlers import base_emulator | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
def check_ssrf(url): |
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.
what are you actually checking here?
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.
Can you find a better name for this function?
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.
maybe you can give me a better name.
except BaseException as e: | ||
return False, str(e) | ||
except: | ||
return False, "unknow error" |
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.
Is it really unknown?
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.
please improve the exception handling here
return False, "unknow error" | ||
|
||
def safe_request_url(url, **kwargs): | ||
success, errstr = check_ssrf(url) |
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.
success?
def safe_request_url(url, **kwargs): | ||
success, errstr = check_ssrf(url) | ||
if not success: | ||
raise requests.exceptions.InvalidURL("SSRF Attack: %s" % (errstr,)) |
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.
was it actually a SSRF attack or just someone using the server from someone else to inject some code?
Closing due to inactivity |
Someone may attack the local network, I fix it. My English is not fluent, I hope you can understand my means.