gost-yescrypt: Fix alignment problem for GOST 34.11 (Streebog) #86

vt-alt · 2019-05-03T16:14:31Z

Some architectures in some circumstances do not allow unaligned
memory access (such as ARM, MIPS, SPARC) triggering SIGBUS. This
patch very crudely fixes this issue.

The issue is found and original fix is proposed by Eric Biggers:

https://patchwork.kernel.org/patch/10878865/

Being unfixed this would trigger SIGBUS when password buffer is
unaligned. Crash and fix are tested on UltraSparc T5 on GCC Compile
farm.

Some architectures in some circumstances do not allow unaligned memory access (such as ARM, MIPS, SPARC) triggering SIGBUS. This patch very crudely fixes this issue. The issue is found and original fix is proposed by Eric Biggers: https://patchwork.kernel.org/patch/10878865/ Being unfixed this would trigger SIGBUS when password buffer is unaligned. Crash and fix are tested on UltraSparc T5 on GCC Compile farm.

zackw · 2019-05-03T18:53:11Z

Does the existing test suite detect this problem when run on hardware that requires alignment of this buffer? If not, please add a test case.

vt-alt · 2019-05-03T20:12:51Z

@zackw Do you want a general test in crypt-kat.c or just for crypt-gost-yescrypt.c? I was quick tested with this:

vt@gcc202:~/src/libxcrypt$ git diff
diff --git test/crypt-kat.c test/crypt-kat.c
index 0cd8c2b..253b0a8 100644
--- test/crypt-kat.c
+++ test/crypt-kat.c
@@ -207,13 +207,17 @@ calc_hashes_crypt_r_rn (ARG_UNUSED (void *unused))
   memset (&data, 0, sizeof data);
   for (i = 0; i < ntests; i++)
     {
+       char pass[CRYPT_MAX_PASSPHRASE_SIZE];
+
+       strcpy(pass + 1, tests[i].input);
+       printf("[%d]: %s %s\n", strlen(tests[i].input), tests[i].input, tests[i].salt);
       errno = 0;
-      hash = crypt_r (tests[i].input, tests[i].salt, &data);
+      hash = crypt_r (pass + 1, tests[i].salt, &data);
       status |= report_result ("crypt_r", hash, errno, &tests[i],
                                ENABLE_FAILURE_TOKENS);

       errno = 0;
-      hash = crypt_rn (tests[i].input, tests[i].salt, &data, (int)sizeof data);
+      hash = crypt_rn (pass + 1, tests[i].salt, &data, (int)sizeof data);
       status |= report_result ("crypt_rn", hash, errno, &tests[i], false);
     }

Only gost-yescrypt triggered an error.

Currently, calc_hashes_crypt_r_rn does not output anything, silent test, so when it crashes it's unknown why - that's why there is printf. Also, you don't have an error message, just a crash with Bus error.

vt-alt · 2019-05-03T20:20:03Z

Does the existing test suite detect this problem when run on hardware that requires alignment of this buffer?

Forgot to say 'no'.

besser82 · 2019-05-04T17:27:25Z

@zackw @vt-alt LGTM.

@zackw Do you want a general test in crypt-kat.c or just for crypt-gost-yescrypt.c? I was quick tested with this:

vt@gcc202:~/src/libxcrypt$ git diff
diff --git test/crypt-kat.c test/crypt-kat.c
index 0cd8c2b..253b0a8 100644
--- test/crypt-kat.c
+++ test/crypt-kat.c
@@ -207,13 +207,17 @@ calc_hashes_crypt_r_rn (ARG_UNUSED (void *unused))
   memset (&data, 0, sizeof data);
   for (i = 0; i < ntests; i++)
     {
+       char pass[CRYPT_MAX_PASSPHRASE_SIZE];
+
+       strcpy(pass + 1, tests[i].input);
+       printf("[%d]: %s %s\n", strlen(tests[i].input), tests[i].input, tests[i].salt);
       errno = 0;
-      hash = crypt_r (tests[i].input, tests[i].salt, &data);
+      hash = crypt_r (pass + 1, tests[i].salt, &data);
       status |= report_result ("crypt_r", hash, errno, &tests[i],
                                ENABLE_FAILURE_TOKENS);

       errno = 0;
-      hash = crypt_rn (tests[i].input, tests[i].salt, &data, (int)sizeof data);
+      hash = crypt_rn (pass + 1, tests[i].salt, &data, (int)sizeof data);
       status |= report_result ("crypt_rn", hash, errno, &tests[i], false);
     }

Only gost-yescrypt triggered an error.

Currently, calc_hashes_crypt_r_rn does not output anything, silent test, so when it crashes it's unknown why - that's why there is printf. Also, you don't have an error message, just a crash with Bus error.

The test-case seems fine to me this way as well. Maybe it can be optimized a bit, so that the buffer won't be allocated in every iteration of the loop. Also we may want to use strncpy(pass + 1, tests[i].input, CRYPT_MAX_PASSPHRASE_SIZE - 1);, so the buffer is guaranteed to be filled with trailing zeros.

Anyways, I'm going to modify the test-case myself after merging fix.

vt-alt · 2019-05-04T17:29:17Z

@besser82

Anyways, I'm going to modify the test-case myself after merging fix.
Ah, I will at least test it.

zackw · 2019-05-04T17:31:13Z

I do want each hash algorithm to be tested at least once for this bug. I doubt we get much more value out of testing non-aligned inputs for every subcase in test-crypt-kat.c, but if that's the easiest way to accomplish the test then I don't see any harm in it.

Having a test that we know will crash on architectures with these constraints, when an alignment bug is present, is adequate, I think.

solardiz · 2019-05-04T17:41:51Z

