using explicit_bzero

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

using explicit_bzero

Peter Eisentraut-6
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@...
[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

0001-Use-explicit_bzero.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Tom Lane-2
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Dagfinn Ilmari Mannsåker
Tom Lane <[hidden email]> writes:

> Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Tom Lane-2
[hidden email] (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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
In reply to this post by Tom Lane-2
On 2019-06-21 15:25, Tom Lane wrote:
> Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
In reply to this post by Dagfinn Ilmari Mannsåker
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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
In reply to this post by Peter Eisentraut-6
On 2019-06-23 21:55, Peter Eisentraut wrote:

> On 2019-06-21 15:25, Tom Lane wrote:
>> Peter Eisentraut <[hidden email]> 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

v2-0001-Use-explicit_bzero.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Michael Paquier-2
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 <[hidden email]> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Michael Paquier-2
In reply to this post by Peter Eisentraut-6
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Dagfinn Ilmari Mannsåker
Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Thomas Munro-5
In reply to this post by Peter Eisentraut-6
On Mon, Jun 24, 2019 at 7:57 AM Peter Eisentraut
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
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

v3-0001-Use-explicit_bzero.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Thomas Munro-5
On Sat, Jul 6, 2019 at 1:07 AM Peter Eisentraut
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
On 2019-07-05 23:02, Thomas Munro wrote:

> On Sat, Jul 6, 2019 at 1:07 AM Peter Eisentraut
> <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Thomas Munro-5
On Sun, Jul 7, 2019 at 1:11 AM Peter Eisentraut
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Michael Paquier-2
In reply to this post by Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Alvaro Herrera-9
In reply to this post by Thomas Munro-5
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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Tom Lane-2
Alvaro Herrera <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
In reply to this post by Michael Paquier-2
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


Reply | Threaded
Open this post in threaded view
|

Re: using explicit_bzero

Peter Eisentraut-6
In reply to this post by Tom Lane-2
On 2019-07-18 00:45, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> 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


12