From c50b49db96fbc5b3df79342ea23ae58cd92f22b8 Mon Sep 17 00:00:00 2001 From: Kirill Solomko Date: Thu, 29 Jul 2021 17:46:31 +0200 Subject: [PATCH] TT#133800 fix password fields behaviour in API * PATCH: password fields are not removed when resource is created for apply_patch(), they are removed under the same condititions later when hal is generated, that is to ensure that admin users without the 'show_passwords' flag as well as subscribers will not run into situation when they use PATCH and cannot apply it for "path": "/password" or/and "path": "/webpassword", as they were removed before apply_patch() * rework encrypted webpassword detection. webpasword is detected as encrypted if its length is 54 or 56 and it contains at least one '$' char, there is a chance for false positive detection when a user provides with a plain-text password with the same pattern but it's very unlikely, as well as since mr8.5 webpasswords are expected to be encrypted, and moreover worth case scenario is that the plain-text password will not be returned to the user Change-Id: I8ea739cbf728b2134f3ce00cee29da42ab3fb4a3 --- lib/NGCP/Panel/Controller/API/Subscribers.pm | 2 - .../Panel/Controller/API/SubscribersItem.pm | 2 - lib/NGCP/Panel/Role/API/Subscribers.pm | 42 ++++++++++++++----- lib/NGCP/Panel/Utils/Subscriber.pm | 6 ++- 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/lib/NGCP/Panel/Controller/API/Subscribers.pm b/lib/NGCP/Panel/Controller/API/Subscribers.pm index 09113c8c84..203c193616 100644 --- a/lib/NGCP/Panel/Controller/API/Subscribers.pm +++ b/lib/NGCP/Panel/Controller/API/Subscribers.pm @@ -335,7 +335,6 @@ sub GET :Allow { now => $now) if !exists $contract_map{$contract->id}; #apply underrun lock level $contract_map{$contract->id} = 1; my $resource = $self->resource_from_item($c, $subscriber, $form); - delete $resource->{webpassword}; # since it's encrypted, no point to return it push @embedded, $self->hal_from_item($c, $subscriber, $resource, $form); push @links, Data::HAL::Link->new( relation => 'ngcp:'.$self->resource_name, @@ -473,7 +472,6 @@ sub POST :Allow { my ($_form) = $self->get_form($c); my $_subscriber = $self->item_by_id($c, $subscriber->id); my $_resource = $self->resource_from_item($c, $_subscriber, $_form); - delete $_resource->{webpassword}; # since it's encrypted, no point writing it into journal as well return $self->hal_from_item($c,$_subscriber,$_resource,$_form); }); $guard->commit; diff --git a/lib/NGCP/Panel/Controller/API/SubscribersItem.pm b/lib/NGCP/Panel/Controller/API/SubscribersItem.pm index 5c38dd36b0..712b5ae796 100644 --- a/lib/NGCP/Panel/Controller/API/SubscribersItem.pm +++ b/lib/NGCP/Panel/Controller/API/SubscribersItem.pm @@ -101,7 +101,6 @@ sub PUT :Allow { last unless $subscriber; $resource = $self->resource_from_item($c, $subscriber, $form); - delete $resource->{webpassword}; # since it's encrypted, no point to return it my $hal = $self->hal_from_item($c, $subscriber, $resource, $form); last unless $self->add_update_journal_item_hal($c,$hal); @@ -151,7 +150,6 @@ sub PATCH :Allow { last unless $subscriber; $resource = $self->resource_from_item($c, $subscriber, $form); - delete $resource->{webpassword}; # since it's encrypted, no point to return it my $hal = $self->hal_from_item($c, $subscriber, $resource, $form); last unless $self->add_update_journal_item_hal($c,$hal); diff --git a/lib/NGCP/Panel/Role/API/Subscribers.pm b/lib/NGCP/Panel/Role/API/Subscribers.pm index 2465a84896..cfdc46c908 100644 --- a/lib/NGCP/Panel/Role/API/Subscribers.pm +++ b/lib/NGCP/Panel/Role/API/Subscribers.pm @@ -55,8 +55,17 @@ sub resource_from_item { delete $prov_resource->{domain_id}; delete $prov_resource->{account_id}; my %resource = %{ merge($bill_resource, $prov_resource) }; + my $delete_passwords = 1; $resource{administrative} = delete $resource{admin}; + if ($c->request->method eq 'PATCH' && !$resource{pre_patch_resource}) { + $delete_passwords = 0; + $resource{pre_patch_resource} = 1; + } else { + $delete_passwords = 1; + delete $resource{pre_patch_resource}; + } + unless($customer->product->class eq 'pbxaccount') { delete $resource{is_pbx_group}; delete $resource{is_pbx_pilot}; @@ -76,10 +85,19 @@ sub resource_from_item { $resource{email} = undef; $resource{timezone} = undef; } - if (length($resource{webpassword}) - and $resource{webpassword} =~ /^([^\$]+)\$([^\$]+)$/ - and length($1) == ceil(4 * (($NGCP::Panel::Utils::Auth::SALT_LENGTH / 8) / 3))) { - delete $resource{webpassword}; + # if the webpassword length is 54 or 56 chars and it contains $, + # we assume that the password is encrypted, + # as we do not have an explicit flag for the password field + # wether it's encrypted or not, there is a chance that + # if somebody manages to create a 54 chars password containing + # '$', it will be detected as false positive, but + # - all webpasswords from mr8.5+ are meant to be encrypted + # - in case of the false positive result, the worse that happens + # the password is not returned to the user in plain-text + if ($delete_passwords && + $resource{webpassword} && length $resource{webpassword} =~ /^(54|56)$/ && + $resource{webpassword} =~ /\$/) { + delete $resource{webpassword}; } if(!$form){ ($form) = $self->get_form($c); @@ -180,16 +198,18 @@ sub resource_from_item { if ($c->user->show_passwords) { foreach my $k(qw/password webpassword/) { eval { - $resource{$k} = NGCP::Panel::Utils::Encryption::encrypt_rsa($c,$resource{$k}); + if ($resource{$k}) { + $resource{$k} = NGCP::Panel::Utils::Encryption::encrypt_rsa($c,$resource{$k}); + } }; if ($@) { $c->log->error("Failed to encrypt $k '$resource{$k}': " . $@); - delete $resource{$k}; + delete $resource{$k} if $delete_passwords; } } } else { foreach my $k(qw/password webpassword/) { - delete $resource{$k}; + delete $resource{$k} if $delete_passwords; } } } else { @@ -201,16 +221,16 @@ sub resource_from_item { # TODO: make custom filtering configurable! foreach my $k(qw/password webpassword/) { - delete $resource{$k}; + delete $resource{$k} if $delete_passwords; } } if($c->user->roles eq "subscriberadmin") { $resource{customer_id} = $contract_id; if(!$c->config->{security}->{password_sip_expose_subadmin}) { - delete $resource{password}; + delete $resource{password} if $delete_passwords; } if(!$c->config->{security}->{password_web_expose_subadmin}) { - delete $resource{webpassword}; + delete $resource{webpassword} if $delete_passwords; } } } @@ -496,7 +516,7 @@ sub update_item { $c->log->error("failed to update subscriber, alias " . $c->qs($1) . " already exists"); # TODO: user, message, trace, ... $self->error($c, HTTP_UNPROCESSABLE_ENTITY, "Number '" . $1 . "' already exists.", "Number already exists."); return; - } + } my $billing_res = { external_id => $resource->{external_id}, diff --git a/lib/NGCP/Panel/Utils/Subscriber.pm b/lib/NGCP/Panel/Utils/Subscriber.pm index fdfd7e3031..988bdb0f43 100644 --- a/lib/NGCP/Panel/Utils/Subscriber.pm +++ b/lib/NGCP/Panel/Utils/Subscriber.pm @@ -320,10 +320,12 @@ sub prepare_resource { } } } - + foreach my $k(qw/password webpassword/) { eval { - $resource->{$k} = NGCP::Panel::Utils::Encryption::decrypt_rsa($c,$resource->{$k}); + if ($resource->{$k}) { + $resource->{$k} = NGCP::Panel::Utils::Encryption::decrypt_rsa($c,$resource->{$k}); + } }; if ($@) { $c->log->error("Failed to decrypt $k '$resource->{$k}': " . $@);