and possibly also the TestCMS code.
For example, questions to consider are:
- - Kelley: How good (useful, clear, ...) is the ASVS? How could it be improved?
- - Charlie: How useful were code analysis tools? How could they be improved? How did you experience the rates and amounts of false and true positives? How might that be improved?
- - Wouter: What were the bottlenecks in doing the security review in your experience?
- - Mart: Maybe in the points above you can distinguish different (types of) security flaws or verification requirements. E.g., are some (categories of) verification requirements easier to check than others?
- - Daan: If you would have to do something like this again, what would you do differently? Eg. about organising things within the group: i.e., in retrospect, what do you think the best approach is to organise and divide the work in a team? (Dividing the verification requirements over the team members? Or by dividing the code? Or letting everyone look at everything, because different people will spot different things? Or work in pairs where one person confirms the findings of the other? ...)
- - Kelley: About the TestCMS code: are there important aspects that could (or should) be changed to improve security? Or aspects that could be changed to facilitate doing a security review?
- - Wouter: This last question might be generalised, to (web) applications in general, rather than this specific example of the TestCMS: ie. if you were to develop an application that will need to be subjected to a security review, would you do anything differently, given your experience in doing this, and if so, what and why?
+ - [x] Kelley: How good (useful, clear, ...) is the ASVS? How could it be improved?
+ - [/] Charlie: How useful were code analysis tools? How could they be improved? How did you experience the rates and amounts of false and true positives? How might that be improved?
+ - [x] Wouter: What were the bottlenecks in doing the security review in your experience?
+ - [ ] Mart: Maybe in the points above you can distinguish different (types of) security flaws or verification requirements. E.g., are some (categories of) verification requirements easier to check than others?
+ - [x] Daan: If you would have to do something like this again, what would you do differently? Eg. about organising things within the group: i.e., in retrospect, what do you think the best approach is to organise and divide the work in a team? (Dividing the verification requirements over the team members? Or by dividing the code? Or letting everyone look at everything, because different people will spot different things? Or work in pairs where one person confirms the findings of the other? ...)
+ - [ ] Daan: About the TestCMS code: are there important aspects that could (or should) be changed to improve security? Or aspects that could be changed to facilitate doing a security review?
+ - [/] Wouter: This last question might be generalised, to (web) applications in general, rather than this specific example of the TestCMS: ie. if you were to develop an application that will need to be subjected to a security review, would you do anything differently, given your experience in doing this, and if so, what and why?
\begin{enumerate}[label=(\Alph*)]
\item 50 cases of \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 In the \textbf{privacy 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).
\begin{table}[th!]
\centering
%\renewcommand{\arraystretch}{1}
-\begin{tabular}{@{}lllllllllll@{}}
+\begin{tabular}{@{}llllllllll@{}}
\toprule
\# &
\textbf{V2} &
\textbf{V3} &
\textbf{V4} &
-\textbf{V5} &
+\textbf{V5 (6)} &
\textbf{V7} &
\textbf{V8} &
\textbf{V9} &
-\textbf{V12} &
-\textbf{V17} \\
+\textbf{V11} \\
\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 & & & & & & & & \\
+% V2 V3 V4 V5 V7 V8 V9 V11
+ 1 & \X & \p & \p & \p & & \X & \F{B}\X & \X \\
+ 2 & \F{B}\p & \p & & & \p & \p & & \p \\
+ 3 & & \X & & \X & & \X & \p & \\
+ 4 & \p & & \p & & & \X & \X & \X \\
+ 5 & & \p & \p & \p & & \p & \p & \p \\
+ 6 & \X & \p & & & \X & \p & & \X \\
+ 7 & \p & \X & & & \p & \p & \p & \X \\
+ 8 & \p & & \p & & & & & \X \\
+ 9 & \X & \p & \X & & \p & & \p & \\
+10 & & \X & \p & \X & & \X & \p & \\
+11 & & \p & & \p & & & \p & \\
+12 & \X & \X & \X & \p & \X & & & \\
+13 & \X & \X & \F{A}\X & \p & \X & \X & & \\
+14 & & & \X & \p & \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.}
--- /dev/null
+ % Wouter: What were the bottlenecks in doing the security review in your
+ % experience?
+
+% Only working on part of the code: 'big picture' may be missing
+When we divided the work that had to be done for the security audit we
+settled on giving each each member two categories to check. Then later we all
+verified each other's findings.
+
+This approach meant that everyone got very familiar with a certain
+functionality of the web application. But it also created a bottleneck in
+the sense that it is easy to lose overview of the application as the sum of
+its parts. We feel this bottleneck is an acceptable consequence of the way we
+divided the workload.
+
+% Finding the code which handles a requirement (failing is easier than pass)
+Another bottleneck we ran into when failing a requirement was not trivial, was
+finding the actual code which handles a requirement. When assessing if a certain
+requirement passes or fails the OWASP guidelines, one has to find the code which
+implements the requirement. For many of the requirements this comes down to
+finding a counterexample. In our experience there are not often very hard to
+find for this specific web application. However for some requirements
+counterexamples will be harder to find. For these we needed to find all the code
+which touches upon this requirement and then verify it.
+
+To summarize the previous paragraph: it is almost always easier to fail a
+requirement and be sure of it, then to pass it. Because passing means that the
+requirement is OK in \emph{all} cases. While failing needs only to mean that an
+requirement fails in \emph{one} case.
+
+% Verifying each other's results
+The final bottleneck we like to note concerns the verifying of each other's
+results. When verifying the results of your group members (collegues) it may be
+difficult to fully double-check their work. This touches upon the previous
+point, for when a counterexample is giving it is trivial to see the result is
+correct. However when a member of the group passes a requirement it will take
+more time to verify this is the correct verdict. In the end we are confident of
+our results, but this may be a bottleneck which can be addressed in the initial
+organisation.
+++ /dev/null
-
-(TODO: Kelley)
-
-(TODO: Wouter)
--- /dev/null
+% Wouter: This last question might be generalised, to (web) applications in
+% general, rather than this specific example of the TestCMS: ie. if you were to
+% develop an application that will need to be subjected to a security review,
+% would you do anything differently, given your experience in doing this, and if
+% so, what and why?
+
+After our experience doing a security review of TestCMS, there are definitely
+lessons to be learned for development of easy{-}to{-}review applications.
+
+% Mark sections of code as implementing some requirement
+One could, for example, mark sections of code which treat a certain security
+requirement clearly as such. This would allow for a speed up of the reviewing
+process. The reviewers would still need to check if the marked section treat
+what they state they do, but this would still lead to a decrease in the amount
+of time spent per requirement in the OWASP guidelines.
+
+This would be for the reason that such an approach would clearly save time in
+the review, which would also increase the quality of the review and the overall
+security of the application. Of course every advantage come with certain
+disadvantages, and the disadvantage here is that such an approach would increase
+development time. This would also require more careful planning of the
+development of the application.
+
+% Centralize user input
+Another improvement which would streamline the security audit would be to
+centralize the locations in which user input is handled. By doing this the whole
+class of vulnerabilities which stems from the handling of user input could be
+neutralized to an extent. This would be a relatively easy change in the
+development process with comparatively big security benefits.
+
+% Centralize application output
+In the same category; during development the application could be designed in
+such a way that all dynamic output is sanitized before outputting it to the
+user. This would create another defense{-}in{-}depth layer for handling
+attacker controlled input.
+
+% Use working frameworks
+To achieve the last two points one could use a framework verified to be secure,
+the security reviewers could then spend less time on verifying the framework and
+more on the implementation of the specific features of this application.
--- /dev/null
+% About the TestCMS code: are there important aspects that could (or should)
+% be changed to improve security? Or aspects that could be changed to
+% facilitate doing a security review?
+
+We found that the general design of the TestCMS codebase was pretty adequate.
+Functionality was neatly grouped by in different modules in the \code{system}
+directory. The code was very readable and commented. This made the security
+audit easier to do than we had initially expected.
+
+At the same time the TestCMS did not really do this in the case of security
+critical components. Input sanitization happens all over the place (and in
+some cases it does not happen at all). Middleware based design patterns could
+make the processing of input and output a somewhat less cluttered.
+
+Another thing that striked us about the TestCMS code is that all functionality
+was written by the programmer theirself. Although it may make the application
+a bit slower, using a template engine (like Twig\footnote{\url{http://twig.sensiolabs.org/}})
+could make the application design clearer and more secure by design. While a
+template engine is not necesarry, we think that using the new \code{MySQLi} API
+and in combination with prepared statements is a good change to start with.
\subsection{On our auditing process}
\input{reflection.auditing_process.tex}
-(TODO: Wouter)
+\subsection{On the bottlenecks}
+\input{reflection.bottlenecks.tex}
\subsection{On the ASVS checklist}
\input{reflection.asvs.tex}
\subsection{?}
(TODO: Mart)
-\subsection{On the code \& streamlining subsequent security audits}
-\input{reflection.code_and_auditing.tex}
+\subsection{TestCMS code security}
+\input{reflection.testcms_code.tex}
+
+\subsection{On the general development of secure software}
+\input{reflection.secure_development.tex}
repositories / ssproject1617.git / commitdiff