[PATCH 07/11] libxlMakeNetworkDiskSrc: Don't bother with secure erase of secrets

[Peter Krempa]

On Mon, Dec 12, 2022 at 09:12:15 +0000, Daniel P. Berrangé wrote:
> On Fri, Dec 09, 2022 at 05:28:59PM +0100, Peter Krempa wrote:
> > The contents of both 'secret' and 'base64secret' are part of different
> > buffers wich are not erased securely. Don't bother with virSecureErase*.
> 
> Again, just because other code isn't right, doesn't mean we should
> delete this. Make the other buffer be erased securely instead.

In many cases such as this one the secret is formatted into a buffer
that may be realloc'd while it's being constructed while the secret is
inside.

Assume the following program, which "decrypts" a secret and writes it to
a file. The buffer used for the operation is reallocd() and cleared:

$ cat secret.c 
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>

int main(int argc, const char *argv[])
{
    char *buf;
    const char *fragment1 = "STCSRNM";
    const char *fragment2 = "EEATOOY";
    char *buf2;
    size_t i;
    int b = atoi(argv[1]);
    int fd = open("/dev/null", O_WRONLY);
    char *r1;
    char *r2;

    buf = malloc(b);
    buf2 = malloc(256);
    memcpy(buf2, "somethingelse", strlen("somethingelse"));

    for (i = 0; i < b; i++) {
        if ((i % 2) == 0) {
            buf[i] = fragment1[(i/2)%14];
        } else {
            buf[i] = fragment2[(i/2)%14];
        }
    }

    write(fd, buf, b);
    close(fd);

    r1 = buf;

    if (b != 1024)
        buf = realloc(buf, 1024);

    r2 = buf;

    if (b != 666) {
        for (i = 0; i < b; i++)
            memcpy(buf + i, "X", 2);
    }

    memcpy(buf2, buf, 100);

    getchar();

    printf("r1: %p, r2: %p\n", r1, r2);

    puts(buf);
    puts(buf2);
}


Now see the following cases:

When I run it with a buffer > 1024 or the magic value 1024 when it's not
reallocated the result is:

$ ./a.out 1200 &  sleep 1; gcore $! ; echo corestrings ; strings core.$! | grep SETE ; rm core* ; fg
[1] 2832373

[1]+  Stopped                 ./a.out 1200
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Program received signal SIGTTIN, Stopped (tty input).
0x00007f6265553021 in read () from /lib64/libc.so.6
warning: target file /proc/2832373/cmdline contained unexpected null characters
warning: Memory read failed for corefile section, 4096 bytes at 0xffffffffff600000.
Saved corefile core.2832373
[Inferior 1 (process 2832373) detached]

[1]+  Stopped                 ./a.out 1200
corestrings
./a.out 1200

r1: 0x1a682a0, r2: 0x1a682a0
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX


... but now if I run it with a smaller initial size:

$ ./a.out 150 &  sleep 1; gcore $! ; echo corestrings ; strings core.$! | grep SETE ; rm core* ; fg
[1] 2833245

[1]+  Stopped                 ./a.out 150
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Program received signal SIGTTIN, Stopped (tty input).
0x00007f8c366cd021 in read () from /lib64/libc.so.6
warning: target file /proc/2833245/cmdline contained unexpected null characters
warning: Memory read failed for corefile section, 4096 bytes at 0xffffffffff600000.
Saved corefile core.2833245
[Inferior 1 (process 2833245) detached]

[1]+  Stopped                 ./a.out 150
corestrings
jRE/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRO
E/EdSETECASTRONOMY
O/OnSETECASTRONO
E/EdSETECASTRONOMY
O/OnSETECASTRONO
E/EdSETECASTRONOMY
O/OnSETECASTRONO
./a.out 150

r1: 0xdbf2a0, r2: 0xdbf450
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX


Thus ... for buffers where realloc is in use, the attempt to clear it
afterwards is basically equivalent in not clearing the buffer before
freeing it.