View Issue Details

IDProjectCategoryView StatusLast Update
0021611mantisbtsecuritypublic2016-08-28 01:12
ReporterwdollmanAssigned Todregad 
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version1.2.0 
Target Version1.3.1Fixed in Version1.3.1 
Summary0021611: CVE-2016-6837: XSS vulnerability in view_all_bug_page.php
Description

The value of the view_type parameter on the view_all_bug_page.php page is not encoded before being displayed on the page.

For example, browsing to /view_all_bug_page.php?view_type=">injected-html results in the following HTML:

<input type="hidden" name="view_type" value="">injected-html" />

This allows arbitrary JavaScript to be injected into the page, and affects the latest stable release (1.3.0).

Steps To Reproduce

This installation of Mantis is vulnerable to the issue, and runs with a Content-Security-Policy that allows inline JavaScript. Visiting the following URL in a browser with no cross-site scripting filter (e.g. Mozilla Firefox), or a browser with the cross-site scripting filter disabled, will trigger a JavaScript popup:

http://mantisbt.org/bugs/view_all_bug_page.php?view_type="><script>alert(1);</script>

A general example is /view_all_bug_page.php?view_type="><script>alert(1);</script> This proof-of-concept is blocked by the default Content-Security-Policy set by Mantis 1.3.0, though as shown above, some sites may have disabled or relaxed the default CSP.

Additional Information

If you request a CVE for this issue, please credit Will Dollman of Netcraft Ltd.

TagsNo tags attached.

Relationships

related to 0021263 closedvboctor CVE-2016-7111: Content Security Policy is weakened by Gravatar plugin 

Activities

atrol

atrol

2016-08-15 06:03

developer   ~0053808

Changing line 2258 in file core/filter_api.php should fix the issue

  • <input type="hidden" name="view_type" value="<?php echo $t_view_type?>" />
  • <input type="hidden" name="view_type" value="<?php echo 'advanced' == $t_view_type ? 'advanced' : 'simple'?>" />
wdollman

wdollman

2016-08-15 06:40

reporter   ~0053809

I've included our patch for the issue below. It sanitizes the variable in filter_ensure_valid_filter(), so it's safe for display even if it's echoed elsewhere in a future change:

core/filter_api.php | 5 +++++

