From: Kelley van Evert Date: Mon, 14 Nov 2016 12:50:12 +0000 (+0100) Subject: Added a chapter analysing Fortify's results; some general beautification; modified... X-Git-Url: https://git.martlubbers.net/?a=commitdiff_plain;h=b4eaf4d5efb8cea0c57c5d14be25ff38c1deeb2d;p=ssproject1617.git Added a chapter analysing Fortify's results; some general beautification; modified [make clean] so that it doesn't remove the resulting pdf (sorry Mart, I know you won't agree with this change!) --- diff --git a/report/Makefile b/report/Makefile index d82ce26..8d4dc16 100644 --- a/report/Makefile +++ b/report/Makefile @@ -21,5 +21,5 @@ all: $(DOC).pdf $(LATEX) $(LATEXFLAGS) $< || true clean: - $(RM) $(addprefix $(DOC).,aux log fmt toc bbl blg mlog run.xml pdf out)\ + $(RM) $(addprefix $(DOC).,aux log fmt toc bbl blg mlog run.xml out)\ $(DOC)-blx.bib logo.png diff --git a/report/fortify.tex b/report/fortify.tex new file mode 100644 index 0000000..8e2bb66 --- /dev/null +++ b/report/fortify.tex @@ -0,0 +1,95 @@ + + +\subsection{Fortify's results, summarized} + +Fortify's results can be summarized to the following: + +\begin{enumerate}[label=(\Alph*)] + \item 50 cases of \textbf{XSS} vurnerabilities, all labeled \textbf{critical}, because none of the CMS's forms include nonces / protection against XSS is indeed missing. + \item \textbf{Password management}. In a user password reset form in \code{reset.php}, if the resetting fails, the password the user just entered reappears in the password field. This is not a database-retrieved password, and hence not actually as \textbf{critical} as Fortify labels it, but of course bad practice nonetheless. + \item In the \textbf{privact violation} category, Fortify found errors and warnings printed back to the browser, and labelled it \textbf{critical}. However, this happens in the installer script, which we have decided to treat separately, as explained earlier. + \item \textbf{SQL injection} attacks are possible on the installer script, labelled \textbf{critical}. Yet again: the installer script. + \item \textbf{Cookie security}: the \code{HttpOnly} header is not set, labelled \textbf{high}. + \item \textbf{Privacy violation}: HTML forms don't disable autocompletion. Labelled \textbf{high}. However, autocompletion of HTML forms by means of the \code{autocompletion="none"} attribute notoriously doesn't really work. The larger problem is that the post/redirect/get pattern is not followed, as stated above at our analysis of OWASP requirement (9.1). + \item Fortify complains that PHP's \code{crypt(...)} function is \textbf{weak encryption} and labels the 5 usages \textbf{high}. +\end{enumerate} + + +\subsection{Analysis} + +The main point that must be observed is that all the above results are quite low-level of nature. The majority of the OWASP ASVS requirements are of a more high-level nature. Two good examples are: + +\begin{description} + \item[V4.9] Verify that the same access control rules implied by the presentation layer are enforced on the server side. \\ + (\textit{The CMS failed this requirement in our analysis.}) + \item[V5.17] Verify that the application has defenses against HTTP parameter pollution attacks, particularly if the application framework makes no distinction about the source of request parameters (GET, POST, cookies, headers, environment, etc.. \\ + (\textit{The CMS passed this requirement in our analysis.}) +\end{description} + +For this reason, Fortify was nowhere near able to identifying all the problems we found in the CMS. An overview of our findings, where Fortify's concurrences are outlined explicitly, is given by the table below. + + +\newcommand{\p}{{\color{lightgray}\pass}} +\newcommand{\X}{\fail} +\setlength\fboxrule{1pt} +\setlength\fboxsep{4pt} +\newcommand{\F}[2]{% + \hspace*{-5pt}% + \boxed{\textrm{#2}}$^{\,\textrm{\small(#1)}}$% + \hspace*{-5pt}% +}% fortify-found security problem: \F\X + +\begin{table}[th!] +\centering +%\renewcommand{\arraystretch}{1} +\begin{tabular}{@{}lllllllllll@{}} +\toprule +\# & +\textbf{V2} & +\textbf{V3} & +\textbf{V4} & +\textbf{V5} & +\textbf{V7} & +\textbf{V8} & +\textbf{V9} & +\textbf{V12} & +\textbf{V17} \\ +\midrule +% V2 V3 V4 V5 V7 V8 V9 V12 V17 + 1 & \X & \p & \p & \p & & \X & \F{B}\X & \X & \TODO \\ + 2 & \F{B}\p & \p & & & \p & \p & & \p & \TODO \\ + 3 & & \X & & \X & & \X & \p & & \TODO \\ + 4 & \p & & \p & & & \X & \X & \X & \TODO \\ + 5 & & \p & \p & \p & & \p & \p & \p & \TODO \\ + 6 & \X & \p & & & & \p & & \X & \TODO \\ + 7 & \p & \X & & & \TODO & \p & \p & \X & \TODO \\ + 8 & \p & & \p & & & & & \X & \\ + 9 & \X & \p & \X & & & & \p & & \TODO \\ +10 & & \X & \p & \X & & \X & \p & & \TODO \\ +11 & & \p & & \p & & & \p & & \TODO \\ +12 & \X & \X & \X & \p & & & & & \\ +13 & \X & \X & \F{A}\X & \p & & \X & & & \\ +14 & & & \X & \p & & & & & \\ +15 & & & \X & \X & & & & & \\ +16 & \X & & \X & \p & & & & & \\ +17 & \p & & & \p & & & & & \\ +18 & \X & & & \X & & & & & \\ +19 & \p & & & \X & & & & & \\ +20 & \X & & & \p & & & & & \\ +21 & \X & & & \p & & & & & \\ +22 & \p & & & \X & & & & & \\ +23 & & & & \X & & & & & \\ +24 & & & & \p & & & & & \\ +25 & \X & & & \p & & & & & \\ +26 & & & & \p & & & & & \\ +27 & \X & & & & & & & & \\ +28 & \X & & & & & & & & \\ +29 & \X & & & & & & & & \\ +30 & & & & & & & & & \\ +31 & & & & & & & & & \\ +32 & \X & & & & & & & & \\ +33 & \p & & & & & & & & \\ +\bottomrule +\end{tabular} +\caption{Summary of our results. Fortify's findings are outlined and labelled, see our analysis above.} +\end{table} diff --git a/report/organization.tex b/report/organization.tex index e69de29..77eca21 100644 --- a/report/organization.tex +++ b/report/organization.tex @@ -0,0 +1,6 @@ + +(TODO write out:) + +This document describes our security analysis of the (?) CMS according to the OWASP ASVS (v3.0.1), as well as an analysis of Fortify's results compared to our findings \& the OWASP ASVS categories. + +Outline: first our analysis, then a summary of Fortify's analysis and how it compares to ours etc., then some reflection. diff --git a/report/preamble.tex b/report/preamble.tex index 692cea3..0de5d4e 100644 --- a/report/preamble.tex +++ b/report/preamble.tex @@ -8,9 +8,16 @@ \usepackage{pifont} \usepackage[dvipsnames]{xcolor} \usepackage{mdframed} +\usepackage{titlesec} +\usepackage{amsmath} +\usepackage{booktabs}% good looking tables \hypersetup{hidelinks, pdftitle={OWASP ASVS Souce Code Review Project}} +\newcommand{\sectionbreak}{\clearpage} +\titleformat{\section}[block]{\Large\bfseries}{\thesection}{1em}{} +\titleformat{\subsection}[block]{\large\bfseries}{\thesubsection}{1em}{} + % Als een criterium niet applicable is (we doen alleen 1 en 2) \newcommand{\notapplicable}[1]{{\color{Gray} #1}} @@ -48,8 +55,6 @@ % span code fragments \newcommand{\code}[1]{\texttt{#1}} -\renewcommand\thesubsection{V\arabic{subsection}} - \author{% Kelley van Evert\\ Charlie Gerhardus\\ diff --git a/report/reflection.tex b/report/reflection.tex index e69de29..30404ce 100644 --- a/report/reflection.tex +++ b/report/reflection.tex @@ -0,0 +1 @@ +TODO \ No newline at end of file diff --git a/report/report.tex b/report/report.tex index d294062..c832984 100644 --- a/report/report.tex +++ b/report/report.tex @@ -1,51 +1,67 @@ %&report \begin{document} \maketitleru[course={Software Security}] + + +\tableofcontents + + \section{Organization} \input{organization.tex} -\section{Verdict} +\section{Our security analysis of the CMS} + + \subsection{General comments} + + \begin{itemize} + \item + The CMS comes equipped with an installer script, which is incredibly insecure, but which the author of the CMS assumes the admin will remove after installation. If we regard this installer script as part of our subject matter, the analysis becomes incredibly simple, as it directly makes the CMS vulnerable to server side code execution, SQL injections, etc. Therefore, we regard the installer script as a special case, and we assume that it is indeed removed after installation. The analysis, then, treats the CMS as it is after installation. + + \item + A number of times in our analysis, the OWASP ASVS requirements will require certain overridable server settings or request/response headers to be correcty set. The CMS is in principle able to set these parameters, for example by adding directives to an Apache \code{.htaccess} file, or explicitly setting certain HTTP response headers. However, the CMS does not do so, and therefore relies upon the server's default settings. In these cases, we consider the requirements failed, although one might contest this on the principle of separation of concerns. + \end{itemize} + + + \renewcommand\thesubsection{V\arabic{subsection}} -\subsection*{General comments} + \subsection{Authentication} + \input{v2_authentication} -\begin{itemize} - \item \TODO{} (Kelley) Server instellingen. - \item \TODO{} (Kelley) \code{installer.php} and server code execution. -\end{itemize} + \subsection{Session Management} + \input{v3_session.tex} -\addtocounter{subsection}{1} -\subsection{Authentication} -\input{v2_authentication} + \subsection{Access Control} + \input{v4_access.tex} -\subsection{Session Management} -\input{v3_session.tex} + \subsection{Input Validation \& Output Encoding/Escaping} + \input{v5_input.tex} -\subsection{Access Control} -\input{v4_access.tex} + \addtocounter{subsection}{1} -\subsection{Input Validation \& Output Encoding/Escaping} -\input{v5_input.tex} + \subsection{Cryptography at rest} + \input{v7_cryptography.tex} -\addtocounter{subsection}{1} + \subsection{Error Handling \& logging} + \input{v8_error.tex} -\subsection{Cryptography at rest} -\input{v7_cryptography.tex} + \subsection{Data Protection} + \input{v9_data.tex} -\subsection{Error Handling \& logging} -\input{v8_error.tex} + \addtocounter{subsection}{2} + \subsection{HTTP Security} + \input{v11_httpsec.tex} -\subsection{Data Protection} -\input{v9_data.tex} + \addtocounter{subsection}{4} + \subsection{Files and Recourses} + TODO -\addtocounter{subsection}{2} -\subsection{HTTP Security} -\input{v11_httpsec.tex} + \renewcommand\thesubsection{\arabic{section}.\arabic{subsection}} -\addtocounter{subsection}{4} -\subsection{Files and Recourses} +\section{Summary and short reflection on Fortify's analysis} +\input{fortify.tex} -\section{Reflection} +\section{General reflection} \input{reflection.tex} \end{document} diff --git a/report/v11_httpsec.tex b/report/v11_httpsec.tex index 2b86bac..1840059 100644 --- a/report/v11_httpsec.tex +++ b/report/v11_httpsec.tex @@ -1,5 +1,5 @@ -\begin{enumerate}[label={11.\arabic*}] +\begin{enumerate}[label={V11.\arabic*}] \item\fail{} Verify that the application accepts only a defined diff --git a/report/v2_authentication.tex b/report/v2_authentication.tex index d58f562..b0b6889 100644 --- a/report/v2_authentication.tex +++ b/report/v2_authentication.tex @@ -1,5 +1,5 @@ -\begin{enumerate}[label={2.\arabic*}] +\begin{enumerate}[label={V2.\arabic*}] \item \fail{} diff --git a/report/v3_session.tex b/report/v3_session.tex index b2fa838..94cf919 100644 --- a/report/v3_session.tex +++ b/report/v3_session.tex @@ -1,4 +1,4 @@ -\begin{enumerate}[label={3.\arabic*}] +\begin{enumerate}[label={V3.\arabic*}] \item \pass{} diff --git a/report/v4_access.tex b/report/v4_access.tex index 506eea7..a26d279 100644 --- a/report/v4_access.tex +++ b/report/v4_access.tex @@ -22,7 +22,7 @@ Our check reveales that the access control mechanisms are basically only a stub, These are the results of our check: -\begin{enumerate}[label={4.\arabic*}] +\begin{enumerate}[label={V4.\arabic*}] % Access controls: % - principle of least privilege? diff --git a/report/v5_input.tex b/report/v5_input.tex index 7a01436..27ee206 100644 --- a/report/v5_input.tex +++ b/report/v5_input.tex @@ -1,4 +1,4 @@ -\begin{enumerate}[label={5.\arabic*}] +\begin{enumerate}[label={V5.\arabic*}] \item\pass{} Verify that the runtime environment is not susceptible to buffer overflows, or that security controls prevent buffer overflows. diff --git a/report/v7_cryptography.tex b/report/v7_cryptography.tex index 9058968..952d832 100644 --- a/report/v7_cryptography.tex +++ b/report/v7_cryptography.tex @@ -1,5 +1,5 @@ % usage of crypt() -\begin{enumerate}[label={7.\arabic*}] +\begin{enumerate}[label={V7.\arabic*}] \addtocounter{enumi}{1} \item diff --git a/report/v8_error.tex b/report/v8_error.tex index fc47ee0..e6b2776 100644 --- a/report/v8_error.tex +++ b/report/v8_error.tex @@ -1,4 +1,4 @@ -\begin{enumerate}[label={8.\arabic*}] +\begin{enumerate}[label={V8.\arabic*}] \item\fail{} Verify that the application does not output error messages or stack traces containing sensitive data that could assist an attacker, diff --git a/report/v9_data.tex b/report/v9_data.tex index ac485c7..0e49d1d 100644 --- a/report/v9_data.tex +++ b/report/v9_data.tex @@ -1,4 +1,4 @@ -\begin{enumerate}[label={9.\arabic*}] +\begin{enumerate}[label={V9.\arabic*}] \item\fail{} Verify that all forms containing sensitive information have disabled client side caching, including autocomplete features.