[gnutls-devel] GnuTLS | bug in gnutls_init() (#1414)

[Read-only notification of GnuTLS library development activities]

GitLab Support Bot commented:


> New response for issue #1414:
>
> Author: Zoltán Fridrich
>
> I believe that if a function outputs its result via an argument, the argument should be left untouched in case of an error. Currently this is clearly not the case with `gnutls_init` which sets session to `NULL` in some cases (but not all) when error occurs. However, I think that handling an error as follows is generally an incorrect approach.

But after auditing the code base, over 80% of the gnutls*_init()
functions DO set the parameter to NULL on most or all of the error
cases; only a very limited few were extremely careful to never touch
the parameter on failure (gnutls_x509_crt_init being a prime example).

> ```
> ret = gnutls_init(session, flags);
> if (ret < 0)
>     gnutls_deinit(session);
>     return ret;
> ```
> User should not expect to clean up functions output if that function failed. If this results in segfault, than its the users fault (unless the scenario was documented and the function is bugged).

It is undocumented what the users should do.  Some avoid calling the
_deinit on errors, but others do not.  Unless you document it, the
segfault from freeing uninit memory is not the user's fault, but a bug
in gnutls' documentation.  But even if you document that the memory
remains uninit on failure, you have NOT actually uniformly implemented
that, and there is EXISTING code in the wild that DOES call the
_deinit function, and therefore WILL crash.

>
> I would suggest either that
> - `gnutls_init` will not modify the output argument on error.

Doable, but a LOT more churn (as I said, I already did the audit, and
more than 80% of the *_init functions would need changing).

> or
> - `gnutls_init` will always set the output argument to some defined value on error. (in this case `NULL`)

That was my preference as well; see
https://gitlab.com/gnutls/gnutls/-/merge_requests/1652

> In either case, the documentation of `gnutls_init` should be updated to clearly state what happens with session when error occurs.

My preference as well, also in the merge commit (well, for
gnutls_init(); but if desired, I could expend the documentation
efforts to repeat that information for all *_init functions).

>
> I would prefer to not touch the argument on error but both solutions are ok imo.

I hope my merge commit can persuade you to ALWAYS touch the argument
on all exit paths, while at the same time documenting that the usesr
need not call the _deinit function on failure, and furthermore that
the user MAY (but not MUST) pre-zero the memory before calling _init,
such that even if the call _deinit on failure, it is safe in (almost)
all cases.  The lone exception is gnutls_pkcs11_privkey_init(), where
the use of the wrong free() function causes certain failures to cause
a double-free if the user calls _deinit on certain failures.

Blindly claiming that it is the user's fault for segfaulting on deinit
after failure when gnutls documentation did not make a strong
guarantee on what the user should do is not a very nice attitude for
security-sensitive software to be portraying.

<details><summary>...</summary>

On Wed, Oct 19, 2022 at 03:12:18PM +0000, Zoltán Fridrich (@ZoltanFridrich) wrote:

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

</details>

-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/-/issues/1414#note_1143168550
You're receiving this email because of your account on gitlab.com.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnutls-devel/attachments/20221020/f408f872/attachment-0001.html>