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.
