Fix regression that discloses file path in some errors #1280

vboctor · 2018-02-03T07:59:40Z

This was introduced as part of refactoring error handler and it happens with some errors even when show_detailed_errors is set to OFF.

Fixes #23925

This was introduced as part of refactoring error handler and it happens with some errors even when show_detailed_errors is set to OFF. Fixes #23925

atrol · 2018-02-03T17:17:26Z

core/error_api.php

@@ -247,11 +247,16 @@ function error_handler( $p_type, $p_error, $p_file, $p_line, array $p_context )

 	$t_error_description = nl2br( $t_error_description );

+	# Make sure the file path is not disclosed via exception details
+	$t_error_description = str_replace( config_get_global( 'absolute_path' ), '.../', $t_error_description );


we have

define ('DIRECTORY_SEPARATOR', "/"); $g_absolute_path = dirname( __FILE__ ) . DIRECTORY_SEPARATOR;

Can't check myself at the moment, did you try if it works on Windows?

Should the path be removed if ON == config_get_global( 'show_detailed_errors' )?

I initially had the code to only show the path when show_detailed_errors is ON. But I then removed this code since I think it is not worth having the extra checks. The real value is the relative path.

I don't have a Windows environment setup at the moment.

I can test on Windows when I'm in the office on Monday.

dregad · 2018-02-03T23:23:46Z

I'm also concerned about the robustness of the proposed str_replace()-based solution. It works for sure on unix-like environments with a standard MantisBT installation, but what about the case where the admin customized the folder locations in config_inc.php ? Regardless of the directory separator, there are bound to be cases where a simple replacement won't do the trick.

I thought about using the file item in the exception's first stack trace entry instead of $g_absolute_path as string to replace, but it this still does not sound very solid.

IMO, PHP errors are not meant to be displayed to end users; I believe it would be preferable and safer to display a generic error, while writing the actual message and stack trace in the system log - at least when $g_show_detailed_errors = OFF.

vboctor · 2018-02-03T23:43:57Z

Will look into having our exception displayed, but PHP exceptions or exceptions that didn't originate from Mantis show a generic message (e.g. internal error, while logging the proper error). If show detailed errors is ON, then we will always show exception. Will refine the approach as I look further on the code.

Will remove path replacement.

This reverts commit d5d85f1.

dregad · 2018-02-03T23:55:20Z

Cheers. Please note that I will probably not have the opportunity to review/test your revised fix until tomorrow evening (~22:00 CET) as I won't be at home the whole day. I'll do my best to do it as soon as possible, so you can release 2.11 as planned, but if I can't manage I'd rather postpone the release by a few days.

vboctor · 2018-02-04T01:26:07Z

I have implemented an alternative fix which:

Logs php errors.
Don't display php exception messages in UI or API.

dregad

Note: I only tested from the GUI, not from SOAP or REST API.

Sorry to insist, but I really think that when $g_show_detailed_errors is ON, we should provide the error message in the GUI as a convenience to administrators and developers, as per patch below.

Other review comments follow.

diff --git a/core/error_api.php b/core/error_api.php
index b0c1517..265fd5a 100644
--- a/core/error_api.php
+++ b/core/error_api.php
@@ -175,6 +175,8 @@ function error_handler( $p_type, $p_error, $p_file, $p_line, array $p_context )
        }
    }
 
