martijnvanbrummelen / nwipe

nwipe secure disk eraser
GNU General Public License v2.0
631 stars 71 forks source link

Implement High-Quality Random Number Generation Using AES-CTR Mode with OpenSSL and AES-NI Support. C++ variant #570

Closed Knogle closed 3 months ago

Knogle commented 3 months ago

Ahoy. Same as https://github.com/martijnvanbrummelen/nwipe/pull/559 but with major rewrite in C++ using smart pointers to ensure memory safety.

PartialVolume commented 3 months ago

This branch is currently being tested, I'm pretty busy at the moment so it may be a few days before I give you an update.

Knogle commented 3 months ago

This branch is currently being tested, I'm pretty busy at the moment so it may be a few days before I give you an update.

An issue i've found, that should be fixed in this branch now, causing buffer overflow. I've used AES-CTR-256, which was writing into a 128-bit buffer. Now i've altered the buffer to 256-bit. Should effectively double the speed.

Screenshot from 2024-04-09 01-56-42

PartialVolume commented 3 months ago

Verification still fails, valgrind error still present but points to the new aes_ctr_prng_genrand_uint256_to_buf() function.


==1439456== Thread 4:
==1439456== Conditional jump or move depends on uninitialised value(s)
==1439456==    at 0x11EDDD: nwipe_random_pass (pass.c:330)
==1439456==    by 0x1279A7: nwipe_runmethod (method.c:934)
==1439456==    by 0x12734D: nwipe_random (method.c:742)
==1439456==    by 0x52AF133: start_thread (pthread_create.c:442)
==1439456==    by 0x532EA3F: clone (clone.S:100)
==1439456==  Uninitialised value was created by a stack allocation
==1439456==    at 0x124110: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
==1439456== 
==1439456== Syscall param write(buf) points to uninitialised byte(s)
==1439456==    at 0x531E27F: __libc_write (write.c:26)
==1439456==    by 0x531E27F: write (write.c:24)
==1439456==    by 0x11EE81: nwipe_random_pass (pass.c:349)
==1439456==    by 
```0x1279A7: nwipe_runmethod (method.c:934)
==1439456==    by 0x12734D: nwipe_random (method.c:742)
==1439456==    by 0x52AF133: start_thread (pthread_create.c:442)
==1439456==    by 0x532EA3F: clone (clone.S:100)
==1439456==  Address 0x6c57bb0 is 0 bytes inside a block of size 4,096 alloc'd
==1439456==    at 0x48455EF: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1439456==    by 0x11EBC2: nwipe_random_pass (pass.c:268)
==1439456==    by 0x1279A7: nwipe_runmethod (method.c:934)
==1439456==    by 0x12734D: nwipe_random (method.c:742)
==1439456==    by 0x52AF133: start_thread (pthread_create.c:442)
==1439456==    by 0x532EA3F: clone (clone.S:100)
==1439456==  Uninitialised value was created by a stack allocation
==1439456==    at 0x124110: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
Knogle commented 3 months ago

Now i've compiled and ran on my Ubuntu server, the result there is entirely different heh!

As we have pointed out before, Fedora uses some different linker, so it was able to build despite Ubuntu being unable to build a few weeks ago as you remember. I think it has something to do with it.

Screenshot from 2024-04-09 11-03-07

Ok now it's the first time i'm encountering those issues as well on Fedora.

Screenshot from 2024-04-09 11-12-38

==13028== Thread 4:
==13028== Conditional jump or move depends on uninitialised value(s)
==13028==    at 0x4118F1: nwipe_random_pass (pass.c:330)
==13028==    by 0x41604C: nwipe_runmethod (method.c:934)
==13028==    by 0x417179: nwipe_random (method.c:742)
==13028==    by 0x52A1896: start_thread (in /usr/lib64/libc.so.6)
==13028==    by 0x53288C3: clone (in /usr/lib64/libc.so.6)
==13028==  Uninitialised value was created by a stack allocation
==13028==    at 0x414980: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
==13028== 
==13028== Syscall param write(buf) points to uninitialised byte(s)
==13028==    at 0x531BF1D: write (in /usr/lib64/libc.so.6)
==13028==    by 0x4117D0: nwipe_random_pass (pass.c:349)
==13028==    by 0x41604C: nwipe_runmethod (method.c:934)
==13028==    by 0x417179: nwipe_random (method.c:742)
==13028==    by 0x52A1896: start_thread (in /usr/lib64/libc.so.6)
==13028==    by 0x53288C3: clone (in /usr/lib64/libc.so.6)
==13028==  Address 0x64452f0 is 0 bytes inside a block of size 4,096 alloc'd
==13028==    at 0x4849E60: calloc (vg_replace_malloc.c:1595)
==13028==    by 0x41172E: nwipe_random_pass (pass.c:268)
==13028==    by 0x41604C: nwipe_runmethod (method.c:934)
==13028==    by 0x417179: nwipe_random (method.c:742)
==13028==    by 0x52A1896: start_thread (in /usr/lib64/libc.so.6)
==13028==    by 0x53288C3: clone (in /usr/lib64/libc.so.6)
==13028==  Uninitialised value was created by a stack allocation
==13028==    at 0x414980: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
==13028== 
==13028== Thread 8:
==13028== Conditional jump or move depends on uninitialised value(s)
==13028==    at 0x484E98E: bcmp (vg_replace_strmem.c:1229)
==13028==    by 0x4114BB: nwipe_random_verify (pass.c:198)
==13028==    by 0x416206: nwipe_runmethod (method.c:961)
==13028==    by 0x417179: nwipe_random (method.c:742)
==13028==    by 0x52A1896: start_thread (in /usr/lib64/libc.so.6)
==13028==    by 0x53288C3: clone (in /usr/lib64/libc.so.6)
==13028==  Uninitialised value was created by a stack allocation
==13028==    at 0x414980: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
==13028== 
==13028== Conditional jump or move depends on uninitialised value(s)
==13028==    at 0x484E965: bcmp (vg_replace_strmem.c:1229)
==13028==    by 0x4114BB: nwipe_random_verify (pass.c:198)
==13028==    by 0x416206: nwipe_runmethod (method.c:961)
==13028==    by 0x417179: nwipe_random (method.c:742)
==13028==    by 0x52A1896: start_thread (in /usr/lib64/libc.so.6)
==13028==    by 0x53288C3: clone (in /usr/lib64/libc.so.6)
==13028==  Uninitialised value was created by a stack allocation
==13028==    at 0x414980: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
==13028== 
==13028== Conditional jump or move depends on uninitialised value(s)
==13028==    at 0x4114BE: nwipe_random_verify (pass.c:198)
==13028==    by 0x416206: nwipe_runmethod (method.c:961)
==13028==    by 0x417179: nwipe_random (method.c:742)
==13028==    by 0x52A1896: start_thread (in /usr/lib64/libc.so.6)
==13028==    by 0x53288C3: clone (in /usr/lib64/libc.so.6)
==13028==  Uninitialised value was created by a stack allocation
==13028==    at 0x414980: aes_ctr_prng_genrand_uint256_to_buf (aes_ctr_prng.cpp:126)
Knogle commented 3 months ago

I've been able to get a clean valgrind output now after fixing the uninitialized array. Please test again if possible :)

Old code was:

unsigned char temp_buffer[32];  // Temporary buffer for pseudorandom output.
int outlen;

if( !EVP_EncryptUpdate( cppState->ctx.get(), temp_buffer, &outlen, temp_buffer, sizeof( temp_buffer ) ) )
{
    nwipe_log( NWIPE_LOG_ERROR, "AES-256 CTR PRNG generation failed." );
    return;
}

There are two ways to solve it.

if( !EVP_EncryptUpdate( cppState->ctx.get(), temp_buffer, &outlen, nullptr, sizeof( temp_buffer ) ) )
{
    nwipe_log( NWIPE_LOG_ERROR, "AES-256 CTR PRNG generation failed." );
    return;
}

I've opted for:

std::memset(temp_buffer, 0, sizeof(temp_buffer));

==14685== Memcheck, a memory error detector
==14685== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==14685== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==14685== Command: src/nwipe /dev/loop0 /dev/loop1 /dev/loop2 /dev/loop3 /dev/loop4 /dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8 /dev/loop9 /dev/loop10 /dev/loop11 /dev/loop12 /dev/loop13 /dev/loop14 /dev/loop15
==14685== Parent PID: 14684
==14685== 
==14685== 
==14685== HEAP SUMMARY:
==14685==     in use at exit: 443,703 bytes in 792 blocks
==14685==   total heap usage: 27,268 allocs, 26,476 frees, 24,552,484 bytes allocated
==14685== 
==14685== LEAK SUMMARY:
==14685==    definitely lost: 496 bytes in 18 blocks
==14685==    indirectly lost: 122,115 bytes in 80 blocks
==14685==      possibly lost: 913 bytes in 12 blocks
==14685==    still reachable: 320,179 bytes in 682 blocks
==14685==         suppressed: 0 bytes in 0 blocks
==14685== Rerun with --leak-check=full to see details of leaked memory
==14685== 
==14685== For lists of detected and suppressed errors, rerun with: -s
==14685== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Screenshot from 2024-04-09 11-30-54

Knogle commented 3 months ago

Not necessary anymore, due to successful C implementation https://github.com/martijnvanbrummelen/nwipe/pull/559