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

Multiple XSS Fixes #1094

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@quantumpacket
Copy link

commented Apr 17, 2017

$_SERVER['PHP_SELF'] is not sanitized before being used to generate URLs.

Yes, we have a CSP policy in place, but it can be disabled optionally per application config, and does not include prefixed headers so IE 10/11 would be susceptible as they use X-Content-Security-Policy according to CanIUse.

PoC: /view_user_page.php/"><script>alert(1)</script><x

http://www.mantisbt.org/bugs/view.php?id=22742

Multiple XSS Fixes
$_SERVER['PHP_SELF'] is not sanitized before being used to generate URLs.
@cproensa

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

@vboctor
what about extending function helper_url_combine()
so that it combines the current functionality, the features of http_build_query, and XSS sanitization on the final url?

@cproensa

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

even if we may discuss to do any code clean up, it shouldn't set back integration of this fix

@dregad

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@quantumpacket thanks for reporting this. In the future, kindly follow our guidelines for security issues reporting to avoid early public disclosure, see http://www.mantisbt.org/wiki/doku.php/mantisbt:handling_security_problems

IE 10/11 would be susceptible as they use X-Content-Security-Policy according to CanIUse

For the record, we're not using X-Content-Security-Policy anymore, because it is deprecated and having it in addition to the new Content-Security-Policy header could cause issues, as per https://content-security-policy.com. But maybe a differentiated test detecting browser type could be implemented, I'll see what can be done.

@@ -79,14 +79,14 @@
echo '<div class="btn-group">';
$t_url_params['days'] = $f_days + 7;
$t_href = $t_url_page . '?' . http_build_query( $t_url_params );
$t_href = htmlspecialchars( $t_url_page, ENT_QUOTES, 'UTF-8' ) . '?' . http_build_query( $t_url_params );

This comment has been minimized.

Copy link
@dregad

dregad Apr 18, 2017

Member

Escaping should be done via string_attribute() API function.

Can you explain the rationale for using ENT_QUOTES vs the default of ENT_COMPAT ?

Also, is there a reason to force UTF-8 charset ? (for the record, this is the default since PHP 5.4)

This comment has been minimized.

Copy link
@quantumpacket

quantumpacket Apr 18, 2017

Author

I personally always use ENT_QUOTES when it's not directly being output in the HTML string, but being set as a variable somewhere else, because I don't want to run into a situation where the HTML format is changed and quoting could accidentally end up no longer being escaped. As for the UTF-8, I should have checked the packages minimum supported versions. I don't use MantisBT, so not really familiar with it at all. I'm just reporting this, how you guys go about fixing it is up to you. :)

This comment has been minimized.

Copy link
@dregad

dregad Apr 18, 2017

Member

Thanks for the clarification.

@atrol

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Isn't it enough to change just at this single place?

$t_url_page = $_SERVER["PHP_SELF"];
@quantumpacket

This comment has been minimized.

Copy link
Author

commented Apr 18, 2017

Isn't it enough to change just at this single place?

It would be easier to do that, but you really should be doing the sanitizing as close to the output line as possible, so that you can rest assure that the variable being used is safe. $t_url_page says nothing about if the value is safe to print or not. Should a developer have to traverse up a file to determine if a variable is safe, or would it not be easier to assume it is not and just escape at the point of output?

@quantumpacket thanks for reporting this. In the future, kindly follow our guidelines for security issues reporting to avoid early public disclosure, see http://www.mantisbt.org/wiki/doku.php/mantisbt:handling_security_problems

I'd recommend that you link to that in the README. Having it buried in a Wiki on the website is not very likely to be stumbled upon in this repository. I'd have gone that route if it was more obvious that there was a security procedure in place. I've never used MantisBT and this was my first time even visiting this repository. Please accept my apology. :)

@dregad

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

I'd recommend that you link to that in the README. Having it buried in a Wiki on the website is not very likely to be stumbled upon in this repository.

Good suggestion, I'll take care of it.

Please accept my apology. :)

No worries 😄

@dregad

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@quantumpacket

  • did you request a CVE for this ? If so, let us know the ID when you get it; if not, I'll take care of it.
  • How would you like to be credited for the finding ?

@dregad dregad closed this in a1c7193 Apr 18, 2017

@dregad dregad added the merged label Apr 18, 2017

@quantumpacket

This comment has been minimized.

Copy link
Author

commented Apr 18, 2017

I did not request a CVE, so feel free to get one. For credits, I guess just use my Github account? Thanks, and great work on getting this fixed quickly. 😃

@quantumpacket quantumpacket deleted the quantumpacket:patch-2 branch Apr 18, 2017

@smoorez

This comment has been minimized.

Copy link

commented Apr 18, 2017

CVE-2017-7897 has been assigned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.