# This fixes OSA-2017-04, also known as CVE-2017-14635: An attacker who is # logged into OTRS as an agent with write permissions for statistics can # inject arbitrary code into the system. This can lead to serious problems # like privilege escalation, data loss, and denial of service. # Closes: #876462 # URL: https://www.otrs.com/security-advisory-2017-04-security-update-otrs-versions/ diff -Naur otrs2-5.0.16.orig/Kernel/Modules/AgentStatistics.pm otrs2-5.0.16/Kernel/Modules/AgentStatistics.pm --- otrs2-5.0.16.orig/Kernel/Modules/AgentStatistics.pm 2017-01-17 03:39:35.000000000 +0100 +++ otrs2-5.0.16/Kernel/Modules/AgentStatistics.pm 2017-09-29 09:14:16.013352183 +0200 @@ -247,15 +247,17 @@ UserID => $Self->{UserID}, ); - if ($StatID) { + if ( !$StatID ) { $Errors{FileServerError} = 'ServerError'; $Errors{FileServerErrorMessage} = Translatable("Statistic could not be imported."); } + else { - # redirect to configure - return $LayoutObject->Redirect( - OP => "Action=AgentStatistics;Subaction=Edit;StatID=$StatID" - ); + # Redirect to statistic edit page. + return $LayoutObject->Redirect( + OP => "Action=AgentStatistics;Subaction=Edit;StatID=$StatID" + ); + } } else { $Errors{FileServerError} = 'ServerError'; @@ -853,6 +855,17 @@ $Data{Object} = $Object; } + my $StatsObject = $Kernel::OM->Get('Kernel::System::Stats'); + + my $ObjectModuleCheck = $StatsObject->ObjectModuleCheck( + StatType => $Data{StatType}, + ObjectModule => $Data{ObjectModule}, + ); + + if (!$ObjectModuleCheck) { + $Errors{ObjectModuleServerError} = 'ServerError'; + } + for my $Key (qw(SumRow SumCol Cache ShowAsDashboardWidget)) { $Data{$Key} = $ParamObject->GetParam( Param => $Key ) // ''; } @@ -878,7 +891,7 @@ ); } - $Param{StatID} = $Kernel::OM->Get('Kernel::System::Stats')->StatsAdd( + $Param{StatID} = $StatsObject->StatsAdd( UserID => $Self->{UserID}, ); if ( !$Param{StatID} ) { @@ -886,7 +899,7 @@ Message => Translatable('Could not create statistic.'), ); } - $Kernel::OM->Get('Kernel::System::Stats')->StatsUpdate( + $StatsObject->StatsUpdate( StatID => $Param{StatID}, Hash => \%Data, UserID => $Self->{UserID}, diff -Naur otrs2-5.0.16.orig/Kernel/Output/HTML/Statistics/View.pm otrs2-5.0.16/Kernel/Output/HTML/Statistics/View.pm --- otrs2-5.0.16.orig/Kernel/Output/HTML/Statistics/View.pm 2017-01-17 03:39:35.000000000 +0100 +++ otrs2-5.0.16/Kernel/Output/HTML/Statistics/View.pm 2017-09-29 09:14:16.013352183 +0200 @@ -227,9 +227,13 @@ # load static module my $Params = $Kernel::OM->Get('Kernel::System::Stats')->GetParams( StatID => $StatID ); + + return if !$Params; + $LayoutObject->Block( Name => 'Static', ); + PARAMITEM: for my $ParamItem ( @{$Params} ) { $LayoutObject->Block( diff -Naur otrs2-5.0.16.orig/Kernel/System/Stats.pm otrs2-5.0.16/Kernel/System/Stats.pm --- otrs2-5.0.16.orig/Kernel/System/Stats.pm 2017-01-17 03:39:35.000000000 +0100 +++ otrs2-5.0.16/Kernel/System/Stats.pm 2017-09-29 09:14:16.013352183 +0200 @@ -940,6 +940,7 @@ return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); my $StatObject = $ObjectModule->new( %{$Self} ); return if !$StatObject; + return if !$StatObject->can('GetObjectAttributes'); # load attributes my @ObjectAttributesRaw = $StatObject->GetObjectAttributes(); @@ -1061,7 +1062,7 @@ next OBJECT; } $Filelist{$Object} = $Self->GetObjectName( - ObjectModule => $Filelist{$Object}{Module}, + ObjectModule => $Filelist{$Object}->{Module}, ); } return if !%Filelist; @@ -1093,6 +1094,8 @@ # get name my $StatObject = $Module->new( %{$Self} ); return if !$StatObject; + return if !$StatObject->can('GetObjectName'); + my $Name = $StatObject->GetObjectName(); # cache the result @@ -1133,8 +1136,8 @@ my $StatObject = $Module->new( %{$Self} ); return if !$StatObject; - return if !$StatObject->can('GetObjectBehaviours'); + my %ObjectBehaviours = $StatObject->GetObjectBehaviours(); # cache the result @@ -1145,6 +1148,8 @@ =item ObjectFileCheck() +AT THE MOMENT NOT USED + check readable object file my $ObjectFileCheck = $StatsObject->ObjectFileCheck( @@ -1172,6 +1177,94 @@ return; } +=item ObjectModuleCheck() + +Check the object module. + + my $ObjectModuleCheck = $StatsObject->ObjectModuleCheck( + StatType => 'static', + ObjectModule => 'Kernel::System::Stats::Static::StateAction', + ); + +Returns true on success and false on error. + +=cut + +sub ObjectModuleCheck { + my ( $Self, %Param ) = @_; + + return if !$Param{StatType} || !$Param{ObjectModule}; + return if $Param{StatType} ne 'static' && $Param{StatType} ne 'dynamic'; + + my $CheckFileLocation = 'Kernel::System::Stats::' . ucfirst $Param{StatType}; + return if $Param{ObjectModule} !~ m{ \A $CheckFileLocation }xms; + + my $ObjectName = [ split( m{::}, $Param{ObjectModule} ) ]->[-1]; + return if !$ObjectName; + return if $ObjectName !~ m{[A-Z_a-z][0-9A-Z_a-z]*}xms; + + my @RequiredObjectFunctions; + + if ( $Param{StatType} eq 'static' ) { + + @RequiredObjectFunctions = ( + 'GetObjectBehaviours', + 'Param', + 'Run', + ); + + my $StaticFiles = $Self->GetStaticFiles( + OnlyUnusedFiles => 1, + UserID => 1, + ); + + if ( $ObjectName && !$StaticFiles->{$ObjectName} ) { + $Kernel::OM->Get('Kernel::System::Log')->Log( + Priority => 'error', + Message => "Static object $ObjectName doesn't exist or static object already in use!", + ); + return; + } + } + else { + + @RequiredObjectFunctions = ( + 'GetObjectName', + 'GetObjectBehaviours', + 'GetObjectAttributes', + ); + + # Check if the given Object exists in the statistic object registartion. + my $DynamicFiles = $Self->GetDynamicFiles(); + + if ( !$DynamicFiles->{$ObjectName} ) { + $Kernel::OM->Get('Kernel::System::Log')->Log( + Priority => 'error', + Message => "Object $ObjectName doesn't exist!" + ); + return; + } + } + + my $ObjectModule = $Param{ObjectModule}; + return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); + + my $StatObject = $ObjectModule->new( %{$Self} ); + return if !$StatObject; + + # Check for the required object functions. + for my $RequiredObjectFunction (@RequiredObjectFunctions) { + return if !$StatObject->can($RequiredObjectFunction); + } + + # Special check for some fucntions in the dynamic statistic object. + if ( $Param{StatType} eq 'dynamic' ) { + return if !$StatObject->can('GetStatTable') && !$StatObject->can('GetStatElement'); + } + + return 1; +} + =item Export() get content from stats for export @@ -1199,9 +1292,7 @@ my @XMLHash = $XMLObject->XMLHashGet( Type => 'Stats', - - #Cache => 0, - Key => $Param{StatID} + Key => $Param{StatID}, ); my $StatsXML = $XMLHash[0]->{otrs_stats}->[1]; @@ -1211,36 +1302,7 @@ ); $File{Filename} .= '.xml'; - # settings for static files - if ( - $StatsXML->{StatType}->[1]->{Content} - && $StatsXML->{StatType}->[1]->{Content} eq 'static' - ) - { - my $FileLocation = $StatsXML->{ObjectModule}->[1]->{Content}; - $FileLocation =~ s{::}{\/}xg; - $FileLocation .= '.pm'; - my $File = $Kernel::OM->Get('Kernel::Config')->Get('Home') . "/$FileLocation"; - my $FileContent = ''; - - open my $Filehandle, '<', $File || die "Can't open: $File: $!"; ## no critic - - # set bin mode - binmode $Filehandle; - while (<$Filehandle>) { - $FileContent .= $_; - } - close $Filehandle; - - $Kernel::OM->Get('Kernel::System::Encode')->EncodeInput( \$FileContent ); - $StatsXML->{File}->[1]->{File} = $StatsXML->{File}->[1]->{Content}; - $StatsXML->{File}->[1]->{Content} = encode_base64( $FileContent, '' ); - $StatsXML->{File}->[1]->{Location} = $FileLocation; - $StatsXML->{File}->[1]->{Permission} = '644'; - $StatsXML->{File}->[1]->{Encode} = 'Base64'; - } - - # delete create and change data + # Delete not needed and useful keys from the stats xml. for my $Key (qw(Changed ChangedBy Created CreatedBy StatID)) { delete $StatsXML->{$Key}; } @@ -1267,10 +1329,11 @@ my $StatObject = $ObjectModule->new( %{$Self} ); return if !$StatObject; - # load attributes - $StatsXML = $StatObject->ExportWrapper( - %{$StatsXML}, - ); + if ( $StatObject->can('ExportWrapper') ) { + $StatsXML = $StatObject->ExportWrapper( + %{$StatsXML}, + ); + } } # convert hash to string @@ -1326,34 +1389,48 @@ ); # check if the required elements are available - for my $Element ( - qw( Description Format Object ObjectModule Permission StatType SumCol SumRow Title Valid) - ) - { + for my $Element (qw( Description Format Object ObjectModule Permission StatType SumCol SumRow Title Valid)) { if ( !defined $StatsXML->{$Element}->[1]->{Content} ) { $Kernel::OM->Get('Kernel::System::Log')->Log( Priority => 'error', - Message => - "Can't import Stat, because the required element $Element is not available!" + Message => "Can't import Stat, because the required element $Element is not available!" ); return; } } - # get config object + my $ObjectModuleCheck = $Self->ObjectModuleCheck( + StatType => $StatsXML->{StatType}->[1]->{Content}, + ObjectModule => $StatsXML->{ObjectModule}->[1]->{Content} + ); + + return if !$ObjectModuleCheck; + + my $ObjectModule = $StatsXML->{ObjectModule}->[1]->{Content}; + return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); + + my $StatObject = $ObjectModule->new( %{$Self} ); + return if !$StatObject; + + my $ObjectName = [ split( m{::}, $StatsXML->{ObjectModule}->[1]->{Content} ) ]->[-1]; + if ( $StatsXML->{StatType}->[1]->{Content} eq 'static' ) { + $StatsXML->{File}->[1]->{Content} = $ObjectName; + } + else { + $StatsXML->{Object}->[1]->{Content} = $ObjectName; + } + my $ConfigObject = $Kernel::OM->Get('Kernel::Config'); # if-clause if a stat-xml includes a StatNumber my $StatID = 1; if ( $StatsXML->{StatNumber} ) { - my $XMLStatsID = $StatsXML->{StatNumber}->[1]->{Content} - - $ConfigObject->Get('Stats::StatsStartNumber'); + my $XMLStatsID = $StatsXML->{StatNumber}->[1]->{Content} - $ConfigObject->Get('Stats::StatsStartNumber'); for my $Key (@Keys) { if ( $Key eq $XMLStatsID ) { $Kernel::OM->Get('Kernel::System::Log')->Log( Priority => 'error', - Message => - "Can't import StatNumber $Key, because this StatNumber is already used!" + Message => "Can't import StatNumber $Key, because this StatNumber is already used!" ); return; } @@ -1384,83 +1461,6 @@ $StatsXML->{ChangedBy}->[1]->{Content} = $Param{UserID}; $StatsXML->{StatNumber}->[1]->{Content} = $StatID + $ConfigObject->Get('Stats::StatsStartNumber'); - my $DynamicFiles = $Self->GetDynamicFiles(); - - # Because some xml-parser insert \n instead of - if ( $StatsXML->{Object}->[1]->{Content} ) { - $StatsXML->{Object}->[1]->{Content} =~ s{\n}{}x; - } - - if ( - $StatsXML->{Object}->[1]->{Content} - && !$DynamicFiles->{ $StatsXML->{Object}->[1]->{Content} } - ) - { - $Kernel::OM->Get('Kernel::System::Log')->Log( - Priority => 'error', - Message => "Object $StatsXML->{Object}->[1]->{Content} doesn't exist!" - ); - return; - } - - # static statistic - if ( - $StatsXML->{StatType}->[1]->{Content} - && $StatsXML->{StatType}->[1]->{Content} eq 'static' - ) - { - my $FileLocation = $StatsXML->{ObjectModule}[1]{Content}; - $FileLocation =~ s{::}{\/}gx; - $FileLocation = $ConfigObject->Get('Home') . '/' . $FileLocation . '.pm'; - - # if no inline file is given in the stats definition - if ( !$StatsXML->{File}->[1]->{Content} ) { - - # get the file name - $FileLocation =~ s{ \A .*? ( [^/]+ ) \. pm \z }{$1}xms; - - # set the file name - $StatsXML->{File}->[1]->{Content} = $FileLocation; - } - - # write file if it is included in the stats definition - ## no critic - elsif ( open my $Filehandle, '>', $FileLocation ) { - ## use critic - - print STDERR "Notice: Install $FileLocation ($StatsXML->{File}[1]{Permission})!\n"; - if ( $StatsXML->{File}->[1]->{Encode} && $StatsXML->{File}->[1]->{Encode} eq 'Base64' ) - { - $StatsXML->{File}->[1]->{Content} = decode_base64( $StatsXML->{File}->[1]->{Content} ); - $Kernel::OM->Get('Kernel::System::Encode')->EncodeOutput( - \$StatsXML->{File}->[1]->{Content} - ); - } - - # set utf8 or bin mode - if ( $StatsXML->{File}->[1]->{Content} =~ /use\sutf8;/ ) { - open $Filehandle, '>:utf8', $FileLocation; ## no critic - } - else { - binmode $Filehandle; - } - print $Filehandle $StatsXML->{File}->[1]->{Content}; - close $Filehandle; - - # set permission - if ( length( $StatsXML->{File}->[1]->{Permission} ) == 3 ) { - $StatsXML->{File}->[1]->{Permission} = "0$StatsXML->{File}->[1]->{Permission}"; - } - chmod( oct( $StatsXML->{File}->[1]->{Permission} ), $FileLocation ); - $StatsXML->{File}->[1]->{Content} = $StatsXML->{File}->[1]->{File}; - - delete $StatsXML->{File}->[1]->{File}; - delete $StatsXML->{File}->[1]->{Location}; - delete $StatsXML->{File}->[1]->{Permission}; - delete $StatsXML->{File}->[1]->{Encode}; - } - } - # wrapper to change used spelling in ids # wrap permissions my %Groups = $Kernel::OM->Get('Kernel::System::Group')->GroupList( Valid => 1 ); @@ -1488,19 +1488,10 @@ } # wrap object dependend ids - if ( $StatsXML->{Object}->[1]->{Content} ) { - - # load module - my $ObjectModule = $StatsXML->{ObjectModule}->[1]->{Content}; - return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); - my $StatObject = $ObjectModule->new( %{$Self} ); - return if !$StatObject; - - # load attributes + if ( $StatObject->can('ImportWrapper') ) { $StatsXML = $StatObject->ImportWrapper( %{$StatsXML} ); } - # new return if !$XMLObject->XMLHashAdd( Type => 'Stats', Key => $StatID, @@ -1552,6 +1543,7 @@ return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); my $StatObject = $ObjectModule->new( %{$Self} ); return if !$StatObject; + return if !$StatObject->can('Param'); # get params @Params = $StatObject->Param(); @@ -2123,6 +2115,7 @@ return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); my $StatObject = $ObjectModule->new( %{$Self} ); return if !$StatObject; + return if !$StatObject->can('Run'); my @Result; my %GetParam = %{ $Param{GetParam} }; @@ -2218,6 +2211,7 @@ return if !$Kernel::OM->Get('Kernel::System::Main')->Require($ObjectModule); my $StatObject = $ObjectModule->new( %{$Self} ); return if !$StatObject; + return if !$StatObject->can('GetStatTable') && !$StatObject->can('GetStatElement'); # get time object my $TimeObject = $Kernel::OM->Get('Kernel::System::Time'); diff -Naur otrs2-5.0.16.orig/scripts/test/sample/Stats/Stats.Static.NotExisting.xml otrs2-5.0.16/scripts/test/sample/Stats/Stats.Static.NotExisting.xml --- otrs2-5.0.16.orig/scripts/test/sample/Stats/Stats.Static.NotExisting.xml 1970-01-01 01:00:00.000000000 +0100 +++ otrs2-5.0.16/scripts/test/sample/Stats/Stats.Static.NotExisting.xml 2017-09-29 09:14:16.013352183 +0200 @@ -0,0 +1,16 @@ + + +0 +Not existing static object module. +CSV +Excel +Print + +Kernel::System::Stats::Static::NotExisting +stats +static +1 +1 +Not existing static object module +1 + diff -Naur otrs2-5.0.16.orig/scripts/test/Selenium/Agent/AgentStatistics/Import.t otrs2-5.0.16/scripts/test/Selenium/Agent/AgentStatistics/Import.t --- otrs2-5.0.16.orig/scripts/test/Selenium/Agent/AgentStatistics/Import.t 2017-01-17 03:39:35.000000000 +0100 +++ otrs2-5.0.16/scripts/test/Selenium/Agent/AgentStatistics/Import.t 2017-09-29 09:14:16.013352183 +0200 @@ -120,6 +120,26 @@ $Selenium->VerifiedGet("${ScriptAlias}index.pl?Action=AgentStatistics;Subaction=Import"); # import test selenium statistic + my $LocationNotExistingObject = $Kernel::OM->Get('Kernel::Config')->Get('Home') . "/scripts/test/sample/Stats/Stats.Static.NotExisting.xml"; + $Selenium->find_element( "#File", 'css' )->send_keys($LocationNotExistingObject); + + $Selenium->find_element("//button[\@value='Import'][\@type='submit']")->VerifiedClick(); + + # Confirm JS error. + $Selenium->find_element( "#DialogButton1", 'css' )->click(); + + # Verify error class. + $Self->Is( + $Selenium->execute_script( + "return \$('#File').hasClass('Error')" + ), + '1', + 'Import file field has class error', + ); + + $Selenium->VerifiedGet("${ScriptAlias}index.pl?Action=AgentStatistics;Subaction=Import"); + + # import test selenium statistic my $Location = $Kernel::OM->Get('Kernel::Config')->Get('Home') . "/scripts/test/sample/Stats/Stats.TicketOverview.de.xml"; $Selenium->find_element( "#File", 'css' )->send_keys($Location); diff -Naur otrs2-5.0.16.orig/scripts/test/Stats.t otrs2-5.0.16/scripts/test/Stats.t --- otrs2-5.0.16.orig/scripts/test/Stats.t 2017-01-17 03:39:35.000000000 +0100 +++ otrs2-5.0.16/scripts/test/Stats.t 2017-09-29 09:14:16.013352183 +0200 @@ -445,6 +445,32 @@ "Export-Importcheck - check if import file content equal export file content.\n Be careful, if it gives errors if you run OTRS with default charset utf-8,\n because the examplefile is iso-8859-1, but at my test there a no problems to compare a utf-8 string with an iso string?!\n", ); +# Import a static statistic with not exsting object module + +# load example file +my $PathNotExistingStatistic = $ConfigObject->Get('Home') . '/scripts/test/sample/Stats/Stats.Static.NotExisting.xml'; +my $FilehandleNotExistingStatistic; +if ( !open $FilehandleNotExistingStatistic, '<', $PathNotExistingStatistic ) { ## no critic + $Self->True( + 0, + 'Get the file which should be imported', + ); +} + +@Lines = <$FilehandleNotExistingStatistic>; +my $ImportContentNotExistingStatistic = join '', @Lines; + +close $Filehandle; + +my $NotExistingStatID = $StatsObject->Import( + Content => $ImportContentNotExistingStatistic, + UserID => 1, +); +$Self->False( + $NotExistingStatID, + 'Import() statistic with not existing object module must fail', +); + # try to use otrs.Console.pl Maint::Stats::Generate # check the imported stat