gnutls_init: Always initialize *session

We provide gnutls_session_t as an opaque type, therefore, unless we
document otherwise, client code should not assume that there is a safe
initialization value to assign to such storage, leaving the only way
to properly initialize the type as a call to gnutls_init().  Likewise,
the documentation was clear that gnutls_deinit(session) must be used
after success, but ambiguous as to whether that was necessary after
failure.

Our implementation has always been such that the opaque types are
pointers under the hood, where gnutls_deinit(NULL) is a no-op, and
that (for gnutls_init at least) it is safe to omit a call to
gnutls_deinit(session) on failure.  But without documentation, clients
cannot rely on either of those facts; and our code base was
inconsistent on whether all other *_init/*_deinit function pairs
behave in the same manner (see the next commit).

A search of existing code in the wild shows that some clients
pre-initialize the memory to 0 (which happens to be safe although
currently undocumented), often by passing in a pointer to a
gnutls_session_t residing in a larger struct that was reserved with
calloc(), cleared with memset(), or similar; but this is not
universal, and there are other clients in the wild that pass in
uninitialized memory.  It's too late to change the documentation to
mandate that users should pre-initialize their memory to 0 prior to
gnutls_init(), although it doesn't hurt to recommend it for
portability when building for older versions of gnutls.

In most cases, using gnutls_deinit(session) after failure was a no-op
- most of our error exit paths use the gnutls_free() macro which has
the side effect of forcing the caller's pointer to NULL on failure
(since gnutls is built with GNUTLS_INTERNAL_BUILD defined).  We also
happen to be lucky for a user that pre-initializes their memory to 0
before calling gnutls_init() - any error exit path where we did not
touch the user's pointer leaves the client with gnutls_deinit(session)
being a no-op.  But if the client passes in an uninitialized pointer,
and FAIL_IF_LIB_ERROR triggers, then we fail the function while
leaving the pointer uninitialized, at which point the caller using
gnutls_deinit(session) attempts to free uninitialized memory, which
has potential security implications - yet we did not warn the client
to avoid gnutls_deinit() in that scenario.

The most robust fix is thus along two fronts: improving the
documentation to inform the user what they can expect, but also
tweaking our code to avoid undefined behavior with existing client
code bases by guaranteeing that whether or not the client
pre-initializes memory to 0 and/or calls gnutls_deinit() on failure,
they can't mess up.

Fixes: bug #1414.
Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/lib/state.c b/lib/state.c
index 347c77b537c698b03f8a946752b669d7fb9d9063..aee6901343e35c4fdfc53df34f91e5670d219454 100644 (file)
--- a/lib/state.c
+++ b/lib/state.c
@@ -552,9 +552,9 @@ void _gnutls_handshake_internal_state_clear(gnutls_session_t session)
  * @session: is a pointer to a #gnutls_session_t type.
  * @flags: indicate if this session is to be used for server or client.
  *
- * This function initializes the provided session. Every
- * session must be initialized before use, and must be deinitialized
- * after used by calling gnutls_deinit().
+ * This function initializes the provided session. Every session must
+ * be initialized before use, and after successful initialization and
+ * use must be deinitialized by calling gnutls_deinit().
  *
  * @flags can be any combination of flags from %gnutls_init_flags_t.
  *
@@ -563,12 +563,22 @@ void _gnutls_handshake_internal_state_clear(gnutls_session_t session)
  * request in client side by default. To prevent that use the %GNUTLS_NO_EXTENSIONS
  * flag.
  *
+ * Note that it is never mandatory to use gnutls_deinit() after this
+ * function fails.  Since gnutls 3.8.0, it is safe to unconditionally
+ * use gnutls_deinit() even after failure regardless of whether the
+ * memory was initialized prior to gnutls_init(); however, clients
+ * wanting to be portable to older versions of the library should
+ * either skip deinitialization on failure, or pre-initialize the
+ * memory passed in to gnutls_init() to all zeroes via memset() or
+ * similar.
+ *
  * Returns: %GNUTLS_E_SUCCESS on success, or an error code.
  **/
 int gnutls_init(gnutls_session_t * session, unsigned int flags)
 {
        int ret;
 
+       *session = NULL;
        FAIL_IF_LIB_ERROR;
 
        *session = gnutls_calloc(1, sizeof(struct gnutls_session_int));