@@ -492,6 +492,11 @@ function filter_ensure_valid_filter( array $p_filter_arr ) {
    if( !isset( $p_filter_arr['_view_type'] ) ) {
        $p_filter_arr['_view_type'] = gpc_get_string( 'view_type', 'simple' );
    }
+
+   if (!in_array($p_filter_arr['_view_type'], array("simple", "advanced")) ) {
+       $p_filter_arr['_view_type'] = "simple";
+   }
+
    if( !isset( $p_filter_arr[FILTER_PROPERTY_ISSUES_PER_PAGE] ) ) {
        $p_filter_arr[FILTER_PROPERTY_ISSUES_PER_PAGE] = gpc_get_int( FILTER_PROPERTY_ISSUES_PER_PAGE, config_get( 'default_limit_view' ) );
    }
dregad

dregad

2016-08-15 09:22

developer   ~0053810

The issue actually exists (at least) since 1.2.0.

dregad

dregad

2016-08-16 17:53

developer  

0001-Fix-XSS-in-view_all_bug_page.php.patch (1,293 bytes)
From 421af1ed47a2b9a72c999876e6e17c7f621acb5f Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Tue, 16 Aug 2016 23:22:40 +0200
Subject: [PATCH] Fix XSS in view_all_bug_page.php

The value of the view_type parameter on the view_all_bug_page.php page
was not encoded before being displayed.

This vulnerability was discovered by Will Dollman of Netcraft Ltd.

Fixes #21611
---
 core/filter_api.php | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/core/filter_api.php b/core/filter_api.php
index c8e8eae..9294f64 100644
--- a/core/filter_api.php
+++ b/core/filter_api.php
@@ -489,9 +489,15 @@ function filter_ensure_valid_filter( array $p_filter_arr ) {
 		# if the version is old, update it
 		$p_filter_arr['_version'] = FILTER_VERSION;
 	}
+
+	# Filter type - ensure it's either 'simple' or 'advanced' (prevent XSS)
 	if( !isset( $p_filter_arr['_view_type'] ) ) {
 		$p_filter_arr['_view_type'] = gpc_get_string( 'view_type', 'simple' );
 	}
+	if( $p_filter_arr['_view_type'] != 'advanced' ) {
+		$p_filter_arr['_view_type'] = 'simple';
+	}
+
 	if( !isset( $p_filter_arr[FILTER_PROPERTY_ISSUES_PER_PAGE] ) ) {
 		$p_filter_arr[FILTER_PROPERTY_ISSUES_PER_PAGE] = gpc_get_int( FILTER_PROPERTY_ISSUES_PER_PAGE, config_get( 'default_limit_view' ) );
 	}
-- 
2.7.4

dregad

dregad

2016-08-16 17:54

developer   ~0053821

I believe the better solution is indeed to ensure that whatever value we store in the filter array is properly sanitized, to guarantee that any future use is safe.

Please see proposed patch attached. @wdollman kindly confirm that this indeed fixes the issue; upon receiving your confirmation, I'll request a CVE.

Note: my fix improves the performance of the code proposed in 0021611:0053809, as it does not rely on calling the in_array() function.

wdollman

wdollman

2016-08-17 05:03

reporter   ~0053832

I've attached an updated patch with a minor change which swaps the != comparison for a !==

If $p_filter_arr['_view_type'] were set to 0 (integer), PHP will cast "advanced" to an integer, bypassing the != check and we'd end up with a sanitized $p_filter_arr['_view_type'] value of 0. I don't see any way of triggering this - gpc_get_string() returns a string rather than an integer, and also don't see any way it could be exploited - but I don't like the check returning an unexpected value.

If you're ok with this updated patch, I'm happy that it resolves the issue and for you to request a CVE.

wdollman

wdollman

2016-08-17 05:03

reporter  

0001-Fix-XSS-in-view_all_bug_page.php-safecmp.patch (1,294 bytes)
From 421af1ed47a2b9a72c999876e6e17c7f621acb5f Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Tue, 16 Aug 2016 23:22:40 +0200
Subject: [PATCH] Fix XSS in view_all_bug_page.php

The value of the view_type parameter on the view_all_bug_page.php page
was not encoded before being displayed.

This vulnerability was discovered by Will Dollman of Netcraft Ltd.

Fixes #21611
---
 core/filter_api.php | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/core/filter_api.php b/core/filter_api.php
index c8e8eae..9294f64 100644
--- a/core/filter_api.php
+++ b/core/filter_api.php
@@ -489,9 +489,15 @@ function filter_ensure_valid_filter( array $p_filter_arr ) {
 		# if the version is old, update it
 		$p_filter_arr['_version'] = FILTER_VERSION;
 	}
+
+	# Filter type - ensure it's either 'simple' or 'advanced' (prevent XSS)
 	if( !isset( $p_filter_arr['_view_type'] ) ) {
 		$p_filter_arr['_view_type'] = gpc_get_string( 'view_type', 'simple' );
 	}
+	if( $p_filter_arr['_view_type'] !== 'advanced' ) {
+		$p_filter_arr['_view_type'] = 'simple';
+	}
+
 	if( !isset( $p_filter_arr[FILTER_PROPERTY_ISSUES_PER_PAGE] ) ) {
 		$p_filter_arr[FILTER_PROPERTY_ISSUES_PER_PAGE] = gpc_get_int( FILTER_PROPERTY_ISSUES_PER_PAGE, config_get( 'default_limit_view' ) );
 	}
-- 
2.7.4

dregad

dregad

2016-08-17 09:13

developer   ~0053833

Last edited: 2016-08-17 09:16

View 2 revisions

Thanks for the feedback Will.

I have no objections with your proposed use of the strict comparison operator, even though as you pointed out I don't think it's possible for gpc_string() to return anything other than a string in this context, but it doesn't hurt. Belt and braces ;-)

I'll push the modified patch to Github now, and request the CVE later; I will update this issue when I receive the ID.

Note: the issue is now public

cproensa

cproensa

2016-08-17 17:33

developer   ~0053835

I believe the better solution is indeed to ensure that whatever value we store in the filter array is properly sanitized, to guarantee that any future use is safe.

That is fine, but note that a filter array may not come only from previously stored filter.
IIRC, a filter array may be provided as url parameters in some places, so validating again in "filter_ensure_valid_filter" is still needed.

dregad

dregad

2016-08-17 18:50

developer   ~0053836

validating again in "filter_ensure_valid_filter" is still needed

You're confusing me. This is actually exactly where I've applied the patch... Did I miss something ?

cproensa

cproensa

2016-08-17 19:01

developer   ~0053837

Did I miss something ?

Maybe i didn't explain right. As you said, the validation must to be done in "filter_ensure_valid_filter".

As i read "I believe the better solution is indeed to ensure that whatever value we store in the filter array is properly sanitized":
this is good, but not a replacement for said "filter_ensure..." check

dregad

dregad

2016-08-18 02:44

developer   ~0053844

CVE-2016-6837 assigned.
http://www.openwall.com/lists/oss-security/2016/08/18/8

Related Changesets

MantisBT: master-1.3.x 7086c2d8

2016-08-16 17:22:40

dregad

Details Diff
Fix XSS in view_all_bug_page.php

The value of the view_type parameter on the view_all_bug_page.php page
was not encoded before being displayed.

This vulnerability was discovered by Will Dollman of Netcraft Ltd.

Initial patch modified to use strict comparison per Will's suggestion.

Fixes 0021611
mod - core/filter_api.php Diff File

Issue History

Date Modified Username Field Change
2016-08-15 04:57 wdollman New Issue
2016-08-15 05:13 atrol Status new => confirmed
2016-08-15 05:50 atrol Relationship added related to 0021263
2016-08-15 06:03 atrol Note Added: 0053808
2016-08-15 06:40 wdollman Note Added: 0053809
2016-08-15 09:17 dregad Product Version 1.3.0 => 1.2.0
2016-08-15 09:17 dregad Target Version => 1.3.1
2016-08-15 09:22 dregad Note Added: 0053810
2016-08-16 02:36 dregad Assigned To => dregad
2016-08-16 02:36 dregad Status confirmed => assigned
2016-08-16 17:53 dregad File Added: 0001-Fix-XSS-in-view_all_bug_page.php.patch
2016-08-16 17:54 dregad Note Added: 0053821
2016-08-17 05:03 wdollman Note Added: 0053832
2016-08-17 05:03 wdollman File Added: 0001-Fix-XSS-in-view_all_bug_page.php-safecmp.patch
2016-08-17 09:13 dregad Note Added: 0053833
2016-08-17 09:13 dregad Changeset attached => MantisBT 13x 7086c2d8
2016-08-17 09:13 dregad Status assigned => resolved
2016-08-17 09:13 dregad Resolution open => fixed
2016-08-17 09:16 dregad Note Edited: 0053833 View Revisions
2016-08-17 09:16 dregad View Status private => public
2016-08-17 17:33 cproensa Note Added: 0053835
2016-08-17 18:50 dregad Note Added: 0053836
2016-08-17 19:01 cproensa Note Added: 0053837
2016-08-18 02:44 dregad Severity minor => major
2016-08-18 02:44 dregad Summary Cross-site scripting vulnerability in view_all_bug_page.php => CVE-2016-6837: XSS vulnerability in view_all_bug_page.php
2016-08-18 02:44 dregad Note Added: 0053844
2016-08-18 06:01 atrol Fixed in Version => 1.3.1
2016-08-28 01:12 vboctor Status resolved => closed