Merge branch 'master' of gitlab.science.ru.nl:mlubbers/ssproject1617
[ssproject1617.git] / report / v2_authentication.tex
index adfc0f8..eb512cc 100644 (file)
@@ -1,11 +1,11 @@
 
-\begin{enumerate}[label={2.\arabic*}]
+\begin{enumerate}[label={V2.\arabic*}]
 
 \item
 \fail{}
 Verify all pages and resources by default require
 authentication except those specifically intended to be
-public (Principle of complete mediation).
+public (principle of complete mediation).
 
 \begin{result}
     All admin pages are private. Unpublished posts are also private.
@@ -15,15 +15,19 @@ public (Principle of complete mediation).
 \end{result}
 
 \item
-\pass{}
+\fail{}
 Verify that forms containing credentials are not filled in by
 the application. Pre-filling by the application implies that
-credentials are stored in plaintext or a reversible format,
+credentials are stored in plain-text or a reversible format,
 which is explicitly prohibited.
 
 \begin{result}
-No credentials (that should not be stored in plain text) are ever filled in by
-the application.
+No credentials that come from the database are pre-filled by the application.
+However, in some forms, the application pre-fills password fields from the
+request's POST data. This is not necesarry.\footnote{This issue was actually
+overlooked when auditing manually, and was found when running the Fortify tool.
+In the initial audit, we only ensured that no internal information (from the
+database) was leaked in this way.}
 \end{result}
 
 \setcounter{enumi}{3}
@@ -36,7 +40,7 @@ server side.
 \begin{result}
     All authentication controls (login credentials and client cookies) are
     enforced by the application. Note however item~\ref{auth:6}, about the
-    security of these controls in the immplementation.
+    security of these controls in the implementation.
 \end{result}
 
 \setcounter{enumi}{5}
@@ -48,10 +52,10 @@ attackers cannot log in.
 
 \begin{result}
     The input to various forms is not sanitized at all. This makes the implementation
-    in (not only) \texttt{Users::find} vulnerable to SQL injections. The login form
-    is also vulnerable. Any user can execute arbitrary SQL code from the username field.
+    in (not only) \code{Users::find} vulnerable to \SQL{} injections. Through this vulnerability,
+    an attacker can execute arbitrary \SQL{} code from the username field in the login form.
     The following example code can submitted as username in the login form, which
-    will set the password of the \texttt{admin} user to \texttt{s3cret}:
+    will set the password of the \code{admin} user to \code{s3cret}:
 
     \code{"; UPDATE users SET password='\$1\$OWgsBb90\$Lkko6aZwmp9XOVrFI09Ab0' WHERE \\username='admin' AND 'a' = "a}
 
@@ -61,13 +65,13 @@ attackers cannot log in.
 \item
 \pass{}
 Verify password entry fields allow, or encourage, the use
-of passphrases, and do not prevent password managers,
-long passphrases or highly complex passwords being
+of pass-phrases, and do not prevent password managers,
+long pass-phrases or highly complex passwords being
 entered.
 
 \begin{result}
-    The application allows the user to use any password (except ones that contain
-    SQL code).
+    The application allows the user to use any password (except ones that trigger
+    errors in injected \SQL{} code).
 \end{result}
 
 \item
@@ -89,7 +93,7 @@ authentication mechanism.
     expire, however. It would be even better to require that a token be used
     withing a day (or so) after creation.
 
-    The security of the SMTP connection is at the discretion of the web server.
+       The security of the \SMTP{} connection is at the discretion of the web server.
     Often these connections are not very secure, but this worry is beyond the
     scope of this audit.
 \end{result}
@@ -126,11 +130,14 @@ salt, and there is sufficient work factor to defeat brute
 force and password hash recovery attacks.
 
 \begin{result}
-    Password are stored in database using the PHP function \texttt{crypt}. Internally, this
-    function uses salted MD5. This is way too reverse with brute-force attacks using dictionary files.
-
-    Instead it would be better to use the \texttt{argon2} password hashing algorithm
-    or the PHP \texttt{password\_hash} function (which currently uses BCRYPT).
+    Password are stored in database using the \PHP{} function \code{crypt}. Internally, this
+    function uses salted MD5 on modern UNIX systems. On legacy systems, this uses an old DES
+    based algorithm, which uses only the first eight characters of the supplied password.
+    This is  way too easy to reverse with brute-force attacks using dictionary files.
+
+    Instead the \CMS{} should use the \code{argon2} password hashing algorithm
+    or the \PHP{} \code{password\_hash} function (which currently uses BCRYPT) which are
+    (at this moment) considered safe to use.
 \end{result}
 
 \setcounter{enumi}{15}
@@ -143,11 +150,12 @@ user to enter credentials are done so using an encrypted
 link.
 
 \begin{result}
-    The app allows admin users to log in over HTTP. This is insecure, as it allows
-    eavesdroppers to intercept password.
-    The app should force HTTPS for at least the login form, the \texttt{admin\_controller} and
+    The app allows admin users to log in over \HTTP{}. This is insecure, as it allows
+    eavesdroppers to intercept any password.
+    The app should force \HTTPS{} for at least the login form, the \code{admin\_controller} and
     for the installation script (because the users posts secrets like the database
-    password to this page).
+    password to this page and the \CMS{} supplies the user with the credentials of the \code{admin}
+    account).
 \end{result}
 
 \item
@@ -163,16 +171,27 @@ that the new password is not sent in clear text to the user.
     information.
 \end{result}
 
-\TODO{\item
-
+\item
+\fail{}
 Verify that information enumeration is not possible via
-login, password reset, or forgot account functionality.}
+login, password reset, or forgot account functionality.
 
