[Pkg-privacy-maintainers] Bug#932927: libotr: buggy unit test: test_auth.c: test_auth_clear()

[Aurelien Jarno]

Source: libotr
Version: 4.1.1-3
Severity: normal
Tags: patch upstream
User: debian-riscv at lists.debian.org
Usertags: riscv64

I have been investigating while test_auth fails with a segmentation
fault on riscv64:

| aurel32 at riscv64:~/libotr/libotr-4.1.1/tests/unit$ ./test_auth
| 1..5
| ok 1 - OTR auth info init is valid
| Segmentation fault

Here is the corresponding backtrace:

| #0  _gcry_mpi_free_limb_space (a=0x100f30003, nlimbs=1179403647) at ../../mpi/mpiutil.c:142
| #1  0x00000020000c531c in _gcry_mpi_free (a=0x2000000000) at ../../mpi/mpiutil.c:224
| #2  0x00000020000471e8 in otrl_dh_keypair_free (kp=kp at entry=0x3fffa95170) at dh.c:89
| #3  0x000000200004adc4 in otrl_auth_clear (auth=0x3fffa95160) at auth.c:107
| #4  0x0000002aaaaab0f6 in test_auth_clear () at test_auth.c:70
| #5  0x0000002aaaaaaed2 in main (argc=<optimized out>, argv=<optimized out>) at test_auth.c:176

It happens that test_auth_clear() uses for its test a struct context
allocated on the stack. As such its contents is random. 
otrl_auth_clear() is called on this random data, which in turns call
otrl_dh_keypair_free() on this random data. The DH_keypair structure
contains 2 gcry_mpi_t fields, which are pointers. otrl_dh_keypair_free()
calls gcry_mpi_release() on those pointers, which ends up calling
_gcry_mpi_free_limb_space() to free the memory. Boom.

It seems this test only works by chance on other architectures as the
struct context in test_auth_new() and test_auth_clear() are at the same
location on the stack, and thus its contents is not random anymore when
entering test_auth_clear(). This seems to be confirmed by the last test
of this function: auth->context == &ctx. There is no way that
otrl_auth_clear() can set this value correctly, and it doesn't get
passed a pointer to ctx.

Therefore I believe that otrl_auth_new() should be called before
otrl_auth_clear(), like in the following patch:

--- a/tests/unit/test_auth.c
+++ b/tests/unit/test_auth.c
@@ -67,6 +67,7 @@
 	OtrlAuthInfo *auth = &ctx.auth;
 
 	/* API call. */
+	otrl_auth_new(&ctx);
 	otrl_auth_clear(auth);
 
 	ok(auth->authstate == OTRL_AUTHSTATE_NONE &&

Note that the issue can be easily reproduced on other architectures like
amd64 by making sure that ctx is random:

--- libotr-4.1.1.orig/tests/unit/test_auth.c
+++ libotr-4.1.1/tests/unit/test_auth.c
@@ -66,6 +66,11 @@ static void test_auth_clear(void)
 	struct context ctx;
 	OtrlAuthInfo *auth = &ctx.auth;
 
+	/* Initialize some fields to random values */
+	auth->our_dh.pub = (gcry_mpi_t) 0xc0ffee;
+	auth->our_dh.priv = (gcry_mpi_t) 0x0c0ffee;
+	auth->context = NULL;
+
 	/* API call. */
 	otrl_auth_clear(auth);