lib/alg-gost3411-2012-core.c

@@ -149,10 +149,13 @@ g(uint512_u *h, const uint512_u *N, const unsigned char *m)
 static inline void
 stage2(GOST34112012Context *CTX, const unsigned char *data)
 {
-    g(&(CTX->h), &(CTX->N), data);
+    union uint512_u m;


Looks like the word union here is unneeded (implied as part of the typedef), right? If so, I'm mildly surprised compilers are happy to ignore it.

Edit: looked it up, this clears up why the compilers are happy anyway:

typedef union uint512_u { unsigned long long QWORD[8]; } uint512_u;

So the same identifier is both the name for the union and for the typedef. Yet it's probably cleaner (and shorter) to only use it as the latter.

Fixed in 5cce1eb.

solardiz · 2019-05-04T17:47:53Z

BTW, pass + 1 doesn't guarantee misalignment, it just makes misalignment very likely. We could want to test e.g. with both pass and pass + 1, so that one of those is certainly misaligned, or test with the right one of them after explicitly testing for misalignment (cast to uintptr_t and test the least significant bit).

solardiz · 2019-05-04T17:54:04Z

Something like:

mispass = pass;
if (!((uintptr_t)mispass & 1))
    mispass++;

or:

mispass = pass + 1;
if (((uintptr_t)pass & 1))
    mispass = pass;

then use mispass in place of the uses of pass + 1 proposed above.

vt-alt · 2019-05-04T17:54:46Z

BTW, pass + 1 doesn't guarantee misalignment

Oh ya since pass is char buffer it does not need to be aligned. Maybe we could just union it to int?

solardiz · 2019-05-04T17:57:59Z

Maybe we could just union it to int?

That's also a valid fix, yes. Edit: and I like it better than those I suggested, so please go ahead with yours.

codecov · 2019-05-04T18:12:41Z

Codecov Report

Merging #86 into develop will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           develop     #86   +/-   ##
=======================================
  Coverage     96.4%   96.4%           
=======================================
  Files           32      32           
  Lines         3112    3112           
=======================================
  Hits          3000    3000           
  Misses         112     112

Impacted Files	Coverage Δ
lib/alg-gost3411-2012-core.c	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60ceb50...03dccc3. Read the comment docs.

besser82 · 2019-05-04T18:17:43Z

BTW, pass + 1 doesn't guarantee misalignment

Oh ya since pass is char buffer it does not need to be aligned. Maybe we could just union it to int?

That does not work as it will trigger -Wstringop-overflow= when using strcpy…

vt-alt · 2019-05-04T18:21:03Z

@besser82 I pushed in #87 my version of it.

vt-alt · 2019-05-04T18:40:55Z

I tested on UltraSparc T5 again.

Current develop branch produces no errors.
Then I reverted commits 5cce1eb and 03dccc3 with Streebog fixes and got

vt@gcc202:~/src/libxcrypt$ test/crypt-kat
...
[80]: 12345678901234567890123456789012345678901234567890123456789012345678901234567890 $gy$j75$.......
Bus error
vt@gcc202:~/src/libxcrypt$ make check
...
PASS: test/crypt-gost-yescrypt
./m4/test-driver: line 107: 195553 Bus error               "$@" > $log_file 2>&1
FAIL: test/crypt-kat
...

Thus we know that test/crypt-kat is correct.

besser82 merged commit 03dccc3 into besser82:develop May 4, 2019

solardiz reviewed May 4, 2019

View reviewed changes

vt-alt deleted the fix_streebog branch May 4, 2019 18:14

vt-alt mentioned this pull request May 4, 2019

crypt-kat: make unalignment test really unaligned #87

Closed

vt-alt mentioned this pull request Oct 17, 2019

travis-ci: enable coverity scan gost-engine/engine#183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gost-yescrypt: Fix alignment problem for GOST 34.11 (Streebog) #86

gost-yescrypt: Fix alignment problem for GOST 34.11 (Streebog) #86

vt-alt commented May 3, 2019

zackw commented May 3, 2019

vt-alt commented May 3, 2019

vt-alt commented May 3, 2019

besser82 commented May 4, 2019

vt-alt commented May 4, 2019

zackw commented May 4, 2019 •

edited

solardiz May 4, 2019 •

edited

besser82 May 4, 2019

solardiz commented May 4, 2019

solardiz commented May 4, 2019

vt-alt commented May 4, 2019

solardiz commented May 4, 2019 •

edited

codecov bot commented May 4, 2019 •

edited

besser82 commented May 4, 2019

vt-alt commented May 4, 2019

vt-alt commented May 4, 2019

gost-yescrypt: Fix alignment problem for GOST 34.11 (Streebog) #86

gost-yescrypt: Fix alignment problem for GOST 34.11 (Streebog) #86

Conversation

vt-alt commented May 3, 2019

zackw commented May 3, 2019

vt-alt commented May 3, 2019

vt-alt commented May 3, 2019

besser82 commented May 4, 2019

vt-alt commented May 4, 2019

zackw commented May 4, 2019 • edited

solardiz May 4, 2019 • edited

Choose a reason for hiding this comment

besser82 May 4, 2019

Choose a reason for hiding this comment

solardiz commented May 4, 2019

solardiz commented May 4, 2019

vt-alt commented May 4, 2019

solardiz commented May 4, 2019 • edited

codecov bot commented May 4, 2019 • edited

Codecov Report

besser82 commented May 4, 2019

vt-alt commented May 4, 2019

vt-alt commented May 4, 2019

zackw commented May 4, 2019 •

edited

solardiz May 4, 2019 •

edited

solardiz commented May 4, 2019 •

edited

codecov bot commented May 4, 2019 •

edited