test_session_cipher & test_session_builder fail on OpenBSD

[blackgnezdo]

---

Bug description

 6/13 Test  #6: test_session_cipher ..............***Failed    0.54 sec
      Start  7: test_session_builder
 7/13 Test  #7: test_session_builder .............***Failed    0.54 sec
      Start  8: test_key_helper

Steps to reproduce

• Get OpenBSD 6.4 amd64 system
• pkg_add cmake check
• Check out patched tree which includes #118
• mkdir -p build && cd build&& cmake -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=1 ..&& cd tests&& make&& cd ..&& ctest

Actual result: 2/13 tests fail Expected result: All tests pass

Device info

Device: OpenBSD 6.4 amd64

Link to logs

https://gist.github.com/blackgnezdo/e4dc98fae8608512a232f5ce32c59795

[blackgnezdo]

Interesting snippets:

tests/test_session_cipher.c:233:F:case:test_basic_session_v3:0: Assertion 'signal_buffer_compare(index_plaintext, plaintext) == 0' failed: signal_buffer_compare(index_plaintext, plaintext) == 55, 0 == 0

tests/test_session_builder.c:1504:F:case:test_basic_pre_key_v3:0: Assertion 'signal_buffer_compare(alice_ooo_plaintext[i], ooo_plaintext) == 0' failed: signal_buffer_compare(alice_ooo_plaintext[i], ooo_plaintext) == 2, 0 == 0

[dkonigsberg]

Since this test passes on every other platform, do you have any idea exactly what's causing this failure on OpenBSD? If necessary, I can setup an OpenBSD VM to try and debug the issue directly, but I don't have one readily available. The specific test failure just means that the original and decrypted plaintext isn't matching, but the decrypt operation isn't returning an error. This definitely merits further investigation, regardless of which of us is doing it.

[blackgnezdo]

Just for kicks confirmed the exact same procedure yields passing tests on GNU/Linux Debian amd64.

[blackgnezdo]

The specific test failure just means that the original and decrypted plaintext isn't matching, but the decrypt operation isn't returning an error. This definitely merits further investigation, regardless of which of us is doing it.

We'll have to instrument the code either way. If you'd like a ready-made GCE image I am happy to give you one or even provide a GCE instance. I don't know an easy way to find your public ssh key on GH though.

[blackgnezdo]

One of these perhaps? https://api.github.com/users/dkonigsberg/keys

[blackgnezdo]

You can also build your own image pretty easily: https://github.com/google/syzkaller/blob/master/tools/create-openbsd-gce-ci.sh

Remember to put your own keys here.

[dkonigsberg]

No need to fill this discussion with GCE tips. I'll get an OpenBSD VM running and try and reproduce your test failures sometime tonight or tomorrow.

[dkonigsberg]

Okay, I just want to let you know that I've reproduced your test failures. (Throwing up a quick OpenBSD install doesn't really take that long.) I'll let you know more once I have the time to actually dig in and debug the issue.

[blackgnezdo]

Looks like some kind of boundary condition. Applied a simple patch to gain some visibility:

diff --git a/tests/test_session_cipher.c b/tests/test_session_cipher.c
index e761fda..afb8cec 100644
--- a/tests/test_session_cipher.c
+++ b/tests/test_session_cipher.c
@@ -3,6 +3,7 @@
 #include <check.h>
 #include <pthread.h>

+#include "../src/signal_protocol_internal.h"
 #include "../src/signal_protocol.h"
 #include "session_record.h"
 #include "session_state.h"
@@ -212,6 +213,58 @@ void generate_test_message_collections(session_cipher *cipher, signal_buffer **p
     shuffle_buffers(ciphertext_messages, size);
 }

