300093 - index.cgi remains unsecure when the SSL parameter is set to "authenticated sessions"

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Frédéric Buclin]

Bug 165075 implemented the ability to log in from the home page index.cgi. When
the "ssl" parameter is set to "authenticated sessions", all login pages should
be secured. But index.cgi isn't.

Note: index.cgi behaves as expected when the "ssl" parameter is set to "always".

Comment 1

[Dave Miller [:justdave]]

See http://www.squarefree.com/2005/05/28/banks-and-https/ for more information
on this.

Comment 2

[Frédéric Buclin]

Bugzilla/CGI.pm, line 57:

    # Redirect to SSL if required
    if (Param('sslbase') ne '' and Param('ssl') eq 'always') {
        $self->require_https(Param('sslbase'));
    }


I think this is the right place to add index.cgi if Param('ssl') eq
'authenticated sessions'. How does CGI.pm get the file name being called?

Comment 3

[:glob ✱]

> I think this is the right place to add index.cgi if Param('ssl') eq
> 'authenticated sessions'.

rather than adding a filename check to CGI, i think it would be better to add
sslbase to the form if required - login.html.tmpl and login-small.html.tmpl.

> How does CGI.pm get the file name being called?

it doesn't, but $cgi->script_name is what you're after.

Comment 4

[:glob ✱]

Attachment: v1

updates login-small to use sslbase if required

note that cgi.script_name returns the full virtual path to the file, so we need
to use basename to grab just the filename.

Comment 5

[Frédéric Buclin]

Comment on attachment 189007 [details] [diff] [review]
v1

>Index: Bugzilla/Template.pm

>+            # File::Basename::basename wrapper
>+            'basename' => sub { return (fileparse(@_[0]))[0]; },

When running checksetup.pl (yes, checksetup, not runtests!), a warning is
displayed:
Scalar value @_[0] better written as $_[0] at Bugzilla/Template.pm line 427.


>Index: template/en/default/account/auth/login-small.html.tmpl

>-<form name="login" action="[% cgi.script_name FILTER html %]" method="POST">
>+[% IF Param('ssl') == 'authenticated sessions' %]
>+  <form name="login" action="[% Param('sslbase') FILTER html %][% basename(cgi.script_name) FILTER html %]" method="POST">
>+[% ELSE %]
>+  <form name="login" action="[% cgi.script_name FILTER html %]" method="POST">
>+[% END %]

This is not the right fix. CGI.pm actually redirects you to the https:// URL
when required. But index.cgi itself remains unsecure and is then untrustable
when filling the login form.

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

glob seems busy these days. Taking!

Comment 7

[Frédéric Buclin]

Comment on attachment 191629 [details] [diff] [review]
patch, v2

mkanat, have you time to review this blocker?

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 191629 [details] [diff] [review]
patch, v2

>Index: index.cgi
>+# Force to use HTTPS unless Param('ssl') equals 'never'.
>+# This is required because the user may want to log in from here.
>+if (Param('sslbase') ne '' and Param('ssl') ne 'never') {
>+    $cgi->require_https(Param('sslbase'));
>+}

I wonder if we should document this somewhere...  If someone edits their login
page so it doesn't have the login form anymore, they might want to remove this
requirement. (not necessarily part of this bug, just thinking out loud). 
Although, maybe we should make it easy to change, i.e. put both "if" statements
in there, and comment out one of them, so that they can easily toggle which one
is commented out...

>-        <a href="[% Param('urlbase') %]">Home</a> | 
>+        <a href="index.cgi">Home</a> | 

The user should never need to know that index.cgi exists.  We can use href="./"

Comment 9

[Frédéric Buclin]

Attachment: patch, v3

replace index.cgi by ./

Comment 10

[Frédéric Buclin]

tip:

Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.15; previous revision: 1.14
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.41; previous revision: 1.40
done


2.20rc2:

Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.13.10.1; previous revision: 1.13
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.39.2.1; previous revision: 1.39
done