path: root/doc/audit/response-202005.tex

\documentclass[11pt]{article}
\oddsidemargin=0in \evensidemargin=0in
\textwidth=6.2in \textheight=8.7in
%\topmargin=-0.2in

\usepackage[ansinew]{inputenc}
\usepackage{makeidx,amsmath,amssymb,exscale,multicol,epsfig,graphics}

\begin{document}
\pagestyle{headings}
\title{Preliminary response to the \\ GNU Taler security audit in Q2/Q3 2020}
\author{Christian Grothoff \and Florian Dold}

\maketitle

\section{Abstract}

This is the response to the source code audit report CodeBlau
created for GNU Taler in Q2/Q3 2020.

\section{Management Summary}

We thank CodeBlau for their detailed report and thorough analysis. We are
particularly impressed that they reported issues against components that were
not even in-scope, and also that they found an {\em interesting} new corner
case we had not previously considered. Finally, we also find several of their
architectural recommendations to strengthen security to be worthwhile, and
while some were already on our long-term roadmap, we will reprioritize our
roadmap given their recommendations.

Given our extensive discussions with CodeBlau, we also have the impression
that they really took the time to understand the system, and look forward
to working with CodeBlau as a competent auditor for GNU Taler in the future.

\section{Issues in the exchange}

We agree with the issues CodeBlau discovered and both parties believe that
they have all been addressed.

\section{Issues in the auditor}

We appreciate CodeBlau's extensive list of checks the Taler auditor performs,
which was previously not documented adequately by us. We agree that the
auditor still needs more comprehensive documentation.

As for issue \#6416, we agree with the analysis. However, the proposed fix
of making the primary key include the denomination would create other problems,
such as the exchange sometimes not having the denomination key (link, refund)
and the code in various places relying on the assumption of the coin's
public key being unique. Furthermore, allowing coin key re-use may validate
a terrible practice. We thus decided it is better to ``fail early'', and
modified the code to check that the coin public key is ``unique'' during
deposit, refresh and recoup and ensured that the exchange returns a proof
of non-uniqueness in case of a violation. The test suite was extended to
cover the corner case.

\section{Issues in GNUnet}

We agree with the issues CodeBlau discovered and both parties believe that
they have all been addressed.

\section{General remarks on the code}

We understand that writing the code in another programming language may make
certain checks for the auditor less work to implement. However, our choice of C
is based on the advantages that make it superior to contemporary languages for
our use case:  relatively low complexity of the language (compared to C++);
availability of mature compilers, static and dynamic analysis tools;
predictable performance; access to stable and battle-tested libraries; and
future-proofness due to portability to older systems as well as new platforms.

We believe creating a parallel implementation in other languages would provide
advantages, especially with respect to avoiding ``the implementation is the
specification''-style issues.  However, given limited resources will not make
this a priority.

We disagree that all modern software development has embraced the idea that
memory errors are to be handled in ways other than terminating or restarting
the process. Many programming languages (Erlang, Java) hardly offer any other
means of handling out-of-memory situations than to terminate the process. We
also insist that Taler {\em does} handle out-of-memory as it does have code
that terminates the process (we do {\em not} simply ignore the return value
from {\tt malloc()} or other allocation functions!). We simply consider that
terminating the process (which is run by a hypervisor that will restart the
service) is the correct way to handle out-of-memory situations.  We also have
limits in place that should prevent attackers from causing large amounts of
memory to be consumed, and also have code to automatically preemptively
restart the process to guard against memory exhaustion from memory
fragmentation.  Finally, a common problem with abrupt termination may be
corrupted files.  However, the code mostly only reads from files and limits
writing to the Postgres database. Hence, there is no possibility of corrupt
files being left behind even in the case of abnormal termination.


\section{More specs and documentation code}

We agree with the recommendation that the documentation should be improved,
and will try to improve it along the lines recommended by CodeBlau.

\section{Protocol change: API for uniformly distributed seeds}

We agree with the suggestion, have made the necessary changes, and both
parties believe that the suggestion has been implemented.

\section{Reduce code complexity}

\subsection{Reduce global variables}

While we do not disagree with the general goal to have few global variables,
we also believe that there are cases where global variables make sense.

We have already tried to minimize the scope of variables. The remaining few
global variables are largely ``read-only'' configuration data. The report does
not point out specific instances that would be particularly beneficial to
eliminate. As we continue to work on the code, we will of course evaluate
whether the removal of a particular global variable would make the code
cleaner.

Also, we want to point out that all global variables we introduce
in the exchange are indicated with a prefix {\tt TEH\_} in the code, so they
are easy to identify as such.

