Обсуждение: using explicit_bzero
In a recent thread[0], the existence of explicit_bzero() was mentioned. I went to look where we could use that to clear sensitive information from memory and found a few candidates: - In be-secure-common.c, clear the entered SSL passphrase in the error path. (In the non-error path, the buffer belongs to OpenSSL.) - In libpq, clean up after reading .pgpass. Otherwise, the entire file including all passwords potentially remains in memory. - In libpq, clear the password after a connection is closed (freePGconn/part of PQfinish). - pg_hba.conf could potentially contain passwords for LDAP, so that should maybe also be cleared, but the structure of that code would make that more involved, so I skipped that for now. Efforts are probably better directed at providing facilities to avoid having to do that.[1] Any other ones? A patch that implements the first three is attached. [0]: https://www.postgresql.org/message-id/043403c2-f04d-3a69-aa8a-9bb7b9ce8e5b@iki.fi [1]: https://www.postgresql.org/message-id/flat/CA%2BhUKGJ44ssWhcKP1KYK2Dm9_XXk1_b629_qSDUhH1fWfuAvXg%40mail.gmail.com -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> +#ifndef HAVE_EXPLICIT_BZERO
> +#define explicit_bzero(b, len) bzero(b, len)
> +#endif
This presumes that every platform has bzero, which is unsafe (POSIX
doesn't specify it) and is an assumption we kicked to the curb a dozen
years ago (067a5cdb3). Please use memset() for the substitute instead.
Also, I'm a bit suspicious of using AC_CHECK_FUNCS for this; that
generally Doesn't Work for anything that's not a vanilla out-of-line
function. Are we worried about people implementing this as a macro,
compiler built-in, etc?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> +#ifndef HAVE_EXPLICIT_BZERO >> +#define explicit_bzero(b, len) bzero(b, len) >> +#endif > > This presumes that every platform has bzero, which is unsafe (POSIX > doesn't specify it) and is an assumption we kicked to the curb a dozen > years ago (067a5cdb3). Please use memset() for the substitute instead. > > Also, I'm a bit suspicious of using AC_CHECK_FUNCS for this; that > generally Doesn't Work for anything that's not a vanilla out-of-line > function. Are we worried about people implementing this as a macro, > compiler built-in, etc? Also, on Linux it requires libbsd: https://libbsd.freedesktop.org/ (which seems to be down, but https://packages.debian.org/buster/libbsd-dev has a list of the functions it provides). - ilmari -- "A disappointingly low fraction of the human race is, at any given time, on fire." - Stig Sandbeck Mathisen
ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes: > Also, on Linux it requires libbsd: https://libbsd.freedesktop.org/ > (which seems to be down, but > https://packages.debian.org/buster/libbsd-dev has a list of the > functions it provides). Ugh, that could be a bit nasty. I might be misremembering, but my hindbrain is running for cover and yelling something about how importing libbsd changes signal semantics. Our git log has a few scary references to other bad side-effects of -lbsd (cf 55c235b26, 1337751e5, a27fafecc). On the whole, I'm not excited about pulling in a library whose entire purpose is to mess with POSIX semantics. regards, tom lane
On 2019-06-21 15:25, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> +#ifndef HAVE_EXPLICIT_BZERO >> +#define explicit_bzero(b, len) bzero(b, len) >> +#endif > > This presumes that every platform has bzero, which is unsafe (POSIX > doesn't specify it) and is an assumption we kicked to the curb a dozen > years ago (067a5cdb3). Please use memset() for the substitute instead. OK, done. > Also, I'm a bit suspicious of using AC_CHECK_FUNCS for this; that > generally Doesn't Work for anything that's not a vanilla out-of-line > function. Are we worried about people implementing this as a macro, > compiler built-in, etc? I think we should address that if we actually find such a case. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-06-21 15:45, Dagfinn Ilmari Mannsåker wrote: > Also, on Linux it requires libbsd: https://libbsd.freedesktop.org/ No, it's in glibc. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-06-23 21:55, Peter Eisentraut wrote: > On 2019-06-21 15:25, Tom Lane wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> +#ifndef HAVE_EXPLICIT_BZERO >>> +#define explicit_bzero(b, len) bzero(b, len) >>> +#endif >> >> This presumes that every platform has bzero, which is unsafe (POSIX >> doesn't specify it) and is an assumption we kicked to the curb a dozen >> years ago (067a5cdb3). Please use memset() for the substitute instead. > > OK, done. and with patch attached -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Sun, Jun 23, 2019 at 09:57:18PM +0200, Peter Eisentraut wrote: > On 2019-06-23 21:55, Peter Eisentraut wrote: >> On 2019-06-21 15:25, Tom Lane wrote: >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>>> +#ifndef HAVE_EXPLICIT_BZERO >>>> +#define explicit_bzero(b, len) bzero(b, len) >>>> +#endif >>> >>> This presumes that every platform has bzero, which is unsafe (POSIX >>> doesn't specify it) and is an assumption we kicked to the curb a dozen >>> years ago (067a5cdb3). Please use memset() for the substitute instead. +1. >> OK, done. > > and with patch attached CreateRole() and AlterRole() can manipulate a password in plain format in memory. The cleanup could be done just after calling encrypt_password() in user.c. Could it be possible to add the new flag in pg_config.h.win32? -- Michael
Вложения
On Sun, Jun 23, 2019 at 09:56:40PM +0200, Peter Eisentraut wrote: > On 2019-06-21 15:45, Dagfinn Ilmari Mannsåker wrote: >> Also, on Linux it requires libbsd: https://libbsd.freedesktop.org/ > > No, it's in glibc. From man: explicit_bzero() first appeared in glibc 2.25. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Sun, Jun 23, 2019 at 09:56:40PM +0200, Peter Eisentraut wrote: >> On 2019-06-21 15:45, Dagfinn Ilmari Mannsåker wrote: >>> Also, on Linux it requires libbsd: https://libbsd.freedesktop.org/ >> >> No, it's in glibc. > > From man: > explicit_bzero() first appeared in glibc 2.25. Ah, I was looking on my Debian Stretch (stable) box, which only has glibc 2.24. Buster (testing, due out next week) has 2.28 which indeed has it. - ilmari -- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl
On Mon, Jun 24, 2019 at 7:57 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 2019-06-23 21:55, Peter Eisentraut wrote:
> > On 2019-06-21 15:25, Tom Lane wrote:
> >> years ago (067a5cdb3). Please use memset() for the substitute instead.
> >
> > OK, done.
+#ifndef HAVE_EXPLICIT_BZERO
+#define explicit_bzero(b, len) memset(b, 0, len)
+#endif
I noticed some other libraries use memset through a function pointer
or at least define a function the compiler can't see.
> and with patch attached
The ssl tests fail:
FATAL: could not load private key file "server-password.key": bad decrypt
That's apparently due to the passphrase being clobbered in the output
buffer before we've managed to use it:
@@ -118,6 +118,7 @@ run_ssl_passphrase_command(const char *prompt,
bool is_server_start, char *buf,
buf[--len] = '\0';
error:
+ explicit_bzero(buf, size);
pfree(command.data);
return len;
}
--
Thomas Munro
https://enterprisedb.com
On 2019-07-05 14:06, Thomas Munro wrote: > +#ifndef HAVE_EXPLICIT_BZERO > +#define explicit_bzero(b, len) memset(b, 0, len) > +#endif > > I noticed some other libraries use memset through a function pointer > or at least define a function the compiler can't see. I don't understand what you are getting at here. > The ssl tests fail: > > FATAL: could not load private key file "server-password.key": bad decrypt > > That's apparently due to the passphrase being clobbered in the output > buffer before we've managed to use it: > > @@ -118,6 +118,7 @@ run_ssl_passphrase_command(const char *prompt, > bool is_server_start, char *buf, > buf[--len] = '\0'; > > error: > + explicit_bzero(buf, size); > pfree(command.data); > return len; > } Yeah, that's a silly mistake. New patch attached. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Sat, Jul 6, 2019 at 1:07 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 2019-07-05 14:06, Thomas Munro wrote: > > +#ifndef HAVE_EXPLICIT_BZERO > > +#define explicit_bzero(b, len) memset(b, 0, len) > > +#endif > > > > I noticed some other libraries use memset through a function pointer > > or at least define a function the compiler can't see. > > I don't understand what you are getting at here. Do we want to provide a replacement implementation that actually prevents the compiler from generating no code in some circumstances? Then I think we need at least a function defined in another translation unit so the compiler can't see what it does, no? -- Thomas Munro https://enterprisedb.com
On 2019-07-05 23:02, Thomas Munro wrote: > On Sat, Jul 6, 2019 at 1:07 AM Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 2019-07-05 14:06, Thomas Munro wrote: >>> +#ifndef HAVE_EXPLICIT_BZERO >>> +#define explicit_bzero(b, len) memset(b, 0, len) >>> +#endif >>> >>> I noticed some other libraries use memset through a function pointer >>> or at least define a function the compiler can't see. >> >> I don't understand what you are getting at here. > > Do we want to provide a replacement implementation that actually > prevents the compiler from generating no code in some circumstances? > Then I think we need at least a function defined in another > translation unit so the compiler can't see what it does, no? I see. My premise, which should perhaps be explained in a comment at least, is that on an operating system that does not provide explicit_bzero() (or an obvious alternative), we don't care about addressing this particular security concern, since the rest of the operating system won't be secure in this way either. It shouldn't be our job to fight this battle if the rest of the OS doesn't care. An alternative patch would define explicit_bzero() to nothing if not available. But that might create bugs if subsequent code relies on the memory being zeroed, independent of security concerns, so I changed it to use memset() so that at least logically both code paths are the same. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Jul 7, 2019 at 1:11 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > I see. My premise, which should perhaps be explained in a comment at > least, is that on an operating system that does not provide > explicit_bzero() (or an obvious alternative), we don't care about > addressing this particular security concern, since the rest of the > operating system won't be secure in this way either. It shouldn't be > our job to fight this battle if the rest of the OS doesn't care. > > An alternative patch would define explicit_bzero() to nothing if not > available. But that might create bugs if subsequent code relies on the > memory being zeroed, independent of security concerns, so I changed it > to use memset() so that at least logically both code paths are the same. Following a trail of crumbs beginning at OpenSSH's fallback implementation of this[1], I learned that C11 has standardised memset_s[2] for this purpose. Macs have memset_s but no explicit_bzero. FreeBSD has both. I wonder if it'd be better to make memset_s the function we use in our code, considering its standard blessing and therefore likelihood of being available on every system eventually. Oh, I see the problem: glibc 2.25 introduced explicit_bzero, but no version of glibc has memset_s yet. So that's why you did it that way... RHEL 8 and Debian 10 ship with explicit_bzero. Bleugh. [1] https://github.com/openssh/openssh-portable/blob/master/openbsd-compat/explicit_bzero.c [2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf#%5B%7B%22num%22%3A1353%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C792%2C0%5D -- Thomas Munro https://enterprisedb.com
On Mon, Jun 24, 2019 at 02:08:50PM +0900, Michael Paquier wrote: > CreateRole() and AlterRole() can manipulate a password in plain format > in memory. The cleanup could be done just after calling > encrypt_password() in user.c. > > Could it be possible to add the new flag in pg_config.h.win32? While remembering about it... Shouldn't the memset(0) now happening in base64.c for the encoding and encoding routines when facing a failure use explicit_zero()? -- Michael
Вложения
On 2019-Jul-11, Thomas Munro wrote: > Following a trail of crumbs beginning at OpenSSH's fallback > implementation of this[1], I learned that C11 has standardised > memset_s[2] for this purpose. Macs have memset_s but no > explicit_bzero. FreeBSD has both. I wonder if it'd be better to make > memset_s the function we use in our code, considering its standard > blessing and therefore likelihood of being available on every system > eventually. Sounds like a future-proof way would be to implement memset_s in src/port if absent from the OS (using explicit_bzero and other tricks), and use that. Here's a portable implementation (includes _WIN32 and NetBSD's explicit_memset) under ISC license: https://github.com/jedisct1/libsodium/blob/master/src/libsodium/sodium/utils.c#L112 (from https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/ ) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jul-11, Thomas Munro wrote:
>> Following a trail of crumbs beginning at OpenSSH's fallback
>> implementation of this[1], I learned that C11 has standardised
>> memset_s[2] for this purpose. Macs have memset_s but no
>> explicit_bzero. FreeBSD has both. I wonder if it'd be better to make
>> memset_s the function we use in our code, considering its standard
>> blessing and therefore likelihood of being available on every system
>> eventually.
> Sounds like a future-proof way would be to implement memset_s in
> src/port if absent from the OS (using explicit_bzero and other tricks),
> and use that.
+1 for using the C11-standard name, even if that's not anywhere
in the real world yet.
regards, tom lane
On 2019-07-11 03:11, Michael Paquier wrote: > On Mon, Jun 24, 2019 at 02:08:50PM +0900, Michael Paquier wrote: >> CreateRole() and AlterRole() can manipulate a password in plain format >> in memory. The cleanup could be done just after calling >> encrypt_password() in user.c. >> >> Could it be possible to add the new flag in pg_config.h.win32? > > While remembering about it... Shouldn't the memset(0) now happening in > base64.c for the encoding and encoding routines when facing a failure > use explicit_zero()? base64.c doesn't know what the data it is dealing with is used for. That should be the responsibility of the caller, no? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-07-18 00:45, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> On 2019-Jul-11, Thomas Munro wrote: >>> Following a trail of crumbs beginning at OpenSSH's fallback >>> implementation of this[1], I learned that C11 has standardised >>> memset_s[2] for this purpose. Macs have memset_s but no >>> explicit_bzero. FreeBSD has both. I wonder if it'd be better to make >>> memset_s the function we use in our code, considering its standard >>> blessing and therefore likelihood of being available on every system >>> eventually. > >> Sounds like a future-proof way would be to implement memset_s in >> src/port if absent from the OS (using explicit_bzero and other tricks), >> and use that. > > +1 for using the C11-standard name, even if that's not anywhere > in the real world yet. ISTM that a problem is that you cannot implement a replacement memset_s() as a wrapper around explicit_bzero(), unless you also want to implement the bound checking stuff. (The "s"/safe in this family of functions refers to the bound checking, not the cannot-be-optimized-away property.) The other way around it is easier. Also, the "s" family of functions appears to be a quagmire of controversy and incompatibility, so it's perhaps better to stay away from it for the time being. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-07-18 00:45, Tom Lane wrote:
>> +1 for using the C11-standard name, even if that's not anywhere
>> in the real world yet.
> ISTM that a problem is that you cannot implement a replacement
> memset_s() as a wrapper around explicit_bzero(), unless you also want to
> implement the bound checking stuff. (The "s"/safe in this family of
> functions refers to the bound checking, not the cannot-be-optimized-away
> property.) The other way around it is easier.
Oh, hm.
> Also, the "s" family of functions appears to be a quagmire of
> controversy and incompatibility, so it's perhaps better to stay away
> from it for the time being.
Fair enough.
regards, tom lane
Another patch, with various fallback implementations. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Mon, Jul 29, 2019 at 11:30:53AM +0200, Peter Eisentraut wrote: > Another patch, with various fallback implementations. I have spotted some issues with this patch: 1) The list of port files @pgportfiles in Mkvcbuild.pm has not been updated with the new file explicit_bzero.c, so the compilation would fail with MSVC. 2) pg_config.h.win32 does not include the two new flags (same as https://www.postgresql.org/message-id/20190624050850.GE1637@paquier.xyz) 3) What about CreateRole() and AlterRole() which can manipulate a password in plain format before hashing? (same message as previous point). Nit: src/port/explicit_bzero.c misses its IDENTIFICATION tag. -- Michael
Вложения
Hi,
On 2019-07-29 11:30:53 +0200, Peter Eisentraut wrote:
> For platforms that don't have explicit_bzero(), provide various
> fallback implementations. (explicit_bzero() itself isn't standard,
> but as Linux/glibc and OpenBSD have it, it's the most common spelling,
> so it makes sense to make that the invocation point.)
I think it's better to have a pg_explicit_bzero or such, and implement
that via the various platform dependant mechanisms. It's considerably
harder to understand code when one is surprised that a function normally
not available is called, the buildsystem part is really hard to
understand (with runtime and code filenames differing etc), and invites
API breakages. And it's not really more work to have our own name.
> +/*-------------------------------------------------------------------------
> + *
> + * explicit_bzero.c
> + *
> + * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + *-------------------------------------------------------------------------
> + */
> +
> +#include "c.h"
> +
> +#if defined(HAVE_MEMSET_S)
> +
> +void
> +explicit_bzero(void *buf, size_t len)
> +{
> + (void) memset_s(buf, len, 0, len);
> +}
> +
> +#elif defined(WIN32)
> +
> +#include "c.h"
Hm?
> +/*
> + * Indirect call through a volatile pointer to hopefully avoid dead-store
> + * optimisation eliminating the call. (Idea taken from OpenSSH.) We can't
> + * assume bzero() is present either, so for simplicity we define our own.
> + */
> +
> +static void
> +bzero2(void *buf, size_t len)
> +{
> + memset(buf, 0, len);
> +}
> +
> +static void (* volatile bzero_p)(void *, size_t) = bzero2;
Hm, I'm not really sure that this does that much. Especially when the
call is via a function in the same translation unit.
> +void
> +explicit_bzero(void *buf, size_t len)
> +{
> + bzero_p(buf, len);
I've not followed this discussion. But why isn't the obvious
implementation here memset(...); pg_compiler_barrier()?
A quick web search indicates that that's what a bunch of projects in the
cryptography space also ended up with (well, __asm__ __volatile__("" :::
"memory"), which is what pg_compiler_barrier boils down to for
gcc/clang/compatibles).
Greetings,
Andres Freund
On Tue, Jul 30, 2019 at 5:58 PM Andres Freund <andres@anarazel.de> wrote:
> > +#include "c.h"
>
> Hm?
Heh.
> > +static void (* volatile bzero_p)(void *, size_t) = bzero2;
>
> Hm, I'm not really sure that this does that much. Especially when the
> call is via a function in the same translation unit.
Yeah, I wondered the same (when reading the OpenSSH version). You'd
think you'd need a non-static global so it has to assume that it could
change.
> > +void
> > +explicit_bzero(void *buf, size_t len)
> > +{
> > + bzero_p(buf, len);
>
> I've not followed this discussion. But why isn't the obvious
> implementation here memset(...); pg_compiler_barrier()?
>
> A quick web search indicates that that's what a bunch of projects in the
> cryptography space also ended up with (well, __asm__ __volatile__("" :::
> "memory"), which is what pg_compiler_barrier boils down to for
> gcc/clang/compatibles).
At a glance, I think 3.4.3 of this 2017 paper says that might not work
on Clang and those other people might have a bug:
https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-yang.pdf
cfbot says:
fe-connect.obj : error LNK2019: unresolved external symbol
explicit_bzero referenced in function freePGconn
[C:\projects\postgresql\libpq.vcxproj]
Moved to next CF.
--
Thomas Munro
https://enterprisedb.com
Hi,
On 2019-08-01 20:08:15 +1200, Thomas Munro wrote:
> On Tue, Jul 30, 2019 at 5:58 PM Andres Freund <andres@anarazel.de> wrote:
> > > +#include "c.h"
> > > +static void (* volatile bzero_p)(void *, size_t) = bzero2;
> >
> > Hm, I'm not really sure that this does that much. Especially when the
> > call is via a function in the same translation unit.
>
> Yeah, I wondered the same (when reading the OpenSSH version). You'd
> think you'd need a non-static global so it has to assume that it could
> change.
The implementations in other projects I saw did the above trick, but
also marked the symbol as weak. Telling the compiler it can't know what
version will be used at runtime. But that adds a bunch of compiler
dependencies too.
> > > +void
> > > +explicit_bzero(void *buf, size_t len)
> > > +{
> > > + bzero_p(buf, len);
> >
> > I've not followed this discussion. But why isn't the obvious
> > implementation here memset(...); pg_compiler_barrier()?
> >
> > A quick web search indicates that that's what a bunch of projects in the
> > cryptography space also ended up with (well, __asm__ __volatile__("" :::
> > "memory"), which is what pg_compiler_barrier boils down to for
> > gcc/clang/compatibles).
>
> At a glance, I think 3.4.3 of this 2017 paper says that might not work
> on Clang and those other people might have a bug:
>
> https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-yang.pdf
https://bugs.llvm.org/show_bug.cgi?id=15495
We could just combine it with volatile out of paranoia anyway. But I'm
also more than bit doubtful about this bugreport. There's simply no
memory here. It's not that the memset is optimized away, it's that
there's no memory at all. It results in:
.file "test.c"
.globl foo # -- Begin function foo
.p2align 4, 0x90
.type foo,@function
foo: # @foo
.cfi_startproc
# %bb.0:
#APP
#NO_APP
retq
there's no secrets left over here. If you actuall force the memory be
filled, even if it's afterwards dead, you do get the memory
cleaned. E.g.
#include <string.h>
static void mybzero(char *buf, int len) {
memset(buf,0,len);
asm("" : : : "memory");
}
extern void grab_password(char *buf, int len);
int main(int argc, char **argv)
{
char buf[512];
grab_password(buf, sizeof(buf));
mybzero(buf, sizeof(buf));
return 0;
}
results in
main: # @main
.cfi_startproc
# %bb.0:
pushq %rbx
.cfi_def_cfa_offset 16
subq $512, %rsp # imm = 0x200
.cfi_def_cfa_offset 528
.cfi_offset %rbx, -16
movq %rsp, %rbx
movq %rbx, %rdi
movl $512, %esi # imm = 0x200
callq grab_password
movl $512, %edx # imm = 0x200
movq %rbx, %rdi
xorl %esi, %esi
callq memset
#APP
#NO_APP
xorl %eax, %eax
addq $512, %rsp # imm = 0x200
.cfi_def_cfa_offset 16
popq %rbx
.cfi_def_cfa_offset 8
retq
.Lfunc_end0:
.size main, .Lfunc_end0-main
.cfi_endproc
# -- End function
Although - and that is not surprising - if you lie and mark
grab_password as being pure (__attribute__((pure)), which signals the
function has no sideeffects except its return value), it'll optimize the
whole memory away again. But no secrets leaked again:
main: # @main
.cfi_startproc
# %bb.0:
#APP
#NO_APP
xorl %eax, %eax
retq
.Lfunc_end0:
.size main, .Lfunc_end0-main
.cfi_endproc
Out of paranoia we could go add add the additional step and have a
barrier variant that's variable specific, and make the __asm__
__volatile__ als take the input as "r"(buf), which'd prevent even this
issue (because now the memory is actually understood as being used).
Which turns out to be e.g. what google did for boringssl...
https://boringssl.googlesource.com/boringssl/+/ad1907fe73334d6c696c8539646c21b11178f20f%5E!/#F0
Greetings,
Andres Freund
On 2019-07-30 07:08, Michael Paquier wrote: > On Mon, Jul 29, 2019 at 11:30:53AM +0200, Peter Eisentraut wrote: >> Another patch, with various fallback implementations. > > I have spotted some issues with this patch: > 1) The list of port files @pgportfiles in Mkvcbuild.pm has not been > updated with the new file explicit_bzero.c, so the compilation would > fail with MSVC. > 2) pg_config.h.win32 does not include the two new flags (same as > https://www.postgresql.org/message-id/20190624050850.GE1637@paquier.xyz) Another patch, to attempt to fix the Windows build. > 3) What about CreateRole() and AlterRole() which can manipulate a > password in plain format before hashing? (same message as previous > point). If you want to secure CREATE ROLE foo PASSWORD 'plaintext' then you need to also analyze memory usage in protocol processing and parsing and the like. This would be a laborious and difficult to verify undertaking. It's better to say, if you want to be secure, don't do that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2019-07-30 07:58, Andres Freund wrote:
> I think it's better to have a pg_explicit_bzero or such, and implement
> that via the various platform dependant mechanisms. It's considerably
> harder to understand code when one is surprised that a function normally
> not available is called, the buildsystem part is really hard to
> understand (with runtime and code filenames differing etc), and invites
> API breakages. And it's not really more work to have our own name.
explicit_bzero() is a pretty established and quasi-standard name by now,
not too different from other things in src/port/.
>> +/*
>> + * Indirect call through a volatile pointer to hopefully avoid dead-store
>> + * optimisation eliminating the call. (Idea taken from OpenSSH.) We can't
>> + * assume bzero() is present either, so for simplicity we define our own.
>> + */
>> +
>> +static void
>> +bzero2(void *buf, size_t len)
>> +{
>> + memset(buf, 0, len);
>> +}
>> +
>> +static void (* volatile bzero_p)(void *, size_t) = bzero2;
>
> Hm, I'm not really sure that this does that much. Especially when the
> call is via a function in the same translation unit.
This is the fallback implementation from OpenSSH, so it's plausible that
it does something. It's worth verifying, of course.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Aug 13, 2019 at 10:30:39AM +0200, Peter Eisentraut wrote: > Another patch, to attempt to fix the Windows build. I have not been able to test the compilation, but the changes look good on this side. -- Michael
Вложения
On 2019-08-14 05:00, Michael Paquier wrote: > On Tue, Aug 13, 2019 at 10:30:39AM +0200, Peter Eisentraut wrote: >> Another patch, to attempt to fix the Windows build. > > I have not been able to test the compilation, but the changes look > good on this side. Rebased patch, no functionality changes. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2019-Aug-24, Peter Eisentraut wrote: > On 2019-08-14 05:00, Michael Paquier wrote: > > On Tue, Aug 13, 2019 at 10:30:39AM +0200, Peter Eisentraut wrote: > >> Another patch, to attempt to fix the Windows build. > > > > I have not been able to test the compilation, but the changes look > > good on this side. > > Rebased patch, no functionality changes. Marked RfC. Can we get on with this? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 04, 2019 at 04:38:21PM -0400, Alvaro Herrera wrote: > Marked RfC. Can we get on with this? FWIW, I have been able to test this one on Windows with MSVC and things are handled correctly. -- Michael
Вложения
On 2019-09-05 04:12, Michael Paquier wrote: > On Wed, Sep 04, 2019 at 04:38:21PM -0400, Alvaro Herrera wrote: >> Marked RfC. Can we get on with this? > > FWIW, I have been able to test this one on Windows with MSVC and > things are handled correctly. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-09-05 08:38:36 +0200, Peter Eisentraut wrote: > On 2019-09-05 04:12, Michael Paquier wrote: > > On Wed, Sep 04, 2019 at 04:38:21PM -0400, Alvaro Herrera wrote: > >> Marked RfC. Can we get on with this? > > > > FWIW, I have been able to test this one on Windows with MSVC and > > things are handled correctly. > > committed I still think this change is done wrongly, by providing an implementation for a library function implemented in various projects. If you e.g. dynamically load a library that implements its own version of bzero, ours will replace it in many cases. I think all this implementation actually guarantees is that bzero2 is read, but not that the copy is not elided. In practice that's *probably* good enough, but a compiler could just check whether bzero_p points to memset. http://cseweb.ucsd.edu/~lerner/papers/dse-usenix2017.pdf https://boringssl-review.googlesource.com/c/boringssl/+/1339/ I think we have absolutely no business possibly intercepting / replacing actually securely coded implementations of sensitive functions. Greetings, Andres Freund
On 2019-09-09 17:18, Andres Freund wrote: > I think all this implementation actually guarantees is that bzero2 is > read, but not that the copy is not elided. In practice that's *probably* > good enough, but a compiler could just check whether bzero_p points to > memset. Are you saying that the replacement implementation we provide is not good enough? If so, I'm happy to look at alternatives. But that's the design from OpenSSH, so if that is wrong, then there are bigger problems. We could also take the OpenBSD implementation, but that has a GCC-ish dependency, so we would probably want the OpenSSH implementation as a fallback anyway. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services