using explicit_bzero

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]https://github.com/openssh/openssh-portable/blob/master/openbsd-compat/explicit_bzero.c, I learned that C11 has standardised
memset_s[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 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