+void hexDump (char *desc, void *addr, int len) {
+    int i;
+    unsigned char buff[17];
+    unsigned char *pc = (unsigned char*)addr;
+
+    // Output description if given.
+    if (desc != NULL)
+      fprintf (stderr, "%s:\n", desc);
+
+    if (len == 0) {
+        fprintf(stderr, "  ZERO LENGTH\n");
+        return;
+    }
+    if (len < 0) {
+        fprintf(stderr, "  NEGATIVE LENGTH: %i\n",len);
+        return;
+    }
+
+    // Process every byte in the data.
+    for (i = 0; i < len; i++) {
+        // Multiple of 16 means new line (with line offset).
+
+        if ((i % 16) == 0) {
+            // Just don't print ASCII for the zeroth line.
+            if (i != 0)
+                fprintf (stderr, "  %s\n", buff);
+
+            // Output the offset.
+            fprintf (stderr, "  %04x ", i);
+        }
+
+        // Now the hex code for the specific character.
+        fprintf (stderr, " %02x", pc[i]);
+
+        // And store a printable ASCII character for later.
+        if ((pc[i] < 0x20) || (pc[i] > 0x7e))
+            buff[i % 16] = '.';
+        else
+            buff[i % 16] = pc[i];
+        buff[(i % 16) + 1] = '\0';
+    }
+
+    // Pad out last line if not exactly 16 characters.
+    while ((i % 16) != 0) {
+        fprintf (stderr, "   ");
+        i++;
+    }
+
+    // And print the final ASCII bit.
+    fprintf (stderr, "  %s\n", buff);
+}
+
 void decrypt_and_compare_messages(session_cipher *cipher, signal_buffer *ciphertext, signal_buffer *plaintext)
 {
     int result = 0;
@@ -230,7 +283,14 @@ void decrypt_and_compare_messages(session_cipher *cipher, signal_buffer *ciphert
     ck_assert_int_eq(result, 0);

     /* Compare the messages */
-    ck_assert_int_eq(signal_buffer_compare(index_plaintext, plaintext), 0);
+    int v = signal_buffer_compare(index_plaintext, plaintext);
+    
+    fprintf(stderr, "v is %d, len is %zu, data is %p\n", v, index_plaintext->len, index_plaintext->data);
+
+    hexDump("plaintext", plaintext->data, plaintext->len);
+    hexDump("index_plaintext", index_plaintext->data, index_plaintext->len);
+    
+    ck_assert_int_eq(v, 0);

     /* Cleanup */
     SIGNAL_UNREF(index_message_deserialized);

The output is like so

mkdir -p build && cd build&&  cmake -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=1 ..&&  cd tests&&  make&&  cd ..&&  tests/test_session_cipher
-- Configuring done
-- Generating done
-- Build files have been written to: /home/greg/s/libsignal-protocol-c/build
[ 48%] Built target curve25519
[ 49%] Built target protobuf-c
[ 64%] Built target signal-protocol-c
[ 67%] Built target test_device_consistency
[ 69%] Built target test_fingerprint
[ 72%] Built target test_session_cipher
[ 75%] Built target test_key_helper
[ 77%] Built target test_session_builder
[ 80%] Built target test_sender_key_record
[ 83%] Built target test_session_record
[ 85%] Built target test_protocol
[ 88%] Built target test_ratchet
[ 91%] Built target test_simultaneous_initiate
[ 94%] Built target test_hkdf
[ 97%] Built target test_group_cipher
[100%] Built target test_curve25519
Running suite(s): session_cipher
v is 0, len is 28, data is 0xd6ec0ea7048
plaintext:
  0000  54 68 69 73 20 69 73 20 61 20 70 6c 61 69 6e 74  This is a plaint
  0010  65 78 74 20 6d 65 73 73 61 67 65 2e              ext message.
index_plaintext:
  0000  54 68 69 73 20 69 73 20 61 20 70 6c 61 69 6e 74  This is a plaint
  0010  65 78 74 20 6d 65 73 73 61 67 65 2e              ext message.
Interaction complete: Alice -> Bob
v is 0, len is 27, data is 0xd6f1e453e88
plaintext:
  0000  54 68 69 73 20 69 73 20 61 20 6d 65 73 73 61 67  This is a messag
  0010  65 20 66 72 6f 6d 20 42 6f 62 2e                 e from Bob.
index_plaintext:
  0000  54 68 69 73 20 69 73 20 61 20 6d 65 73 73 61 67  This is a messag
  0010  65 20 66 72 6f 6d 20 42 6f 62 2e                 e from Bob.
Interaction complete: Bob -> Alice
v is 32, len is 32, data is 0xd6f1e453fc8
plaintext:
  0000  d1 81 d0 bc d0 b5 d1 80 d1 82 d1 8c 20 d0 b7 d0  ............ ...
  0010  b0 20 d1 81 d0 bc d0 b5 d1 80 d1 82 d1 8c 20 31  . ............ 1
index_plaintext:
  0000  d1 81 d0 bc d0 b5 d1 80 d1 82 d1 8c 20 d0 b7 d0  ............ ...
  0010  b0 20 d1 81 d0 bc d0 b5 d1 80 d1 82 d1 8c 20 11  . ............ .
[WARNING] Received message with old counter: 2010, 0
50%: Checks: 2, Failures: 1, Errors: 0
/home/greg/s/libsignal-protocol-c/tests/test_session_cipher.c:293:F:case:test_basic_session_v3:0: Assertion 'v == 0' failed: v == 32, 0 == 0
/home/greg/s/libsignal-protocol-c/tests/test_session_cipher.c:520:P:case:test_message_key_limits:0: Passed

Observe a single bit corruption.

[blackgnezdo]

BTW, kudos on hidden messages in Russian :)

[dkonigsberg]

Okay, I think I found the problem. OpenBSD's implementation of rand(), unlike every other platform, is non-deterministic. Meanwhile, these tests depend on generating repeatable pseudo-random sequences so that the plaintext and ciphertext collections are shuffled into the same order.

https://man.openbsd.org/rand

Thankfully the fix is straightforward, and involves replacing all calls to srand() with calls to srand_deterministic(). Of course since this change is OpenBSD-specific, it'll need to be encapsulated inside a common function that wraps some #ifdefs before it can be committed.