From 789263864a53aabd5c6c0b0c61764892f82fd455 Mon Sep 17 00:00:00 2001 From: Len Kranendonk Date: Thu, 25 Aug 2005 15:18:40 +0000 Subject: [PATCH] Bugfix [ 1243559 ] profile field security vulnerability --- docs/changelog/6.x.x.txt | 1 + lib/WebGUI/Operation/Profile.pm | 22 ++++++++--------- lib/WebGUI/Operation/ProfileSettings.pm | 13 ++++------ lib/WebGUI/Operation/Shared.pm | 33 +++++++++++++++++++++++++ lib/WebGUI/Operation/User.pm | 10 ++++---- 5 files changed, 55 insertions(+), 24 deletions(-) diff --git a/docs/changelog/6.x.x.txt b/docs/changelog/6.x.x.txt index 9d5ae3995..c7024ded3 100644 --- a/docs/changelog/6.x.x.txt +++ b/docs/changelog/6.x.x.txt @@ -1,4 +1,5 @@ 6.7.2 + - fix [ 1243559 ] profile field security vulnerability (Len Kranendonk) - fix [ 1271490 ] uncommitted version appears in Navigation (midellaq) - fix [ 1268605 ] field type selection broken (midellaq) - fix [ 1267548 ] can't add metadata field (midellaq) diff --git a/lib/WebGUI/Operation/Profile.pm b/lib/WebGUI/Operation/Profile.pm index eeabb44c2..750765be1 100644 --- a/lib/WebGUI/Operation/Profile.pm +++ b/lib/WebGUI/Operation/Profile.pm @@ -44,10 +44,10 @@ sub getRequiredProfileFields { while($data = $a->hashRef) { my %hash = (); $method = $data->{dataType}; - $label = eval $data->{fieldLabel}; - $default = eval $data->{dataDefault}; + $label = WebGUI::Operation::Shared::secureEval($data->{fieldLabel}); + $default = WebGUI::Operation::Shared::secureEval($data->{dataDefault}); if ($method eq "selectList") { - $values = eval $data->{dataValues}; + $values = WebGUI::Operation::Shared::secureEval($data->{dataValues}); # note: this big if statement doesn't look elegant, but doing regular ORs caused problems with the array reference. if ($session{form}{$data->{fieldName}}) { $default = [$session{form}{$data->{fieldName}}]; @@ -116,7 +116,7 @@ sub validateProfileData { while (%field = $a->hash) { $data{$field{fieldName}} = WebGUI::Macro::negate(WebGUI::FormProcessor::process($field{fieldName},$field{dataType}, $field{dataDefault})); if ($field{required} && $data{$field{fieldName}} eq "") { - $error .= '
  • '.(eval $field{fieldLabel}).' '.WebGUI::International::get(451).'
    • '; + $error .= '
  • '.(WebGUI::Operation::Shared::secureEval($field{fieldLabel})).' '.WebGUI::International::get(451).'
    • '; }elsif($field{fieldName} eq "email" && isDuplicateEmail($data{$field{fieldName}})){ $warning .= '
  • '.WebGUI::International::get(1072).'
    • '; } @@ -145,13 +145,13 @@ sub www_editProfile { while($data = $a->hashRef) { $counter++; my %hash = (); - $category = eval $data->{categoryName}; + $category = WebGUI::Operation::Shared::secureEval($data->{categoryName}); $method = $data->{dataType}; - $label = eval $data->{fieldLabel}; - $default = eval $data->{dataDefault}; + $label = WebGUI::Operation::Shared::secureEval($data->{fieldLabel}); + $default = WebGUI::Operation::Shared::secureEval($data->{dataDefault}); if ($method eq "selectList" || $method eq "checkList" || $method eq "radioList") { - $values = eval $data->{dataValues}; + $values = WebGUI::Operation::Shared::secureEval($data->{dataValues}); my $orderedValues = {}; tie %{$orderedValues}, 'Tie::IxHash'; foreach my $ov (sort keys %{$values}) { @@ -231,16 +231,16 @@ sub www_viewProfile { and userProfileCategory.visible=1 and userProfileField.visible=1 order by userProfileCategory.sequenceNumber, userProfileField.sequenceNumber",WebGUI::SQL->getSlave); while (%data = $a->hash) { - $category = eval $data{categoryName}; + $category = WebGUI::Operation::Shared::secureEval($data{categoryName}); if ($category ne $previousCategory) { my $header; $header->{'profile.category'} = $category; push(@array,$header); } - $label = eval $data{fieldLabel}; + $label = WebGUI::Operation::Shared::secureEval($data{fieldLabel}); if ($data{dataValues}) { - $value = eval $data{dataValues}; + $value = WebGUI::Operation::Shared::secureEval($data{dataValues}); $value = ${$value}{$u->profileField($data{fieldName})}; } else { $value = $u->profileField($data{fieldName}); diff --git a/lib/WebGUI/Operation/ProfileSettings.pm b/lib/WebGUI/Operation/ProfileSettings.pm index cef396945..a9de30548 100644 --- a/lib/WebGUI/Operation/ProfileSettings.pm +++ b/lib/WebGUI/Operation/ProfileSettings.pm @@ -21,6 +21,7 @@ use WebGUI::International; use WebGUI::Privilege; use WebGUI::Session; use WebGUI::SQL; +use WebGUI::Operation::Shared; #------------------------------------------------------------------- sub _reorderCategories { @@ -72,7 +73,7 @@ sub _submenu { #------------------------------------------------------------------- sub www_deleteProfileCategoryConfirm { return WebGUI::Privilege::adminOnly() unless (WebGUI::Grouping::isInGroup(3)); - return WebGUI::AdminConsole->new("userProfiling")->render(WebGUI::Privilege::vitalComponent()) if ($session{form}{cid} < 1000 && $session{form}{cid} > 0); + return WebGUI::AdminConsole->new("userProfiling")->render(WebGUI::Privilege::vitalComponent()) if (length($session{form}{cid}) != 22 && $session{form}{cid} < 1000 && $session{form}{cid} > 0); WebGUI::SQL->write("delete from userProfileCategory where profileCategoryId=".quote($session{form}{cid})); WebGUI::SQL->write("update userProfileField set profileCategoryId='1' where profileCategoryId=".quote($session{form}{cid})); return www_editProfileSettings(); @@ -143,8 +144,6 @@ sub www_editProfileCategorySave { return WebGUI::Privilege::adminOnly() unless (WebGUI::Grouping::isInGroup(3)); my ($sequenceNumber, $test); $session{form}{categoryName} = 'Unamed' if ($session{form}{categoryName} eq "" || $session{form}{categoryName} eq "''"); - $test = eval($session{form}{categoryName}); - $session{form}{categoryName} = "'".$session{form}{categoryName}."'" if ($test eq ""); if ($session{form}{cid} eq "new") { $session{form}{cid} = WebGUI::Id::generate(); ($sequenceNumber) = WebGUI::SQL->quickArray("select max(sequenceNumber) from userProfileCategory"); @@ -236,7 +235,7 @@ sub www_editProfileField { tie %hash, 'Tie::CPHash'; %hash = WebGUI::SQL->buildHash("select profileCategoryId,categoryName from userProfileCategory order by categoryName"); foreach $key (keys %hash) { - $hash{$key} = eval $hash{$key}; + $hash{$key} = WebGUI::Operation::Shared::secureEval($hash{$key}); } $f->selectList( -name=>"profileCategoryId", @@ -255,8 +254,6 @@ sub www_editProfileFieldSave { return WebGUI::Privilege::adminOnly() unless (WebGUI::Grouping::isInGroup(3)); my ($sequenceNumber, $fieldName, $test); $session{form}{fieldLabel} = 'Unamed' if ($session{form}{fieldLabel} eq "" || $session{form}{fieldLabel} eq "''"); - $test = eval($session{form}{fieldLabel}); - $session{form}{fieldLabel} = "'".$session{form}{fieldLabel}."'" if ($test eq ""); if ($session{form}{dataDefault} && $session{form}{dataType}=~/List$/) { unless ($session{form}{dataDefault} =~ /^\[/) { $session{form}{dataDefault} = "[".$session{form}{dataDefault}; @@ -303,7 +300,7 @@ sub www_editProfileSettings { $output .= moveUpIcon('op=moveProfileCategoryUp&cid='.$category{profileCategoryId}); $output .= moveDownIcon('op=moveProfileCategoryDown&cid='.$category{profileCategoryId}); $output .= ' '; - $output .= eval $category{categoryName}; + $output .= WebGUI::Operation::Shared::secureEval($category{categoryName}); $output .= '
    '; $b = WebGUI::SQL->read("select * from userProfileField where profileCategoryId=".quote($category{profileCategoryId})." order by sequenceNumber"); @@ -314,7 +311,7 @@ sub www_editProfileSettings { $output .= moveUpIcon('op=moveProfileFieldUp&fid='.$field{fieldName}); $output .= moveDownIcon('op=moveProfileFieldDown&fid='.$field{fieldName}); $output .= ' '; - $output .= eval $field{fieldLabel}; + $output .= WebGUI::Operation::Shared::secureEval($field{fieldLabel}); $output .= '
    '; } $b->finish; diff --git a/lib/WebGUI/Operation/Shared.pm b/lib/WebGUI/Operation/Shared.pm index a37418484..01cf9ca34 100644 --- a/lib/WebGUI/Operation/Shared.pm +++ b/lib/WebGUI/Operation/Shared.pm @@ -18,6 +18,7 @@ use WebGUI::International; use WebGUI::Session; use WebGUI::SQL; use WebGUI::Style; +use Safe; our @ISA = qw(Exporter); our @EXPORT = qw(&menuWrapper); @@ -95,6 +96,38 @@ sub userStyle { } } +#------------------------------------------------------------------- +# This function is here to replace the dangerous eval calls in the User Profile System. +sub secureEval { + my $code = shift; + + # Handle WebGUI function calls + my %trusted = ( + 'WebGUI::International::get' => sub {WebGUI::International::get(@_)}, + 'WebGUI::International::getLanguages' => sub { WebGUI::International::getLanguages(@_) }, + 'WebGUI::DateTime::epochToHuman' => sub { WebGUI::DateTime::epochToHuman(@_) }, + 'WebGUI::Icon::getToolbarOptions' => sub { WebGUI::Icon::getToolbarOptions(@_) }, + ); + foreach my $function (keys %trusted ) { + while ($code =~ /($function\(([^)]*)\)\s*;*)/g) { + my $cmd = $1; + my @param = split (/,/,$2); + @param = map { s/^['"]|['"]$//g; $_; } @param; + my $output = $trusted{$function}(@param); + return $output if (ref $output); + $code =~ s/\Q$cmd/'$output'/g; + } + } + + # Execute simple perl code like ['English'] for default value. + # Inside the Safe compartment there's no WebGUI available + my $compartment = new Safe; + my $eval = $compartment->reval($code); + if ($eval) { + return $eval; + } + return $code; +} 1; diff --git a/lib/WebGUI/Operation/User.pm b/lib/WebGUI/Operation/User.pm index 6bb04e4df..6550726dd 100644 --- a/lib/WebGUI/Operation/User.pm +++ b/lib/WebGUI/Operation/User.pm @@ -280,13 +280,13 @@ sub www_editUser { order by userProfileCategory.sequenceNumber,userProfileField.sequenceNumber"); my $previousCategory; while(my %data = $a->hash) { - my $category = eval $data{categoryName}; + my $category = WebGUI::Operation::Shared::secureEval($data{categoryName}); if ($category ne $previousCategory) { $tabform->getTab("profile")->raw(''.$category.''); } - my $values = eval $data{dataValues}; + my $values = WebGUI::Operation::Shared::secureEval($data{dataValues}); my $method = $data{dataType}; - my $label = eval $data{fieldLabel}; + my $label = WebGUI::Operation::Shared::secureEval($data{fieldLabel}); my $default; if ($method eq "selectList" || $method eq "checkList" || $method eq "radioList") { my $orderedValues = {}; @@ -299,7 +299,7 @@ sub www_editUser { } elsif (defined $u->profileField($data{fieldName}) && (defined($values->{$u->profileField($data{fieldName})}))) { $default = [$u->profileField($data{fieldName})]; } else { - $default = eval $data{dataDefault}; + $default = WebGUI::Operation::Shared::secureEval($data{dataDefault}); } $tabform->getTab("profile")->$method( -name=>$data{fieldName}, @@ -313,7 +313,7 @@ sub www_editUser { } elsif (defined $u->profileField($data{fieldName})) { $default = $u->profileField($data{fieldName}); } else { - $default = eval $data{dataDefault}; + $default = WebGUI::Operation::Shared::secureEval($data{dataDefault}); } $tabform->getTab("profile")->$method( -name=>$data{fieldName},