NCSC-NL / taranis3

Taranis
Other
59 stars 17 forks source link

Taranis4U login failure when special characters are used in password #35

Closed bsi-lz closed 4 years ago

bsi-lz commented 4 years ago

We observed strange behaviour when users with same rights tried to login into Taranis4U. After a painfull investigation, we think it comes down to using the char '&' in passwords. After changing the password the user could login as desired. The user could login to taranis with the password (containing '&') without any problems.

'&' is the only char we found so far...

markov2 commented 4 years ago

I suspect this is an encoding issue. Internally, in Taranis, everything is stored in html-encoded form, so your password checks-out correctly via the web interface. The REST interface uses serialized but unencoded data fields.

markov2 commented 4 years ago

Hope you have time to test this fix in Taranis/REST/AccessToken.pm:

sub _create_access_token($) {
   ...
-   my $username = $login->{username};
-   my $password = $login->{password};
+  use HTML::Entities qw(encode_entities);
+  my $username = encode_entities $login->{username};
+  my $password = encode_entities $login->{password};
bsi-lz commented 4 years ago

So i tested your offered solution. It seems to work with some drawbacks. Using pw='123&123' it works. Usingpw='&%$§' it arrives in postdata as '&%$§' then encode_entities wont help cause of the additional char 'Â'. Can you tell where that char comes from?

Another solution might be to escape all chars in javascript in taranis4u then unescape in taranis...

markov2 commented 4 years ago

Errors in texts with capital 'A' versions always point to double encoded utf8: text which already was in utf8 encoding interpreted as latin1 text and the converted to utf8 again. This is orthogonal to HTML entity encoding, is used to avoid problems when text is mixed with HTML markup like <, > and "

It is often quite hard where the double-utf8 origins: you have to carefully debug each of the steps.

bsi-lz commented 4 years ago

i think this happens anywhere in CGI (?)... i will look arround / try to escape on side of taranis4u

bsi-lz commented 4 years ago

using encodeURIComponent(password) on taranis4u's side won't work - here i get the 'Â' aswell as part of the encoded string already. Atm best workarround would be - not all special chars are allowed for passwords. But that seems ugly :(

markov2 commented 4 years ago

The URI encoding could help, but usually does not. It is not about character serialization but about incorrect handling of character sets. There is quite a long path between supplying passwords and checking them. In each step, you have to handle the character-set correctly, otherwise you get this (very common) kind of utf8 problems.

For instance, if the password is stored in a file, the file must be read with open(..., "<:encoding(utf8)", ...). Otherwise, when written to an utf8 output (like the database) it will get "upgraded" from presumed Latin1 into utf8 which results in the double encoding which you witness. So: any input and any output must handle this perfectly... which makes it hard to debug.

The main parts of Taranis were developed before utf8 was supported by Perl. The Postgres database connectors did not support it transparently until a few years ago. So: I am sure there are plenty of bugs in this respect still to be fixed. (See man 3pm utf8 for debugging helpers)

bsi-lz commented 4 years ago

well - this is just a "minor" bug and it consumed alot of time already. i will mark it as "wont-fix" internally. I will add some input-restrictions for special chars and a hint for the user when setting new passwords. will add a diff as anwer I will keep your encode_entities fix.

diff --git a/pm/Taranis/REST/AccessToken.pm b/pm/Taranis/REST/AccessToken.pm
index b2117ee..8b6ea57 100644
--- a/pm/Taranis/REST/AccessToken.pm
+++ b/pm/Taranis/REST/AccessToken.pm
@@ -16,6 +16,7 @@ use SQL::Abstract::More;

 Taranis::REST->addRoute(
        route    => qr[^auth/?$],
@@ -45,14 +46,16 @@ sub _create_access_token($) {

        my $request  = $args{request};
        my $postdata = $request->param('POSTDATA');
+
        unless($postdata) {
                print CGI->header( -status => '406 Not acceptable');
                return;
        }
+       my $login = from_json($postdata, {utf8 => 1});

-       my $login    = from_json $postdata;
        my $username = encode_entities $login->{username};
        my $password = encode_entities $login->{password};
+
        my $is_valid_user = 0;

        my $users       = Taranis::Users->new($config);
bsi-lz commented 4 years ago

Possible workarround:

diff --git a/pm/Taranis/REST/AccessToken.pm b/pm/Taranis/REST/AccessToken.pm
index b2117ee..44c01fd 100644
--- a/pm/Taranis/REST/AccessToken.pm
+++ b/pm/Taranis/REST/AccessToken.pm
@@ -45,14 +45,16 @@ sub _create_access_token($) {

        my $request  = $args{request};
        my $postdata = $request->param('POSTDATA');
+
        unless($postdata) {
                print CGI->header( -status => '406 Not acceptable');
                return;
        }
+       my $login = from_json($postdata, {utf8 => 1});

-       my $login    = from_json $postdata;
        my $username = encode_entities $login->{username};
        my $password = encode_entities $login->{password};
+
        my $is_valid_user = 0;

diff --git a/templates/user_panel.tt b/templates/user_panel.tt
index dcb2e01..560f1c6 100644
--- a/templates/user_panel.tt
+++ b/templates/user_panel.tt
@@ -23,7 +23,21 @@
                                        </div><br>
                                        <div class="dialog-input-wrapper block">
                                                <label for="user-panel-new-password" class="dialog-input-label">New password*</label><br>
-                                               <input type="password" name="new_pwd" id="user-panel-new-password" class="input-default dialog-input-text-wider">
+                                               <span class="note">
+                                                       <b>Hint:</b>Mind following policies:<br>
+                                                       <ul style="list-style: disc !important; padding-left: 2em; line-height: 1.2em;">
+                                                               <li>At least 1 Uppercase (A-Z|ÖÄÜ)</li>
+                                                               <li>At least 1 Lowercase (a-z|öäü)</li>
+                                                               <li>At least 1 Number (0-9)</li>
+                                                               <li>At least 1 Symbol, symbols allowed: !@#$%^&*_=+-</li>
+                                                               <li>Symbols <b>NOT</b> allowed: \n\s§</li>
+                                                               <li> Min 8 chars to max 60 chars total</li>
+                                                       </ul>
+                                               </span>
+                                               <input type="password" name="new_pwd" id="user-panel-new-password" class="input-default dialog-input-text-wider"
+                                               pattern="^(?=.*[a-z|üäö])(?=.*[A-Z|ÜÄÖ])(?=.*[0-9])(?=.*[!@#$%^&*_=+-])(?!.*[\n\s§]).{8,60}$">
+                                               <br>
+                                               <input type="checkbox" onclick="showPW()">Show Password
                                        </div><br>
                                        <div class="dialog-input-wrapper block">
                                                <label for="user-panel-confirm-password" class="dialog-input-label">Confirm new password*</label><br>

diff --git a/webinterface/include/js/user_panel.js b/webinterface/include/js/user_panel.js
index 71b8051..c2c18be 100644
--- a/webinterface/include/js/user_panel.js
+++ b/webinterface/include/js/user_panel.js
@@ -284,3 +284,12 @@ function saveKeyboardShortcutSettingCallback ( params ) {
                alert( params.message );
        }
 }
+
+function showPW() {
+  let x = $("#user-panel-new-password");
+  if (x.attr('type') === "password") {
+    x.attr('type',"text");
+  } else {
+    x.attr('type',"password");
+  }
+}
markov2 commented 4 years ago
> --- a/pm/Taranis/REST/AccessToken.pm
> +++ b/pm/Taranis/REST/AccessToken.pm
> @@ -45,14 +45,16 @@ sub _create_access_token($) {
> +       my $login = from_json($postdata, {utf8 => 1});
> -       my $login    = from_json $postdata;

Does this improve the behavior, or is your expectation that this is closer to correct behavior? Docs say decode_json() is preferred here.

> diff --git a/templates/user_panel.tt b/templates/user_panel.tt
> --- a/templates/user_panel.tt
> +++ b/templates/user_panel.tt
> +                  <li>At least 1 Uppercase (A-Z|ÖÄÜ)</li>
> +                  <li>At least 1 Lowercase (a-z|öäü)</li>

I experienced there are different versions of ö under utf8. Windows and Linux keyboards produce "LATIN SMALL LETTER O WITH DIAERESIS", where MacOS produces "COMPOSE DIAERESIS" + "LATIN SMALL LETTER O". So: when you use the other platform to type in your password, it will not match. Do you have a different experience?

bsi-lz commented 4 years ago

Does this improve the behavior, or is your expectation that this is closer to correct behavior? Docs say decode_json() is preferred here.

When testing with pw="öäü&%$":

# not working
my $login = from_json $postdata; 
#-> &Atilde;&para;&Atilde;&curren;&Atilde;&frac14;&amp;%$';

# working
my $login = from_json($postdata, {utf8 =>1});
#-> &ouml;&auml;&uuml;&amp;%$

# working aswell
my $login = decode_json($postdata);
#-> &ouml;&auml;&uuml;&amp;%$

# outputs printed after used encode_entities

I experienced there are different versions of ö under utf8. Windows and Linux keyboards produce "LATIN SMALL LETTER O WITH DIAERESIS", where MacOS produces "COMPOSE DIAERESIS" + "LATIN SMALL LETTER O". So: when you use the other platform to type in your password, it will not match. Do you have a different experience?

"Luckily" we have Linux/Windows users only on taranis.

markov2 commented 4 years ago

I have taken the 'decode_json' and the 'show password' flag. However, I expect from security specialists that they know how to produce a safe password. Myself, I prefer a three or four random words.

markov2 commented 4 years ago

Included in release 3.7.4, just published