Page MenuHomePhabricator

Security review of Ex:PageBanner
Closed, ResolvedPublic

Event Timeline

csteipp created this task.Jul 28 2015, 11:25 PM
csteipp claimed this task.
csteipp raised the priority of this task from to Medium.
csteipp updated the task description. (Show Details)
csteipp added subscribers: csteipp, Sumit, Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 28 2015, 11:25 PM
csteipp moved this task from Incoming to In Progress on the Security-Team board.Jul 31 2015, 5:59 PM

I don't see a way to directly exploit this, but this should be more intelligent about how it strips out these classes so it doesn't accidentally change the html context of a part of the output. What's the eta on the FIXME comment, making this less hacky?

if ( strpos( $options['toc'], 'id="toc"' ) !== false ) {
	$options['toc'] = str_replace( 'id="toc"', '', $options['toc'] );
}
if ( strpos( $options['toc'], 'class="toc"' ) !== false ) {
	$options['toc'] = str_replace( 'class="toc"', '', $options['toc'] );
}

The rest of this is safe as is, although the way the parameters are passed to the template looks like it might trip up developers later on. Can the parameters that aren't escaped (toc, icon) get documented in getBannerHtml?

Sumit added a comment.Aug 3 2015, 2:05 PM

I don't see a way to directly exploit this, but this should be more intelligent about how it strips out these classes so it doesn't accidentally change the html context of a part of the output. What's the eta on the FIXME comment, making this less hacky?

if ( strpos( $options['toc'], 'id="toc"' ) !== false ) {
	$options['toc'] = str_replace( 'id="toc"', '', $options['toc'] );
}
if ( strpos( $options['toc'], 'class="toc"' ) !== false ) {
	$options['toc'] = str_replace( 'class="toc"', '', $options['toc'] );
}

The task T105520 needs a tweak in an important part of the core and would need a lot of dicsussion before going forward. Hence, it does not have a defined eta as of now.

The rest of this is safe as is, although the way the parameters are passed to the template looks like it might trip up developers later on. Can the parameters that aren't escaped (toc, icon) get documented in getBannerHtml?

I've opened a task T107755 to explore the possibility of adding clarity to how the banner parameters are set.

Jdlrobson closed this task as Resolved.Aug 5 2015, 5:30 PM