-\TODO{\item
+\begin{result}
+    All these forms are vulnerable to \SQL{} injection attacks. So any information
+    can leak any information from the database.
+\end{result}
 
+\item
+\pass{}
 Verify there are no default passwords in use for the
 application framework or any components used by the
-application (such as “admin/password”).}
+application (such as “admin/password”).
+
+\begin{result}
+    No secrets are initialized by predefined values. The admin user will have
+    username \code{admin} by default. This is no secret and therefore not
+    considered unsafe.
+\end{result}
 
 \item
 \fail{}
@@ -188,85 +207,156 @@ attacks.
         \item Email validation, to harden brute force email address discovery
         \item Installation database check, to prevent guessing attacks for the database password
         \item Login, to prevent login guessing
-        \item And comment submission, to prevent spam, phishing et cetera (by using CAPTCHA).
+        \item And comment submission, to prevent spam, phishing et cetera (by
+        using some CAPTCHA software.
     \end{itemize}
 \end{result}
 
-\TODO{\item
-
+\item
+\fail{}
 Verify that all authentication credentials for accessing
 services external to the application are encrypted and
-stored in a protected location.}
+stored in a protected location.
 
-\TODO{\item
+\begin{result}
+    The database credentials are hard-coded in \code{config.php}. While it
+    would be better to pass secrets as environment variables, this is not
+    really bad practice.
+
+    However, the installation instructions state the following:
+    \begin{verbatim}
+Change the file permissions to allow all users write access to the
+folder you extracted TestCMS to.
+    \end{verbatim}
+    This implies making the configuration file readable for all users on the
+    system. This information should not be accessible for any user other than
+       running the \PHP{} script.
+\end{result}
 
+\item
+\pass{}
 Verify that forgotten password and other recovery paths
-use a TOTP or other soft token, mobile push, or other
+use a \TOTP{} or other soft token, mobile push, or other
 offline recovery mechanism. Use of a random value in an
-e-mail or SMS should be a last resort and is known weak.}
+e-mail or SMS should be a last resort and is known weak.
 
-\TODO{\item
+\begin{result}
+    The password recovery path uses a random looking token. It is sent over
+    e-mail, which is considered weak (but not unsafe).
+\end{result}
 
+\item
+\fail{}
 Verify that account lockout is divided into soft and hard
 lock status, and these are not mutually exclusive. If an
 account is temporarily soft locked out due to a brute force
-attack, this should not reset the hard lock status.}
+attack, this should not reset the hard lock status.
 
-\TODO{\item
+\begin{result}
+    The application has not implemented any lockout mechanisms.
+\end{result}
 
+\item
+\pass{}
 Verify that if shared knowledge based questions (also
-known as "secret questions") are required, the questions
+known as ``secret questions'') are required, the questions
 do not violate privacy laws and are sufficiently strong to
-protect accounts from malicious recovery.}
+protect accounts from malicious recovery.
 
-\TODO{\item
+\begin{result}
+    The application uses no shared knowledge based questions, and thus not
+    violate any privacy laws.
+\end{result}
 
+\item
+\fail{}
 Verify that the system can be configured to disallow the
-use of a configurable number of previous passwords.}
+use of a configurable number of previous passwords.
 
-\TODO{\item
+\begin{result}
+    The system does not remember any previously used passwords and does not
+    require variation in the use of different passwords.
+\end{result}
 
+\item
+\pass{}
 Verify that risk based re-authentication, two factor or
-transaction signing is in place for high value transactions.}
+transaction signing is in place for high value transactions.
 
-\TODO{\item
+\begin{result}
+    There are no (really) risk based action for which re-authentication would be
+    fit.
+\end{result}
 
+\item
+\fail{}
 Verify that measures are in place to block the use of
-commonly chosen passwords and weak passphrases.}
+commonly chosen passwords and weak pass-phrases.
 
-\TODO{\item
+\begin{result}
+    No password strengthening measures are implemented. The app should
+    use some password strength estimator like \texttt{zxcvbn}\footnote{\url{https://github.com/dropbox/zxcvbn}}.
+\end{result}
 
+\notapplicable{
+\item
+% \fail{}
 Verify that all authentication challenges, whether
 successful or failed, should respond in the same average
-response time.}
+response time.
 
-\TODO{\item
+% \begin{result}
+%     String comparisation for checking password hashes and password reset
+%     tokens are not in constant time.
+% \end{result}
+}
 
-Verify that secrets, API keys, and passwords are not
+\notapplicable{
+\item
+% \fail{}
+Verify that secrets, \API{} keys, and passwords are not
 included in the source code, or online source code
-repositories.}
+repositories.
 
-\setcounter{enumi}{30}
+% \begin{result}
+%     The database credentials are hard coded in \code{config.php}. These
+%     credentials should ideally be passed using environment variables.
+% \end{result}
+}
 
-\TODO{\item
+\setcounter{enumi}{30}
 
+\item
+\fail{}
 Verify that if an application allows users to authenticate,
 they can authenticate using two-factor authentication or
 other strong authentication, or any similar scheme that
 provides protection against username + password
-disclosure.}
+disclosure.
 
-\TODO{\item
+\begin{result}
+    No such features are implemented.
+\end{result}
 
+\item
+\fail{}
 Verify that administrative interfaces are not accessible to
-untrusted parties.}
+untrusted parties.
 
-\TODO{\item
+\begin{result}
+    Any authenticated user is allowed to view and use the administrative
+    interface. A separation should be made between administrators and normal
+    users.
+\end{result}
 
-Browser autocomplete, and integration with password
+\item
+\pass{}
+Browser auto-complete, and integration with password
 managers are permitted unless prohibited by risk based
 policy.
 
-}
+\begin{result}
+    Browser auto-complete functionality is not restricted in any way.
+\end{result}
 
 \end{enumerate}