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

refs #1503 fix reflected xss #1699

Merged
merged 3 commits into from Dec 20, 2018
Merged

refs #1503 fix reflected xss #1699

merged 3 commits into from Dec 20, 2018

Conversation

lbhsot
Copy link
Contributor

@lbhsot lbhsot commented Sep 4, 2018

refs #1503 "%20javascrip%0at:alert(/xss/)" will also cause xss attack.

thanks Omar Eissa from Deloitte Germany who found this issue

@lbhsot
Copy link
Contributor Author

lbhsot commented Sep 4, 2018

@pawl @mrjoes

@@ -132,7 +132,8 @@ def prettify_class_name(name):

def is_safe_url(target):
# prevent urls starting with "javascript:"
target = target.strip()
_substitute_whitespace = compile(r'[\s\x00-\x08\x0B\x0C\x0E-\x19]+').sub
Copy link
Contributor

Choose a reason for hiding this comment

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

_substitute_whitespace is better to be a const.

Copy link
Member

Choose a reason for hiding this comment

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

Agree here - please change it to const and I'll merge ASAP.

@fantix
Copy link
Contributor

fantix commented Sep 5, 2018

@lbhsot
Copy link
Contributor Author

lbhsot commented Sep 5, 2018

done. thanks for reviewing the pr. @fantix @mrjoes

@xqliu
Copy link
Contributor

xqliu commented Sep 17, 2018

Dear @fantix @mrjoes,

Is it possible that this could be merged and a new version released for this xss fix?

Thanks :)

# prevent urls like "\\www.google.com"
# some browser will change \\ to // (eg: Chrome)
# refs https://stackoverflow.com/questions/10438008
target = target.replace('\\', '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why you're only matching agains double slashes, and not doing target.replace('\', '/')?

Copy link

@jhatch28 jhatch28 Oct 2, 2018

Choose a reason for hiding this comment

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

This matches one slash. Your comment is a syntax error because the first slash escapes the second one. You can't put a \ before a quote at the end of a string literal in python. Try this:
print '\\'

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah, yes, of course. Thanks @jhatch28

@@ -9,6 +9,8 @@


VALID_SCHEMES = ['http', 'https']
_substitute_whitespace = compile(r'[\s\x00-\x08\x0B\x0C\x0E-\x19]+').sub
_fix_multiple_slashes = compile(r'(^([^/]+:)?//)/*').sub
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment to make it easier to follow the logic of the regex here

@@ -9,6 +9,8 @@


VALID_SCHEMES = ['http', 'https']
_substitute_whitespace = compile(r'[\s\x00-\x08\x0B\x0C\x0E-\x19]+').sub
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there tests for all these specific ASCII characters that we are matching against? The only whitespace that I see in the tests are in assert not helpers.is_safe_url(' javascript:alert(document.domain)') and for that use-case a simple replace(' ', '') would do the job.

Please either expand the tests, or simplify the logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. when checking for control characters, wouldn't it be simpler to do it in the same way as it's done in Django by checking unicodedata.category (see https://github.com/django/django/blob/550cb3a365dee4edfdd1563224d5304de2a57fda/django/utils/http.py#L364)?

@fantix
Copy link
Contributor

fantix commented Oct 23, 2018

@lbhsot please update, thanks!

diegodelemos pushed a commit to diegodelemos/cookiecutter-invenio-instance that referenced this pull request Nov 7, 2018
* Disables Flask-Admin vulnerability 36437 check, to be removed when
  flask-admin/flask-admin#1699 is merged and
  released (Flask-Admin 1.5.3).
diegodelemos pushed a commit to inveniosoftware/cookiecutter-invenio-instance that referenced this pull request Nov 7, 2018
* Disables Flask-Admin vulnerability 36437 check, to be removed when
  flask-admin/flask-admin#1699 is merged and
  released (Flask-Admin 1.5.3).
@m-butterfield
Copy link

@lbhsot Will this be updated and merged anytime soon?

@slint
Copy link

slint commented Nov 22, 2018

FYI, when having the latest version of flask-admin installed and you run pipenv check (which uses pyup), you get something like:

$ pipenv check
Checking PEP 508 requirements…
Passed!
Checking installed package safety…
36437: flask-admin <=1.5.2 resolved (1.5.2 installed)!
helpers.py in Flask-Admin 1.5.2 has Reflected XSS via a crafted URL.
see https://github.com/flask-admin/flask-admin/pull/1699

In which case if this is part of your tests/CI, you'll have to do pipenv check --ignore 36437

@ye
Copy link

ye commented Dec 3, 2018

The last commit in this PR was in September. It's now December. Any updates on this PR? If for whatever reason the original author of the PR cannot update the PR to addressnew code review comments, can someone else step in to finish the job?

FYI a day left Flask-Admin un-patched is a day hundreds of thousands of Flask-Admin sites vulnerable to this XSS attack!

@mrjoes mrjoes merged commit 8af10e0 into flask-admin:master Dec 20, 2018
@Lewiscowles1986
Copy link

Can this be tagged for release please?

@amahlaka
Copy link

Well, github is now giving a big warning banner due to this vulnerability, it would be really good to get this fix released

@olorton
Copy link

olorton commented Jan 21, 2019

@Lewiscowles1986 @amahlaka The tagged release for this is v1.5.3 - see pypi.org/project/Flask-Admin/

@mrjoes
Copy link
Member

mrjoes commented Jan 22, 2019

I'm a little lost here. The fix went live Dec 19, 2018. I don't see the GitHub banner either.

@amahlaka
Copy link

nvm, it turns out that the github banner things was panicking because of a old requirements file that was just in a deprecated folder

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.

None yet