\subsection{Callbacks, type pruning}

We understand that higher order functions in C can be confusing, but this
is also a common pattern to enable code re-use and asynchronous execution
which is essential for network applications. We do not believe that we
use callbacks {\em excessively}.  Rewriting the code in another language
may indeed make this part easier to understand, alas would have other
disadvantages as pointed out previously.

\subsection{Initializing structs with memset}

Using {\tt memset()} first prevents compiler (or valgrind) warnings about
using uninitialized memory, possibly hiding bugs. We also do use struct
initialization in many cases.

The GNUnet-wrappers are generally designed to be ``safer'' or ``stricter''
variants of the corresponding libc functions, and not merely ``the same''.
Hence we do not believe that renaming {\tt GNUNET\_malloc} is indicated.

The argument that {\tt memset()}ing first makes the code inherently more
obvious also seems fallacious, as it would commonly result in dead stores,
which can confuse developers and produce false-positive warnings from static
analysis tools.

\subsection{NULL pointer handling}

The problem with the ``goto fail'' style error handling is that it rarely
results in specific error handling where diagnostics are created that are
specific to the error. Using this style of programming encourages developers
to create simplistic error handling, which can result in inappropriate error
handling logic and also makes it harder to attribute errors to the specific
cause.

However, we have no prohibition on using this style of error handling either:
if it is appropriate, develpers should make a case-by-case decision as to how
to best handle a specific error.

We have made some first changes to how {\tt GNUNET\_free()} works in response
to the report, and will discuss further changes with the GNUnet development
team.

\subsection{Hidden security assumptions}

We disagree that the assumptions stated are ``hidden'', as (1) the Taler code
has its own checks to warrant that the requirements of the {\tt
  GNUNET\_malloc()} API are satisfied (so enforcement is not limited to the
abstraction layer), and (2) the maximum allocation size limit is quite clearly
specified in the GNUnet documentation.  Also, the GNUnet-functions are not
merely an abstraction layer for portability, but they provided extended
semantics that we rely upon. So it is not like it is possible to swap this
layer and expect anything to continue to work.

When we use the libjansson library, it is understood that it does not use
the GNUnet operations, and the code is careful about this distinction.

\subsection{Get rid of boolean function arguments}

We agree that this can make the code more readable, and have in some places
already changed the code in this way.

\section{Structural Recommendation}

\subsection{Least privilege}

It is wrong to say that GNU Taler has ``no work done'' on privilege separation.
For example, the {\tt taler-exchange-dbinit} tool is the only tool that requires
CREATE, ALTER and DROP rights on database tables, thus enusring that the ``main''
process does not need these rights.

We also already had the {\tt taler-exchange-keyup} tool responsible for
initializing keys. In response to the audit, we already changed the GNUnet API
to make sure that tools do not create keys as a side-effect of trying to read
non-existent key files.

We agree with the recommendation on further privilege separation for access
to cryptographic keys, and intend to implement this in the near future.

\subsection{File system access}

The auditor helpers actually only read from the file system, only the LaTeX
invocation to compile the final report to PDF inherently needs write
access. We do not predict that we will retool LaTeX.  Also, the file system
access is completely uncritical, as the auditor by design runs on a system
that is separate from the production exchange system.

Because that system will not have {\em any} crypto keys (not even the one of
the auditor!), CodeBlau is wrong to assume that reading from or writing to the
file system represents a security threat.

We have started to better document the operational requirements on running the
auditor.

\subsection{Avoid dlopen}

Taler actually uses {\tt ltdlopen()} from GNU libtool, which provides
compiler flags to convert the dynamic linkage into static linkage.  For
development, dynamic linkage has many advantages.

We plan to test and document how to build GNU Taler with only static
linkage, and will recommend this style of deployment for the Taler
exchange for production.

\subsection{Reduce reliance on PostgreSQL}

CodeBlau's suggestion to use an append-only transaction logging service in
addition to the PostgreSQL database is a reasonable suggestion for a
production-grade deployment of GNU Taler, as it would allow partial disaster
recovery even in the presence of an attacker that has gained write access to
the exchange's database.

We are currently still investigating whether the transaction logging should be
implemented directly by the exchange service, or via the database's extensible
replication mechanism.  Any implementation of such an append-only logging
mechanism must be carefully designed to ensure it does not negatively impact
the exchange's availability and does not interfere with serializability of
database transactions.  As such we believe that transaction logging can only be
provided on a best-effort basis.  Fortunately, even a best-effort append-only
transaction log would serve to limit the financial damage incurred by the
exchange in an active database compromise scenario.

\end{document}