+   $t_show_detailed_errors = config_get_global( 'show_detailed_errors' ) == ON;
+
    # Force errors to use HALT method.
    if( $p_type == E_USER_ERROR || $p_type == E_ERROR || $p_type == E_RECOVERABLE_ERROR ) {
        $t_method = DISPLAY_ERROR_HALT;
@@ -203,12 +205,15 @@ function error_handler( $p_type, $p_error, $p_file, $p_line, array $p_context )
        case E_USER_ERROR:
            if( $p_error == ERROR_PHP ) {
                $t_error_type = 'INTERNAL APPLICATION ERROR';
-               $t_error_description = '';
 
                global $g_exception;
-               $t_stack_as_string = error_stack_track_as_string();
-               $t_error_to_log =  $g_exception->getMessage() . "\n" . $t_stack_as_string;
+               $t_error_description = $g_exception->getMessage();
+               $t_error_to_log =  $t_error_description . "\n" . error_stack_track_as_string();
                error_log( $t_error_to_log );
+
+               if( !$t_show_detailed_errors ) {
+                   $t_error_description = '';
+               }
            } else {
                $t_error_type = 'APPLICATION ERROR #' . $p_error;
                $t_error_description = error_string( $p_error );
@@ -259,7 +264,7 @@ function error_handler( $p_type, $p_error, $p_file, $p_line, array $p_context )
        if( DISPLAY_ERROR_NONE != $t_method ) {
            echo $t_error_type . ': ' . $t_error_description . "\n";
 
-           if( ON == config_get_global( 'show_detailed_errors' ) ) {
+           if( $t_show_detailed_errors  ) {
                echo "\n";
                error_print_stack_trace();
            }
@@ -342,7 +347,7 @@ function error_handler( $p_type, $p_error, $p_file, $p_line, array $p_context )
                }
                echo '</div>';
 
-               if( ON == config_get_global( 'show_detailed_errors' ) ) {
+               if( $t_show_detailed_errors ) {
                    echo '<p>';
                    error_print_details( $p_file, $p_line, $p_context );
                    echo '</p>';

dregad · 2018-02-04T23:14:25Z

api/rest/index.php

+		$t_error_to_log =  $p_exception->getMessage() . "\n" . $t_stack_as_string;
+		error_log( $t_error_to_log );
+
+		return $p_response->withStatus( HTTP_STATUS_INTERNAL_SERVER_ERROR )->withJson( $t_data );


Is it normal to return ->withJson( $t_data ) which still contains the exception's message ?

Changed code to include it only if show detailed errors is ON.

dregad · 2018-02-04T23:19:45Z

core/error_api.php

 */
-function error_print_stack_trace() {
-	$t_stack = error_stack_trace();
+function error_stack_track_as_string( $p_exception = null ) {


Should be error_stack_trace_as_string, no ?

Fixed typo.

dregad · 2018-02-04T23:20:40Z

core/error_api.php

+	$t_stack = error_stack_trace( $p_exception );
+
+	if( php_sapi_name() == 'cli' ) {
+		echo error_stack_track_as_string( $p_exception );


error_stack_trace_as_string

Fixed typo.

dregad · 2018-02-04T23:23:39Z

core/error_api.php

+ * @param Exception|null $p_exception The exception to print stack trace for.  Null will check last seen exception.
+ */
+function error_print_stack_trace( $p_exception = null ) {
+	$t_stack = error_stack_trace( $p_exception );


Move that assignment after the cli test, just before the foreach loop

dregad · 2018-02-04T23:24:55Z

core/error_api.php

+	$t_stack = error_stack_trace( $p_exception );
+
+	if( php_sapi_name() == 'cli' ) {
+		echo error_stack_track_as_string( $p_exception );
 		return;
 	}



The comment # remove the call to the error handler from the stack trace (a few lines down from here) looks like a leftover from older code which should probably be removed.

vboctor · 2018-02-06T06:27:19Z

@dregad thanks for the review. I applied all your review comments. Let me know if all looks good.

dregad · 2018-02-06T12:22:06Z

Looks good, but I didn't test in depth the behavior from the API.

vboctor self-assigned this Feb 3, 2018

vboctor force-pushed the error_handler_path_disclosure branch from 2e3701c to 67e28b5 Compare February 3, 2018 08:08

Fix regression that discloses file path in some errors

d5d85f1

This was introduced as part of refactoring error handler and it happens with some errors even when show_detailed_errors is set to OFF. Fixes #23925

vboctor force-pushed the error_handler_path_disclosure branch from 67e28b5 to d5d85f1 Compare February 3, 2018 08:17

atrol reviewed Feb 3, 2018

View reviewed changes

Revert "Fix regression that discloses file path in some errors"

9635f90

This reverts commit d5d85f1.

Don’t show php exceptions but log them

08c862a

dregad requested changes Feb 4, 2018

View reviewed changes

vboctor added 4 commits February 5, 2018 22:14

Fix func name typo for getting stack trace as string

eccc32d

Show exceptions in UI when show detailed errors is ON

f22922a

Show PHP exception in REST only if detailed errors is ON

2aabdaf

Re-order code in error_print_stack_trace()

af4519d

dregad approved these changes Feb 6, 2018

View reviewed changes

vboctor merged commit bff178e into mantisbt:master Feb 6, 2018

vboctor mentioned this pull request Feb 6, 2018

Remove utf8 library, enforce mbstring extension #1283

Merged

dregad mentioned this pull request Feb 1, 2019

Fix 'Undefined variable' PHP notice in API REST #1455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression that discloses file path in some errors #1280

Fix regression that discloses file path in some errors #1280

vboctor commented Feb 3, 2018

atrol Feb 3, 2018

vboctor Feb 3, 2018

dregad Feb 3, 2018

dregad commented Feb 3, 2018

vboctor commented Feb 3, 2018

dregad commented Feb 3, 2018

vboctor commented Feb 4, 2018

dregad left a comment •

edited

dregad Feb 4, 2018

vboctor Feb 6, 2018

dregad Feb 4, 2018

vboctor Feb 6, 2018

dregad Feb 4, 2018

vboctor Feb 6, 2018

dregad Feb 4, 2018 •

edited

vboctor Feb 6, 2018

dregad Feb 4, 2018 •

edited

vboctor Feb 6, 2018

vboctor commented Feb 6, 2018

dregad commented Feb 6, 2018

Fix regression that discloses file path in some errors #1280

Fix regression that discloses file path in some errors #1280

Conversation

vboctor commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dregad commented Feb 3, 2018

vboctor commented Feb 3, 2018

dregad commented Feb 3, 2018

vboctor commented Feb 4, 2018

dregad left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dregad Feb 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dregad Feb 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vboctor commented Feb 6, 2018

dregad commented Feb 6, 2018

dregad left a comment •

edited

dregad Feb 4, 2018 •

edited

dregad Feb 4, 2018 •

edited