Re: FPW compression leaks information

Now that we have compression of full-page images in WAL, it can be used
to leak sensitive information you're not supposed to see. Somewhat
similar to the recent BREACH and CRIME attacks on SSL, if you can insert
into a table, the compression ratio gives you a hint of how similar the
existing data on the page is to the data you inserted.

That might seem abstract, so let's consider a concrete example. Imagine
that Eve wants to know the password of another user. She can't read
pg_authid directly, but she can change her own password, which causes an
update in pg_authid. The procedure is to:

1. Perform a checkpoint.
2. Call pg_current_xlog_insert_location() to get current WAL position.
3. Change password.
4. Call pg_current_xlog_insert_location() again, subtract the previous
position to get the difference.

Repeat that thousands of times with different passwords, and record the
WAL record size of each attempt. The smaller the WAL size, the more
common characters that guess had with the victim password.

The passwords are (usually) stored as MD5 hashes, but the procedure
works with those too. Each attempt will give a hint on how many common
bytes the attempt had with the victim hash.

There are some complications that make this difficult to perform in
practice. A regular user can't perform a checkpoint directly, she'll
have to generate a lot of other activity to trigger one, or wait until
one happens naturally. The measurement of the WAL record size might be
conflated by other concurrent activity that wrote WAL. And you cannot
force your own pg_authid row to the same page as that of a chosen victim
- so you're limited to guessing victims that happen to be on the same page.

All in all, this is a bit clumsy and very time-consuming to pull off in
practice, but it's possible at least if the conditions are just right.

What should we do about this? Make it configurable on a per-table basis?
Disable FPW compression on system tables? Disable FPW on tables you
don't have SELECT access to? Add a warning to the docs?

- Heikki

* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> What should we do about this? Make it configurable on a per-table
> basis? Disable FPW compression on system tables? Disable FPW on
> tables you don't have SELECT access to? Add a warning to the docs?

REVOKE EXECUTE ON FUNCTION pg_current_xlog_insert_location() FROM public;

Thanks,

Stephen

On 04/09/2015 06:28 PM, Stephen Frost wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> What should we do about this? Make it configurable on a per-table
>> basis? Disable FPW compression on system tables? Disable FPW on
>> tables you don't have SELECT access to? Add a warning to the docs?
>
> REVOKE EXECUTE ON FUNCTION pg_current_xlog_insert_location() FROM public;

Yeah, that's one option. Will also have to revoke access to
pg_stat_replication and anything else that might indirectly give away
the current WAL position.

- Heikki

* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> On 04/09/2015 06:28 PM, Stephen Frost wrote:
> >* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> >>What should we do about this? Make it configurable on a per-table
> >>basis? Disable FPW compression on system tables? Disable FPW on
> >>tables you don't have SELECT access to? Add a warning to the docs?
> >
> >REVOKE EXECUTE ON FUNCTION pg_current_xlog_insert_location() FROM public;
>
> Yeah, that's one option. Will also have to revoke access to
> pg_stat_replication and anything else that might indirectly give
> away the current WAL position.

Sure, though pg_stat_replication already only returns the PID and no
details, unless you're a superuser.

Thanks,

Stephen

On Fri, Apr 10, 2015 at 1:07 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> On 04/09/2015 06:28 PM, Stephen Frost wrote:
>> >* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> >>What should we do about this? Make it configurable on a per-table
>> >>basis? Disable FPW compression on system tables? Disable FPW on
>> >>tables you don't have SELECT access to? Add a warning to the docs?
>> >
>> >REVOKE EXECUTE ON FUNCTION pg_current_xlog_insert_location() FROM public;
>>
>> Yeah, that's one option. Will also have to revoke access to
>> pg_stat_replication and anything else that might indirectly give
>> away the current WAL position.
>
> Sure, though pg_stat_replication already only returns the PID and no
> details, unless you're a superuser.

For consistency we may want to have the same behavior for
pg_current_xlog_insert_location(), pg_last_xlog_receive_location(),
pg_current_xlog_location() and pg_last_xlog_replay_location().
--
Michael

On Apr 9, 2015, at 8:51 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> What should we do about this?

I bet that there are at least 1000 covert channel attacks that are more practically exploitable than this. When this is anywhere near the top of the list of things to worry about, I recommend we throw a huge party and then fly home in our faster-than-light starships which, by then, will probably be available at Walmart.

...Robert

On 04/10/2015 05:17 AM, Robert Haas wrote:
> On Apr 9, 2015, at 8:51 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> What should we do about this?
>
> I bet that there are at least 1000 covert channel attacks that are more practically exploitable than this.

Care to name some? This is certainly quite cumbersome to exploit, but
it's doable.

We've talked a lot about covert channels and timing attacks on RLS, but
this makes me more worried because you can attack passwords stored in
pg_authid.

- Heikki

On Mon, Apr 13, 2015 at 9:38 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 04/10/2015 05:17 AM, Robert Haas wrote:
>>
>> On Apr 9, 2015, at 8:51 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>>
>>> What should we do about this?
>>
>>
>> I bet that there are at least 1000 covert channel attacks that are more
>> practically exploitable than this.
>
>
> Care to name some? This is certainly quite cumbersome to exploit, but it's
> doable.
>
> We've talked a lot about covert channels and timing attacks on RLS, but this
> makes me more worried because you can attack passwords stored in pg_authid.

Isn't the attack mentioned on this thread true as long as a user knows
that a given table stores a password? I don't see why this would be
limited to pg_authid.
--
Michael

On Thu, Apr 9, 2015 at 8:51 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> All in all, this is a bit clumsy and very time-consuming to pull off in
practice, but it's possible at least if the conditions are just right.
>
> What should we do about this? Make it configurable on a per-table basis?

I think making it configurable on a per-table basis have another advantage
of controlling impact of FPW compression for tables that have less
compressible data (there is a CPU overhead of compression even though
it doesn't actually compress much). In-fact I had added such an option
during development of another related patch (WAL compression for Update),
if we think it is useful, I can extract that part of the patch and rebase
it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> On 04/10/2015 05:17 AM, Robert Haas wrote:
> >On Apr 9, 2015, at 8:51 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> >>What should we do about this?
> >
> >I bet that there are at least 1000 covert channel attacks that are more practically exploitable than this.
>
> Care to name some? This is certainly quite cumbersome to exploit,
> but it's doable.

I don't see any good reason to expose this information to every user on
the system, regardless of how easy (or not easy) it is to exploit.

There's a bunch of information which we want monitoring systems to be
able to gather but which shouldn't be generally available and this is
just another example of that.

Thanks,

Stephen

On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Care to name some? This is certainly quite cumbersome to exploit, but it's
> doable.
>
> We've talked a lot about covert channels and timing attacks on RLS, but this
> makes me more worried because you can attack passwords stored in pg_authid.

Well, one thing to think about is that, if we're going to take this
kind of attack seriously, it could be used on user data, too. I mean,
you've just got to be able to get a row into the same block as the row
you want to find out something about, and there's no reason that need
be true only for pg_authid. And, even if you ban access to
information on the pg_xlog insert location, what's to prevent someone
from gleaning similar information via a timing attack on the
compression itself? You can even get some information from figuring
out, by trial and error, how much you need to expand a row to get it
to spill over into another block. If there happens to be enough
free-space on the page where the row is located to allow a HOT update,
you can repeatedly update it until it gets moved to some unrelated
page. Granted, knowing the length of an unseen row isn't as good as
knowing the contents, but it's still potentially useful information.

As to RLS - yeah, that's where I think a lot of the possible covert
channel attacks are. But it doesn't have to be RLS per se. It can
be, for example, a table some of whose contents you expose via a
security barrier view. It can be a security-definer function that you
call and it returns some data that you don't have rights to view
directly. Basically, it can be any table to which you have some
access, but not complete access. Then you can use timing attacks,
scrutinize EXPLAIN plans for clues, look at CTIDs, and so on.

Basically, I'm worried that it's going to be completely impractical to
prevent a certain amount of information leakage when you have partial
access to a table, and that in a largely-futile effort to try to
prevent it, we'll end up making a whole bunch of things (like the WAL
insert position) super-user only, and that this will in fact be a net
reduction in security because it'll encourage people to use the
superuser account more.

Perhaps that concern is ill-founded, but that's what I'm worried about.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04/14/2015 05:42 AM, Robert Haas wrote:
> On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> Care to name some? This is certainly quite cumbersome to exploit, but it's
>> doable.
>>
>> We've talked a lot about covert channels and timing attacks on RLS, but this
>> makes me more worried because you can attack passwords stored in pg_authid.
>
> Well, one thing to think about is that, if we're going to take this
> kind of attack seriously, it could be used on user data, too. I mean,
> you've just got to be able to get a row into the same block as the row
> you want to find out something about, and there's no reason that need
> be true only for pg_authid.

Sure.

> And, even if you ban access to information on the pg_xlog insert
> location, what's to prevent someone from gleaning similar information
> via a timing attack on the compression itself?

Yep, that's another vector. Even more fiddly than the record size, but
nevertheless.

> You can even get some information from figuring
> out, by trial and error, how much you need to expand a row to get it
> to spill over into another block. If there happens to be enough
> free-space on the page where the row is located to allow a HOT update,
> you can repeatedly update it until it gets moved to some unrelated
> page. Granted, knowing the length of an unseen row isn't as good as
> knowing the contents, but it's still potentially useful information.

True as well. That's not an issue for the MD5 hashes stored in
pg_authid, they all have the same length, but it can be for application
data.

> As to RLS - yeah, that's where I think a lot of the possible covert
> channel attacks are. But it doesn't have to be RLS per se. It can
> be, for example, a table some of whose contents you expose via a
> security barrier view. It can be a security-definer function that you
> call and it returns some data that you don't have rights to view
> directly. Basically, it can be any table to which you have some
> access, but not complete access. Then you can use timing attacks,
> scrutinize EXPLAIN plans for clues, look at CTIDs, and so on.
>
> Basically, I'm worried that it's going to be completely impractical to
> prevent a certain amount of information leakage when you have partial
> access to a table, and that in a largely-futile effort to try to
> prevent it, we'll end up making a whole bunch of things (like the WAL
> insert position) super-user only, and that this will in fact be a net
> reduction in security because it'll encourage people to use the
> superuser account more.
>
> Perhaps that concern is ill-founded, but that's what I'm worried about.

I'm not a big fan of locking down WAL position information either. If we
have to treat the current WAL position is security-sensitive
information, we're doing something wrong.

But I don't want to just give up either. One option is to make this
controllable on a per-table basis, as Amit suggested. Then we could
always disable it for pg_authid, add a suitable warning to the docs, and
let the DBA enable/disable it for other tables. It's a bit of a cop-out,
but would fix the immediate problem.

- Heikki

On Tue, Apr 14, 2015 at 4:50 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> On 04/14/2015 05:42 AM, Robert Haas wrote:
>
>> On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
>> wrote:
>>
>>> As to RLS - yeah, that's where I think a lot of the possible covert
>>>
>> channel attacks are. But it doesn't have to be RLS per se. It can
>> be, for example, a table some of whose contents you expose via a
>> security barrier view. It can be a security-definer function that you
>> call and it returns some data that you don't have rights to view
>> directly. Basically, it can be any table to which you have some
>> access, but not complete access. Then you can use timing attacks,
>> scrutinize EXPLAIN plans for clues, look at CTIDs, and so on.
>>
>> Basically, I'm worried that it's going to be completely impractical to
>> prevent a certain amount of information leakage when you have partial
>> access to a table, and that in a largely-futile effort to try to
>> prevent it, we'll end up making a whole bunch of things (like the WAL
>> insert position) super-user only, and that this will in fact be a net
>> reduction in security because it'll encourage people to use the
>> superuser account more.
>>
>> Perhaps that concern is ill-founded, but that's what I'm worried about.
>>
>
> I'm not a big fan of locking down WAL position information either. If we
> have to treat the current WAL position is security-sensitive information,
> we're doing something wrong.
>
> But I don't want to just give up either. One option is to make this
> controllable on a per-table basis, as Amit suggested. Then we could always
> disable it for pg_authid, add a suitable warning to the docs, and let the
> DBA enable/disable it for other tables. It's a bit of a cop-out, but would
> fix the immediate problem.

I think it's a fairly narrow attack vector. As long as we document it
clearly, and make it easy enough to turn it off, I think that's definitely
enough. Most people will not care at all, I'm sure - but we need to cater
to those that do.

I think we could probably even get away with just documenting the risk and
having people turn off the compression *completely* if they care about it,
but if we can do it at a table level, that's obviously a lot better.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> On Tue, Apr 14, 2015 at 4:50 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> > I'm not a big fan of locking down WAL position information either. If we
> > have to treat the current WAL position is security-sensitive information,
> > we're doing something wrong.

I really don't see the downside to limiting who can see that
information. I meant to mention it in my email last night with the
patch allowing the GRANT system to work for other functions, but I added
the functions suggested by Michael to the list of ones which don't have
the default ACL of 'execute to PUBLIC' to that set.

> > But I don't want to just give up either. One option is to make this
> > controllable on a per-table basis, as Amit suggested. Then we could always
> > disable it for pg_authid, add a suitable warning to the docs, and let the
> > DBA enable/disable it for other tables. It's a bit of a cop-out, but would
> > fix the immediate problem.
>
> I think it's a fairly narrow attack vector. As long as we document it
> clearly, and make it easy enough to turn it off, I think that's definitely
> enough. Most people will not care at all, I'm sure - but we need to cater
> to those that do.

I agree that it's something to document, but I don't think it's sensible
to have the default be "at risk". That said, as long as there's a way
to restrict access to these functions (without having to fight with the
system on upgrades, etc, to have that be preserved..) then I'm not going
to push too hard about the default. We need a proper "hardening"
guide anyway, this would just need to be included in that documentation.

> I think we could probably even get away with just documenting the risk and
> having people turn off the compression *completely* if they care about it,
> but if we can do it at a table level, that's obviously a lot better.

I *really* don't like the idea that the "fix" for such an information
disclosure risk is to completely disable an extremely useful feature.
In fact, I find the suggestion of it rather ridiculous.

I do like the idea of being able to control this on a per-table basis
as that's a useful feature, but that's completely independent from this
concern.

Thanks!

Stephen

On Wed, Apr 15, 2015 at 12:00 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Tue, Apr 14, 2015 at 4:50 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>
>> On 04/14/2015 05:42 AM, Robert Haas wrote:
>>>
>>> On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
>>> wrote:
>>>>
>>>> As to RLS - yeah, that's where I think a lot of the possible covert
>>>
>>> channel attacks are. But it doesn't have to be RLS per se. It can
>>> be, for example, a table some of whose contents you expose via a
>>> security barrier view. It can be a security-definer function that you
>>> call and it returns some data that you don't have rights to view
>>> directly. Basically, it can be any table to which you have some
>>> access, but not complete access. Then you can use timing attacks,
>>> scrutinize EXPLAIN plans for clues, look at CTIDs, and so on.
>>>
>>> Basically, I'm worried that it's going to be completely impractical to
>>> prevent a certain amount of information leakage when you have partial
>>> access to a table, and that in a largely-futile effort to try to
>>> prevent it, we'll end up making a whole bunch of things (like the WAL
>>> insert position) super-user only, and that this will in fact be a net
>>> reduction in security because it'll encourage people to use the
>>> superuser account more.
>>>
>>> Perhaps that concern is ill-founded, but that's what I'm worried about.
>>
>>
>> I'm not a big fan of locking down WAL position information either. If we
>> have to treat the current WAL position is security-sensitive information,
>> we're doing something wrong.
>>
>> But I don't want to just give up either. One option is to make this
>> controllable on a per-table basis, as Amit suggested. Then we could always
>> disable it for pg_authid, add a suitable warning to the docs, and let the
>> DBA enable/disable it for other tables. It's a bit of a cop-out, but would
>> fix the immediate problem.
>
>
> I think it's a fairly narrow attack vector. As long as we document it
> clearly, and make it easy enough to turn it off, I think that's definitely
> enough. Most people will not care at all, I'm sure - but we need to cater to
> those that do.
>
> I think we could probably even get away with just documenting the risk and
> having people turn off the compression *completely* if they care about it,
> but if we can do it at a table level, that's obviously a lot better.

+1

Regards,

--
Fujii Masao

On Wed, Apr 15, 2015 at 11:10 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2015 at 12:00 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Tue, Apr 14, 2015 at 4:50 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>>
>>> On 04/14/2015 05:42 AM, Robert Haas wrote:
>>>>
>>>> On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
>>>> wrote:
>>>>>
>>>>> As to RLS - yeah, that's where I think a lot of the possible covert
>>>>
>>>> channel attacks are. But it doesn't have to be RLS per se. It can
>>>> be, for example, a table some of whose contents you expose via a
>>>> security barrier view. It can be a security-definer function that you
>>>> call and it returns some data that you don't have rights to view
>>>> directly. Basically, it can be any table to which you have some
>>>> access, but not complete access. Then you can use timing attacks,
>>>> scrutinize EXPLAIN plans for clues, look at CTIDs, and so on.
>>>>
>>>> Basically, I'm worried that it's going to be completely impractical to
>>>> prevent a certain amount of information leakage when you have partial
>>>> access to a table, and that in a largely-futile effort to try to
>>>> prevent it, we'll end up making a whole bunch of things (like the WAL
>>>> insert position) super-user only, and that this will in fact be a net
>>>> reduction in security because it'll encourage people to use the
>>>> superuser account more.
>>>>
>>>> Perhaps that concern is ill-founded, but that's what I'm worried about.
>>>
>>>
>>> I'm not a big fan of locking down WAL position information either. If we
>>> have to treat the current WAL position is security-sensitive information,
>>> we're doing something wrong.
>>>
>>> But I don't want to just give up either. One option is to make this
>>> controllable on a per-table basis, as Amit suggested. Then we could always
>>> disable it for pg_authid, add a suitable warning to the docs, and let the
>>> DBA enable/disable it for other tables. It's a bit of a cop-out, but would
>>> fix the immediate problem.
>>
>>
>> I think it's a fairly narrow attack vector. As long as we document it
>> clearly, and make it easy enough to turn it off, I think that's definitely
>> enough. Most people will not care at all, I'm sure - but we need to cater to
>> those that do.
>>
>> I think we could probably even get away with just documenting the risk and
>> having people turn off the compression *completely* if they care about it,
>> but if we can do it at a table level, that's obviously a lot better.
>
> +1

OK. I am fine to implement anything required here if needed, meaning
the following:
1) Doc patch to mention that it is possible that compression can give
hints to attackers when working on sensible fields that have a
non-fixed size.
2) Switch at relation level to control wal_compression. This needs to
change XLogRecordAssemble by adding some new logic to check if a
relation is enforcing WAL compression or not. As a reloption, there
are three possible values: true, false and fallback to system default.
Also, I think that we should simply extend XLogRegisterBuffer() and
pass to it the reloption flag that is then registered in
registered_buffer, and XLogRecordAssemble() decides with this flag if
block is compressed or not. Do we want to add this reloption switch to
indexes as well? Or only tables? For indexes things will get heavier
as we would need to add a parameter for all the index types.
Regards,
--
Michael

On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
> OK. I am fine to implement anything required here if needed, meaning
> the following:
> 1) Doc patch to mention that it is possible that compression can give
> hints to attackers when working on sensible fields that have a
> non-fixed size.
> 2) Switch at relation level to control wal_compression. This needs to
> change XLogRecordAssemble by adding some new logic to check if a
> relation is enforcing WAL compression or not. As a reloption, there
> are three possible values: true, false and fallback to system default.
> Also, I think that we should simply extend XLogRegisterBuffer() and
> pass to it the reloption flag that is then registered in
> registered_buffer, and XLogRecordAssemble() decides with this flag if
> block is compressed or not. Do we want to add this reloption switch to
> indexes as well? Or only tables? For indexes things will get heavier
> as we would need to add a parameter for all the index types.

After looking at reloptions.c, what we are going to need as well is a
new relopt_type, like RELOPT_TYPE_ENUM for this purpose to be able to
define the 'default' value. We could as well have things using
RELOPT_TYPE_STRING. Any input on this matter is welcome.
--
Michael

On Wed, Apr 15, 2015 at 12:15 AM, Stephen Frost wrote:
> We need a proper "hardening"
> guide anyway, this would just need to be included in that documentation.

+1. I am sure that many users would like a hardening manual in the
official documentation.
--
Michael

On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2015 at 11:10 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Apr 15, 2015 at 12:00 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> On Tue, Apr 14, 2015 at 4:50 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>>>
>>>> On 04/14/2015 05:42 AM, Robert Haas wrote:
>>>>>
>>>>> On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
>>>>> wrote:
>>>>>>
>>>>>> As to RLS - yeah, that's where I think a lot of the possible covert
>>>>>
>>>>> channel attacks are. But it doesn't have to be RLS per se. It can
>>>>> be, for example, a table some of whose contents you expose via a
>>>>> security barrier view. It can be a security-definer function that you
>>>>> call and it returns some data that you don't have rights to view
>>>>> directly. Basically, it can be any table to which you have some
>>>>> access, but not complete access. Then you can use timing attacks,
>>>>> scrutinize EXPLAIN plans for clues, look at CTIDs, and so on.
>>>>>
>>>>> Basically, I'm worried that it's going to be completely impractical to
>>>>> prevent a certain amount of information leakage when you have partial
>>>>> access to a table, and that in a largely-futile effort to try to
>>>>> prevent it, we'll end up making a whole bunch of things (like the WAL
>>>>> insert position) super-user only, and that this will in fact be a net
>>>>> reduction in security because it'll encourage people to use the
>>>>> superuser account more.
>>>>>
>>>>> Perhaps that concern is ill-founded, but that's what I'm worried about.
>>>>
>>>>
>>>> I'm not a big fan of locking down WAL position information either. If we
>>>> have to treat the current WAL position is security-sensitive information,
>>>> we're doing something wrong.
>>>>
>>>> But I don't want to just give up either. One option is to make this
>>>> controllable on a per-table basis, as Amit suggested. Then we could always
>>>> disable it for pg_authid, add a suitable warning to the docs, and let the
>>>> DBA enable/disable it for other tables. It's a bit of a cop-out, but would
>>>> fix the immediate problem.
>>>
>>>
>>> I think it's a fairly narrow attack vector. As long as we document it
>>> clearly, and make it easy enough to turn it off, I think that's definitely
>>> enough. Most people will not care at all, I'm sure - but we need to cater to
>>> those that do.
>>>
>>> I think we could probably even get away with just documenting the risk and
>>> having people turn off the compression *completely* if they care about it,
>>> but if we can do it at a table level, that's obviously a lot better.
>>
>> +1
>
> OK. I am fine to implement anything required here if needed, meaning
> the following:
> 1) Doc patch to mention that it is possible that compression can give
> hints to attackers when working on sensible fields that have a
> non-fixed size.

I think that this patch is enough as the first step.

> 2) Switch at relation level to control wal_compression.

ALTER TABLE SET is not allowed on system catalog like pg_authid. So should we
change it so that a user can change the flag even on system catalog? I'm afraid
that the change might cause another problem, though. Probably we can disable
the compression on every system catalogs by default. But I can imagine that
someone wants to enable the compression even on system catalog. For example,
pg_largeobject may cause lots of FPW.

> This needs to
> change XLogRecordAssemble by adding some new logic to check if a
> relation is enforcing WAL compression or not. As a reloption, there
> are three possible values: true, false and fallback to system default.
> Also, I think that we should simply extend XLogRegisterBuffer() and
> pass to it the reloption flag that is then registered in
> registered_buffer, and XLogRecordAssemble() decides with this flag if
> block is compressed or not. Do we want to add this reloption switch to
> indexes as well?

Maybe.

Regards,

--
Fujii Masao

On 04/10/2015 05:17 AM, Robert Haas wrote:
> I bet that there are at least 1000 covert channel attacks that are
> more practically exploitable than this. When this is anywhere near
> the top of the list of things to worry about, I recommend we throw a
> huge party and then fly home in our faster-than-light starships
> which, by then, will probably be available at Walmart.

Let's try to put some perspective on how serious (or not) this leak is.
I'm going to use finding a password hash in pg_authid as the target.

After each checkpoint, we can make one "guess", and see how that
compresses. If we assume ideal conditions, and guess one hex digit at a
time (the md5 hashes are stored as hex strings), you can find a 32 digit
hash in 16*32 = 512 checkpoints. You could possible improve on that, by
doing a binary search of the digits: first use a "guess" that contains
digits 0-8, then 9-f, and find which range the victim was in. The split
that range in two, repeat, until you find the digit. With that method,
the theoretical minimum is log2(16)*32 = 128 checkpoints.

Obviously, the real world is more complicated and you won't get very
close to that ideal case.

For my entertainment, I wrote a little exploit script (attached). I
didn't use the binary-search approach, it guesses one digit at a time.
Testing on my laptop, in a newly initdb'd database with no other
activity, it found a full MD5 hash in 3594 checkpoints. I cheated to
make it faster, and used the CHECKPOINT command instead of waiting or
writing a lot of WAL, to trigger the checkpoints, but that shouldn't
affect the number of checkpoints required. Your mileage may vary, of
course, and I'm sure this script will get confused and not work in many
cases, but OTOH I'm sure you could also optimize it further.

If we take that ~4000 checkpoints figure, and checkpoint_timeout=5
minutes, it would take about two weeks. But if you're happy with getting
just the first 4 or 8 bytes or so, and launch a dictionary attack on
that, you can stop much earlier than that.

- Heikki

On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
>> 1) Doc patch to mention that it is possible that compression can give
>> hints to attackers when working on sensible fields that have a
>> non-fixed size.
>
> I think that this patch is enough as the first step.

I'll get something done for that at least, a big warning below the
description of wal_compression would do it.

>> 2) Switch at relation level to control wal_compression.
>
> ALTER TABLE SET is not allowed on system catalog like pg_authid. So should we
> change it so that a user can change the flag even on system catalog? I'm afraid
> that the change might cause another problem, though. Probably we can disable
> the compression on every system catalogs by default. But I can imagine that
> someone wants to enable the compression even on system catalog. For example,
> pg_largeobject may cause lots of FPW.

We could enforce a value directly in pg_class.h for only pg_authid if
we think that it is a problem that bad, and rely on the default system
value for the rest. That's a hacky-ugly approach though...
--
Michael

On Wed, Apr 15, 2015 at 9:20 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
>> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
>>> 1) Doc patch to mention that it is possible that compression can give
>>> hints to attackers when working on sensible fields that have a
>>> non-fixed size.
>>
>> I think that this patch is enough as the first step.
>
> I'll get something done for that at least, a big warning below the
> description of wal_compression would do it.
>
>>> 2) Switch at relation level to control wal_compression.
>>
>> ALTER TABLE SET is not allowed on system catalog like pg_authid. So should we
>> change it so that a user can change the flag even on system catalog? I'm afraid
>> that the change might cause another problem, though. Probably we can disable
>> the compression on every system catalogs by default. But I can imagine that
>> someone wants to enable the compression even on system catalog. For example,
>> pg_largeobject may cause lots of FPW.
>
> We could enforce a value directly in pg_class.h for only pg_authid if
> we think that it is a problem that bad, and rely on the default system
> value for the rest. That's a hacky-ugly approach though...

Something else that I recalled and has not yet been mentioned on this
thread. Even if the server-wide wal_compression is off, any user can
change its value because it is PGC_USERSET, hence I think that we had
better make it at least PGC_SUSET.
--
Michael

On Wed, Apr 15, 2015 at 9:42 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2015 at 9:20 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
>>> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
>>>> 1) Doc patch to mention that it is possible that compression can give
>>>> hints to attackers when working on sensible fields that have a
>>>> non-fixed size.
>>>
>>> I think that this patch is enough as the first step.
>>
>> I'll get something done for that at least, a big warning below the
>> description of wal_compression would do it.

So here is a patch for this purpose, with the following text being used:
+ <warning>
+ <para>
+ When enabling <varname>wal_compression</varname>, there is a risk
+ to leak data similarly to the BREACH and CRIME attacks on SSL where
+ the compression ratio of a full page image gives a hint of what is
+ the existing data of this page. Tables that contain sensitive
+ information like <structname>pg_authid</structname> with password
+ data could be potential targets to such attacks. Note that as a
+ prerequisite a user needs to be able to insert data on the same page
+ as the data targeted and need to be able to detect checkpoint
+ presence to find out if a compressed full page write is included in
+ WAL to calculate the compression ratio of a page using WAL positions
+ before and after inserting data on the page with data targeted.
+ </para>
+ </warning>

Comments and reformulations are welcome.
Regards,
--
Michael

On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2015 at 9:42 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Wed, Apr 15, 2015 at 9:20 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
>>>> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
>>>>> 1) Doc patch to mention that it is possible that compression can give
>>>>> hints to attackers when working on sensible fields that have a
>>>>> non-fixed size.
>>>>
>>>> I think that this patch is enough as the first step.
>>>
>>> I'll get something done for that at least, a big warning below the
>>> description of wal_compression would do it.
>
> So here is a patch for this purpose, with the following text being used:
> + <warning>
> + <para>
> + When enabling <varname>wal_compression</varname>, there is a risk
> + to leak data similarly to the BREACH and CRIME attacks on SSL where
> + the compression ratio of a full page image gives a hint of what is
> + the existing data of this page. Tables that contain sensitive
> + information like <structname>pg_authid</structname> with password
> + data could be potential targets to such attacks. Note that as a
> + prerequisite a user needs to be able to insert data on the same page
> + as the data targeted and need to be able to detect checkpoint
> + presence to find out if a compressed full page write is included in
> + WAL to calculate the compression ratio of a page using WAL positions
> + before and after inserting data on the page with data targeted.
> + </para>
> + </warning>
>
> Comments and reformulations are welcome.

To make things on this thread move on, I just wanted to add that we
should make wal_compression SUSET without waiting if we make this
parameter a reloption or not. As things stand, even if it is a
reloption, any user can enforce its value in a given session.
--
Michael

On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2015 at 9:42 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Wed, Apr 15, 2015 at 9:20 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
>>>> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
>>>>> 1) Doc patch to mention that it is possible that compression can give
>>>>> hints to attackers when working on sensible fields that have a
>>>>> non-fixed size.
>>>>
>>>> I think that this patch is enough as the first step.
>>>
>>> I'll get something done for that at least, a big warning below the
>>> description of wal_compression would do it.
>
> So here is a patch for this purpose, with the following text being used:

Thanks for the patch!

> + <warning>
> + <para>
> + When enabling <varname>wal_compression</varname>, there is a risk
> + to leak data similarly to the BREACH and CRIME attacks on SSL where

Isn't it better to add the link to the corresponding wikipedia page for
the terms BREACH and CRIME?

> + the compression ratio of a full page image gives a hint of what is
> + the existing data of this page. Tables that contain sensitive
> + information like <structname>pg_authid</structname> with password
> + data could be potential targets to such attacks. Note that as a
> + prerequisite a user needs to be able to insert data on the same page
> + as the data targeted and need to be able to detect checkpoint
> + presence to find out if a compressed full page write is included in
> + WAL to calculate the compression ratio of a page using WAL positions
> + before and after inserting data on the page with data targeted.
> + </para>
> + </warning>

I think that, in addition to the description of the risk, it's better to
say that this vulnerability is cumbersome to exploit in practice.

Attached is the updated version of the patch. Comments?

Regards,

--
Fujii Masao

On Sat, May 30, 2015 at 4:58 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Wed, Apr 15, 2015 at 9:42 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Wed, Apr 15, 2015 at 9:20 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
>>>>> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
>>>>>> 1) Doc patch to mention that it is possible that compression can give
>>>>>> hints to attackers when working on sensible fields that have a
>>>>>> non-fixed size.
>>>>>
>>>>> I think that this patch is enough as the first step.
>>>>
>>>> I'll get something done for that at least, a big warning below the
>>>> description of wal_compression would do it.
>>
>> So here is a patch for this purpose, with the following text being used:
>> + <warning>
>> + <para>
>> + When enabling <varname>wal_compression</varname>, there is a risk
>> + to leak data similarly to the BREACH and CRIME attacks on SSL where
>> + the compression ratio of a full page image gives a hint of what is
>> + the existing data of this page. Tables that contain sensitive
>> + information like <structname>pg_authid</structname> with password
>> + data could be potential targets to such attacks. Note that as a
>> + prerequisite a user needs to be able to insert data on the same page
>> + as the data targeted and need to be able to detect checkpoint
>> + presence to find out if a compressed full page write is included in
>> + WAL to calculate the compression ratio of a page using WAL positions
>> + before and after inserting data on the page with data targeted.
>> + </para>
>> + </warning>
>>
>> Comments and reformulations are welcome.
>
> To make things on this thread move on, I just wanted to add that we
> should make wal_compression SUSET

I'm OK to make it SUSET.

Regards,

--
Fujii Masao

* Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
> On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Wed, Apr 15, 2015 at 9:42 PM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Wed, Apr 15, 2015 at 9:20 PM, Michael Paquier
> >> <michael(dot)paquier(at)gmail(dot)com> wrote:
> >>> On Wed, Apr 15, 2015 at 2:22 PM, Fujii Masao wrote:
> >>>> On Wed, Apr 15, 2015 at 11:55 AM, Michael Paquier wrote:
> >>>>> 1) Doc patch to mention that it is possible that compression can give
> >>>>> hints to attackers when working on sensible fields that have a
> >>>>> non-fixed size.

It's more than "hints", from my recollection of what Heikki
demonstrated.

> >>>> I think that this patch is enough as the first step.
> >>>
> >>> I'll get something done for that at least, a big warning below the
> >>> description of wal_compression would do it.
> >
> > So here is a patch for this purpose, with the following text being used:
>
> Thanks for the patch!
>
> > + <warning>
> > + <para>
> > + When enabling <varname>wal_compression</varname>, there is a risk
> > + to leak data similarly to the BREACH and CRIME attacks on SSL where
>
> Isn't it better to add the link to the corresponding wikipedia page for
> the terms BREACH and CRIME?

Isn't policy that we don't link out from our docs..?

> > + the compression ratio of a full page image gives a hint of what is
> > + the existing data of this page. Tables that contain sensitive
> > + information like <structname>pg_authid</structname> with password
> > + data could be potential targets to such attacks. Note that as a
> > + prerequisite a user needs to be able to insert data on the same page
> > + as the data targeted and need to be able to detect checkpoint
> > + presence to find out if a compressed full page write is included in
> > + WAL to calculate the compression ratio of a page using WAL positions
> > + before and after inserting data on the page with data targeted.
> > + </para>
> > + </warning>
>
> I think that, in addition to the description of the risk, it's better to
> say that this vulnerability is cumbersome to exploit in practice.
>
> Attached is the updated version of the patch. Comments?

Personally, I don't like simply documenting this issue. I'd much rather
we restrict the WAL info as it's really got no business being available
to the general public. I'd be fine with restricting that information to
superusers when compression is enabled, or always, for 9.5 and then
fixing it properly in 9.6 by allowing it to be visible to a
"pg_monitoring" default role which admins can then grant to users who
should be able to see it.

Yes, we'll get flak from people who are running with non-superuser
monitoring tools today, but we already have a bunch of superuser-only
things that monioring tools want, so this doesn't move the needle
particularly far, in my view.

Thanks!

Stephen

On 07/07/2015 04:15 PM, Stephen Frost wrote:
> * Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
>> On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> + the compression ratio of a full page image gives a hint of what is
>>> + the existing data of this page. Tables that contain sensitive
>>> + information like <structname>pg_authid</structname> with password
>>> + data could be potential targets to such attacks. Note that as a
>>> + prerequisite a user needs to be able to insert data on the same page
>>> + as the data targeted and need to be able to detect checkpoint
>>> + presence to find out if a compressed full page write is included in
>>> + WAL to calculate the compression ratio of a page using WAL positions
>>> + before and after inserting data on the page with data targeted.
>>> + </para>
>>> + </warning>
>>
>> I think that, in addition to the description of the risk, it's better to
>> say that this vulnerability is cumbersome to exploit in practice.
>>
>> Attached is the updated version of the patch. Comments?
>
> Personally, I don't like simply documenting this issue. I'd much rather
> we restrict the WAL info as it's really got no business being available
> to the general public. I'd be fine with restricting that information to
> superusers when compression is enabled, or always, for 9.5 and then
> fixing it properly in 9.6 by allowing it to be visible to a
> "pg_monitoring" default role which admins can then grant to users who
> should be able to see it.

Well, then you could still launch the same attack if you have the
pg_monitoring privileges. So it would be more like a "monitoring and
grab everyone's passwords" privilege ;-). Ok, in seriousness the attack
isn't that easy to perform, but I still wouldn't feel comfortable giving
that privilege to anyone who isn't a superuser anyway.

> Yes, we'll get flak from people who are running with non-superuser
> monitoring tools today, but we already have a bunch of superuser-only
> things that monioring tools want, so this doesn't move the needle
> particularly far, in my view.

That would be a major drawback IMHO, and a step in the wrong direction.

- Heikki

* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> On 07/07/2015 04:15 PM, Stephen Frost wrote:
> >* Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
> >>On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
> >><michael(dot)paquier(at)gmail(dot)com> wrote:
> >>>+ the compression ratio of a full page image gives a hint of what is
> >>>+ the existing data of this page. Tables that contain sensitive
> >>>+ information like <structname>pg_authid</structname> with password
> >>>+ data could be potential targets to such attacks. Note that as a
> >>>+ prerequisite a user needs to be able to insert data on the same page
> >>>+ as the data targeted and need to be able to detect checkpoint
> >>>+ presence to find out if a compressed full page write is included in
> >>>+ WAL to calculate the compression ratio of a page using WAL positions
> >>>+ before and after inserting data on the page with data targeted.
> >>>+ </para>
> >>>+ </warning>
> >>
> >>I think that, in addition to the description of the risk, it's better to
> >>say that this vulnerability is cumbersome to exploit in practice.
> >>
> >>Attached is the updated version of the patch. Comments?
> >
> >Personally, I don't like simply documenting this issue. I'd much rather
> >we restrict the WAL info as it's really got no business being available
> >to the general public. I'd be fine with restricting that information to
> >superusers when compression is enabled, or always, for 9.5 and then
> >fixing it properly in 9.6 by allowing it to be visible to a
> >"pg_monitoring" default role which admins can then grant to users who
> >should be able to see it.
>
> Well, then you could still launch the same attack if you have the
> pg_monitoring privileges. So it would be more like a "monitoring and
> grab everyone's passwords" privilege ;-). Ok, in seriousness the
> attack isn't that easy to perform, but I still wouldn't feel
> comfortable giving that privilege to anyone who isn't a superuser
> anyway.

The alternative is to have monitoring tools which are running as
superuser, which, in my view at least, is far worse.

> >Yes, we'll get flak from people who are running with non-superuser
> >monitoring tools today, but we already have a bunch of superuser-only
> >things that monioring tools want, so this doesn't move the needle
> >particularly far, in my view.
>
> That would be a major drawback IMHO, and a step in the wrong direction.

I'm not following. If we don't want the information to be available to
everyone then we need to limit it, and right now the only way to do that
is to restrict it to superuser because we haven't got anything more
granular.

In other words, it seems like your above response about not wanting this
to be available to anyone except superusers is counter to this last
sentence where you seem to be saying we should continue to have the
information available to everyone and simply document that there's a
risk there as in the proposed patch.

Thanks,

Stephen

On Tue, Jul 7, 2015 at 10:31 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> On 07/07/2015 04:15 PM, Stephen Frost wrote:
>> >* Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
>> >>On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
>> >><michael(dot)paquier(at)gmail(dot)com> wrote:
>> >>>+ the compression ratio of a full page image gives a hint of what is
>> >>>+ the existing data of this page. Tables that contain sensitive
>> >>>+ information like <structname>pg_authid</structname> with password
>> >>>+ data could be potential targets to such attacks. Note that as a
>> >>>+ prerequisite a user needs to be able to insert data on the same page
>> >>>+ as the data targeted and need to be able to detect checkpoint
>> >>>+ presence to find out if a compressed full page write is included in
>> >>>+ WAL to calculate the compression ratio of a page using WAL positions
>> >>>+ before and after inserting data on the page with data targeted.
>> >>>+ </para>
>> >>>+ </warning>
>> >>
>> >>I think that, in addition to the description of the risk, it's better to
>> >>say that this vulnerability is cumbersome to exploit in practice.
>> >>
>> >>Attached is the updated version of the patch. Comments?
>> >
>> >Personally, I don't like simply documenting this issue. I'd much rather
>> >we restrict the WAL info as it's really got no business being available
>> >to the general public. I'd be fine with restricting that information to
>> >superusers when compression is enabled, or always, for 9.5 and then
>> >fixing it properly in 9.6 by allowing it to be visible to a
>> >"pg_monitoring" default role which admins can then grant to users who
>> >should be able to see it.
>>
>> Well, then you could still launch the same attack if you have the
>> pg_monitoring privileges. So it would be more like a "monitoring and
>> grab everyone's passwords" privilege ;-). Ok, in seriousness the
>> attack isn't that easy to perform, but I still wouldn't feel
>> comfortable giving that privilege to anyone who isn't a superuser
>> anyway.
>
> The alternative is to have monitoring tools which are running as
> superuser, which, in my view at least, is far worse.
>
>> >Yes, we'll get flak from people who are running with non-superuser
>> >monitoring tools today, but we already have a bunch of superuser-only
>> >things that monioring tools want, so this doesn't move the needle
>> >particularly far, in my view.
>>
>> That would be a major drawback IMHO, and a step in the wrong direction.
>
> I'm not following. If we don't want the information to be available to
> everyone then we need to limit it, and right now the only way to do that
> is to restrict it to superuser because we haven't got anything more
> granular.

A user can very easily limit such information by not enabling wal_compression.
No need to restrict the usage of WAL location functions.
Of course, as Michael suggested, we need to make the parameter SUSET
so that only superuser can change the setting, though.

Or you're concerned about the case where a careless user enables
wal_compression unexpectedly even though he or she thinks the risk
very serious? Yes, in this case, non-superuser may be able to exploit
the vulnerability by seeing the WAL position. But there are many cases
where improper setting causes unwanted result. So I'm not sure why
we need to treat wal_compression so special.

Regards,

--
Fujii Masao

* Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
> On Tue, Jul 7, 2015 at 10:31 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I'm not following. If we don't want the information to be available to
> > everyone then we need to limit it, and right now the only way to do that
> > is to restrict it to superuser because we haven't got anything more
> > granular.
>
> A user can very easily limit such information by not enabling wal_compression.
> No need to restrict the usage of WAL location functions.
> Of course, as Michael suggested, we need to make the parameter SUSET
> so that only superuser can change the setting, though.

I agree with making it SUSET, but that doesn't address the issue.

> Or you're concerned about the case where a careless user enables
> wal_compression unexpectedly even though he or she thinks the risk
> very serious? Yes, in this case, non-superuser may be able to exploit
> the vulnerability by seeing the WAL position. But there are many cases
> where improper setting causes unwanted result. So I'm not sure why
> we need to treat wal_compression so special.

If I understand correctly, and perhaps I don't, this is an optimization
which is an improvment in a large number of cases, to the point where we
should almost certainly have it enabled by default. Having an
exploitable hole in an optimization tunable is akin to having -O3 in gcc
remove bounds checking.

What is the postgresql.conf entry going to look like?

#wal_compression = off # Do not enable, security risk

And this is all to preserve having the WAL location information be world
readable? I do not understand how that makes sense.

Further, I'm very curious what other optimization tunables we have which
open security holes in PG. This is not the same as making untrusted
users superusers or creating poorly written and exploitable security
definer functions.

Thanks,

Stephen

On 07/07/2015 04:31 PM, Stephen Frost wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> On 07/07/2015 04:15 PM, Stephen Frost wrote:
>>> * Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
>>>> On Thu, Apr 16, 2015 at 4:26 PM, Michael Paquier
>>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>>> + the compression ratio of a full page image gives a hint of what is
>>>>> + the existing data of this page. Tables that contain sensitive
>>>>> + information like <structname>pg_authid</structname> with password
>>>>> + data could be potential targets to such attacks. Note that as a
>>>>> + prerequisite a user needs to be able to insert data on the same page
>>>>> + as the data targeted and need to be able to detect checkpoint
>>>>> + presence to find out if a compressed full page write is included in
>>>>> + WAL to calculate the compression ratio of a page using WAL positions
>>>>> + before and after inserting data on the page with data targeted.
>>>>> + </para>
>>>>> + </warning>
>>>>
>>>> I think that, in addition to the description of the risk, it's better to
>>>> say that this vulnerability is cumbersome to exploit in practice.
>>>>
>>>> Attached is the updated version of the patch. Comments?
>>>
>>> Personally, I don't like simply documenting this issue. I'd much rather
>>> we restrict the WAL info as it's really got no business being available
>>> to the general public. I'd be fine with restricting that information to
>>> superusers when compression is enabled, or always, for 9.5 and then
>>> fixing it properly in 9.6 by allowing it to be visible to a
>>> "pg_monitoring" default role which admins can then grant to users who
>>> should be able to see it.
>>
>> Well, then you could still launch the same attack if you have the
>> pg_monitoring privileges. So it would be more like a "monitoring and
>> grab everyone's passwords" privilege ;-). Ok, in seriousness the
>> attack isn't that easy to perform, but I still wouldn't feel
>> comfortable giving that privilege to anyone who isn't a superuser
>> anyway.
>
> The alternative is to have monitoring tools which are running as
> superuser, which, in my view at least, is far worse.

Or don't enable fpw_compression for tables where the information leak is
a problem.

>>> Yes, we'll get flak from people who are running with non-superuser
>>> monitoring tools today, but we already have a bunch of superuser-only
>>> things that monioring tools want, so this doesn't move the needle
>>> particularly far, in my view.
>>
>> That would be a major drawback IMHO, and a step in the wrong direction.
>
> I'm not following. If we don't want the information to be available to
> everyone then we need to limit it, and right now the only way to do that
> is to restrict it to superuser because we haven't got anything more
> granular.
>
> In other words, it seems like your above response about not wanting this
> to be available to anyone except superusers is counter to this last
> sentence where you seem to be saying we should continue to have the
> information available to everyone and simply document that there's a
> risk there as in the proposed patch.

I don't think we can or should try to hide the current WAL location. At
least not until we have a monitoring role separate from superuserness.

- Heikki

* Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> On 07/07/2015 04:31 PM, Stephen Frost wrote:
> >The alternative is to have monitoring tools which are running as
> >superuser, which, in my view at least, is far worse.
>
> Or don't enable fpw_compression for tables where the information
> leak is a problem.

My hope would be that we would enable FPW compression by default for
everyone as a nice optimization. Relegating it to a risky option which
has to be tweaked on a per-table basis, but only for those tables where
you don't mind the risk, makes a nice optimization nearly unusable for
many environments.

> >I'm not following. If we don't want the information to be available to
> >everyone then we need to limit it, and right now the only way to do that
> >is to restrict it to superuser because we haven't got anything more
> >granular.
> >
> >In other words, it seems like your above response about not wanting this
> >to be available to anyone except superusers is counter to this last
> >sentence where you seem to be saying we should continue to have the
> >information available to everyone and simply document that there's a
> >risk there as in the proposed patch.
>
> I don't think we can or should try to hide the current WAL location.
> At least not until we have a monitoring role separate from
> superuserness.

I'd rather we hide it now, to allow FPW compression to be enabled for
everyone, except those few environments where it ends up making things
worse, and then provide the monitoring role in 9.6 which addresses this
and various other pieces that are currently superuser-only. We could
wait, but I think we'd end up discouraging people from using the option
because of the caveat and we'd then spend years undoing that in the
future.

Thanks!

Stephen

On Wed, Jul 8, 2015 at 12:34 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> On 07/07/2015 04:31 PM, Stephen Frost wrote:
>> >The alternative is to have monitoring tools which are running as
>> >superuser, which, in my view at least, is far worse.
>>
>> Or don't enable fpw_compression for tables where the information
>> leak is a problem.
>
> My hope would be that we would enable FPW compression by default for
> everyone as a nice optimization. Relegating it to a risky option which
> has to be tweaked on a per-table basis, but only for those tables where
> you don't mind the risk, makes a nice optimization nearly unusable for
> many environments.
>
>> >I'm not following. If we don't want the information to be available to
>> >everyone then we need to limit it, and right now the only way to do that
>> >is to restrict it to superuser because we haven't got anything more
>> >granular.
>> >
>> >In other words, it seems like your above response about not wanting this
>> >to be available to anyone except superusers is counter to this last
>> >sentence where you seem to be saying we should continue to have the
>> >information available to everyone and simply document that there's a
>> >risk there as in the proposed patch.
>>
>> I don't think we can or should try to hide the current WAL location.
>> At least not until we have a monitoring role separate from
>> superuserness.

+1

> I'd rather we hide it now, to allow FPW compression to be enabled for
> everyone, except those few environments where it ends up making things
> worse, and then provide the monitoring role in 9.6 which addresses this
> and various other pieces that are currently superuser-only. We could
> wait, but I think we'd end up discouraging people from using the option
> because of the caveat and we'd then spend years undoing that in the
> future.

So one (ugly) idea is to introduce new setting value like
more_secure_but_might_break_your_monitoring_system (better name? ;))
in wal_compression. If it's specified, literally FPW is compressed and
non-superuser is disallowed to see WAL locaiton. This isn't helpful for
those who need WAL compression but want to allow even non-superuser
to see WAL location, though. Maybe we need to implement also per-table
FPW compression option, to alleviate this situation.

Or another crazy idea is to append "random length" dummy data into
compressed FPW. Which would make it really hard for an attacker to
guess the information from WAL location. Even if this option is enabled,
you can still have the performance improvement by FPW compression
(of course if dummy data is not so big).

Regards,

--
Fujii Masao

On Tue, Jul 7, 2015 at 12:34 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> On 07/07/2015 04:31 PM, Stephen Frost wrote:
>> >The alternative is to have monitoring tools which are running as
>> >superuser, which, in my view at least, is far worse.
>>
>> Or don't enable fpw_compression for tables where the information
>> leak is a problem.
>
> My hope would be that we would enable FPW compression by default for
> everyone as a nice optimization. Relegating it to a risky option which
> has to be tweaked on a per-table basis, but only for those tables where
> you don't mind the risk, makes a nice optimization nearly unusable for
> many environments.

No, only tables that have RLS (or the equivalent, like in the case of
pg_authid), where the leak may be meaningful.

The attack requires control over an adjacent (same page) row, but not
over the row being attacked. That's RLS.

* Claudio Freire (klaussfreire(at)gmail(dot)com) wrote:
> On Tue, Jul 7, 2015 at 12:34 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
> >> On 07/07/2015 04:31 PM, Stephen Frost wrote:
> >> >The alternative is to have monitoring tools which are running as
> >> >superuser, which, in my view at least, is far worse.
> >>
> >> Or don't enable fpw_compression for tables where the information
> >> leak is a problem.
> >
> > My hope would be that we would enable FPW compression by default for
> > everyone as a nice optimization. Relegating it to a risky option which
> > has to be tweaked on a per-table basis, but only for those tables where
> > you don't mind the risk, makes a nice optimization nearly unusable for
> > many environments.
>
> No, only tables that have RLS (or the equivalent, like in the case of
> pg_authid), where the leak may be meaningful.
>
> The attack requires control over an adjacent (same page) row, but not
> over the row being attacked. That's RLS.

Eh? I don't recall Heikki's attack requiring RLS and what about
column-level privilege cases where you have access to the row but not to
one of the columns?

Thanks,

Stephen

On Tue, Jul 7, 2015 at 2:24 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Claudio Freire (klaussfreire(at)gmail(dot)com) wrote:
>> On Tue, Jul 7, 2015 at 12:34 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > * Heikki Linnakangas (hlinnaka(at)iki(dot)fi) wrote:
>> >> On 07/07/2015 04:31 PM, Stephen Frost wrote:
>> >> >The alternative is to have monitoring tools which are running as
>> >> >superuser, which, in my view at least, is far worse.
>> >>
>> >> Or don't enable fpw_compression for tables where the information
>> >> leak is a problem.
>> >
>> > My hope would be that we would enable FPW compression by default for
>> > everyone as a nice optimization. Relegating it to a risky option which
>> > has to be tweaked on a per-table basis, but only for those tables where
>> > you don't mind the risk, makes a nice optimization nearly unusable for
>> > many environments.
>>
>> No, only tables that have RLS (or the equivalent, like in the case of
>> pg_authid), where the leak may be meaningful.
>>
>> The attack requires control over an adjacent (same page) row, but not
>> over the row being attacked. That's RLS.
>
> Eh? I don't recall Heikki's attack requiring RLS and what about
> column-level privilege cases where you have access to the row but not to
> one of the columns?

That's right, column-level too.

Heiki's "change password" step requires something very similar to RLS
where roles can only update their own row. pg_authid also has
column-level stuff, where you only see your own hashed password, and
that too may make the attack useful. In the absence of row or
column-level privileges, however, the attack is unnecessary, and FPW
compression can be applied liberally.

* Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
> On Wed, Jul 8, 2015 at 12:34 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I'd rather we hide it now, to allow FPW compression to be enabled for
> > everyone, except those few environments where it ends up making things
> > worse, and then provide the monitoring role in 9.6 which addresses this
> > and various other pieces that are currently superuser-only. We could
> > wait, but I think we'd end up discouraging people from using the option
> > because of the caveat and we'd then spend years undoing that in the
> > future.
>
> So one (ugly) idea is to introduce new setting value like
> more_secure_but_might_break_your_monitoring_system (better name? ;))
> in wal_compression. If it's specified, literally FPW is compressed and
> non-superuser is disallowed to see WAL locaiton. This isn't helpful for
> those who need WAL compression but want to allow even non-superuser
> to see WAL location, though. Maybe we need to implement also per-table
> FPW compression option, to alleviate this situation.

I'm not thrilled with that idea, but at the same time, we could have a
generic "hide potentially sensitive information" option that applies to
all of these things and then admins could set that on their monitoring
role, to allow it to see the data. That wouldn't get in the way of the
default roles idea either.

> Or another crazy idea is to append "random length" dummy data into
> compressed FPW. Which would make it really hard for an attacker to
> guess the information from WAL location. Even if this option is enabled,
> you can still have the performance improvement by FPW compression
> (of course if dummy data is not so big).

I'm not sure I'd call that "crazy" as it's done in other systems.. This
would also help with cases where an attacker can view the WAL length
through other means, so it has some indepdent advantages.

Curious to hear what others think about that approach though.

Thanks!

Stephen

On 07/07/2015 07:31 PM, Fujii Masao wrote:
> Or another crazy idea is to append "random length" dummy data into
> compressed FPW. Which would make it really hard for an attacker to
> guess the information from WAL location.

It makes the signal more noisy, but you can still mount the same attack
if you just average it over more iterations. You could perhaps fuzz it
enough to make the attack impractical, but it doesn't exactly give me a
warm fuzzy feeling.

- Heikki

On Tue, Jul 7, 2015 at 2:29 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Or another crazy idea is to append "random length" dummy data into
>> compressed FPW. Which would make it really hard for an attacker to
>> guess the information from WAL location. Even if this option is enabled,
>> you can still have the performance improvement by FPW compression
>> (of course if dummy data is not so big).
>
> I'm not sure I'd call that "crazy" as it's done in other systems.. This
> would also help with cases where an attacker can view the WAL length
> through other means, so it has some indepdent advantages.
>
> Curious to hear what others think about that approach though.

It's difficult to know whether the randomization would be effective.

For instance, if one were to use a simple linear congruence generator,
it's possible that the known relationship between the added lengths
allows their effect to be accounted for. The parameters of such RNG
can be leaked by attacking a different table fully under the control
of the attacker, generating WAL with known compression ratios, and
comparing resulting WAL size. IIRC, most non-crypto RNGs can be
similarly attacked.

So it would have to be a cryptographically secure RNG to be safe, and
that would be very costly to run during FPW.

On Wed, Jul 8, 2015 at 2:40 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 07/07/2015 07:31 PM, Fujii Masao wrote:
>>
>> Or another crazy idea is to append "random length" dummy data into
>> compressed FPW. Which would make it really hard for an attacker to
>> guess the information from WAL location.
>
>
> It makes the signal more noisy, but you can still mount the same attack if
> you just average it over more iterations. You could perhaps fuzz it enough
> to make the attack impractical, but it doesn't exactly give me a warm fuzzy
> feeling.

If the attack is impractical, what makes you feel nervous?
Maybe we should be concerned about a brute-force and dictionary
attacks rather than the attack using wal_compression?
Because ISTM that they are more likely to be able to leak passwords
in practice.

Regards,

--
Fujii Masao

On Tue, Jul 7, 2015 at 11:34 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
>> On Tue, Jul 7, 2015 at 10:31 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > I'm not following. If we don't want the information to be available to
>> > everyone then we need to limit it, and right now the only way to do that
>> > is to restrict it to superuser because we haven't got anything more
>> > granular.
>>
>> A user can very easily limit such information by not enabling wal_compression.
>> No need to restrict the usage of WAL location functions.
>> Of course, as Michael suggested, we need to make the parameter SUSET
>> so that only superuser can change the setting, though.
>
> I agree with making it SUSET, but that doesn't address the issue.

ISTM that one our consensus is to make wal_compression SUSET
as the first step whatever approach we adopt for the risk in question
later. So, barring any objection, I will commit the attached patch
and change the context to PGC_SUSET.

Regards,

--
Fujii Masao

On Wed, Jul 8, 2015 at 11:33 AM, Fujii Masao wrote:
> ISTM that one our consensus is to make wal_compression SUSET
> as the first step whatever approach we adopt for the risk in question
> later. So, barring any objection, I will commit the attached patch
> and change the context to PGC_SUSET.

That's surely a step in the good direction. Thanks.
--
Michael

On Thu, Jul 9, 2015 at 10:05 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Jul 8, 2015 at 11:33 AM, Fujii Masao wrote:
>> ISTM that one our consensus is to make wal_compression SUSET
>> as the first step whatever approach we adopt for the risk in question
>> later. So, barring any objection, I will commit the attached patch
>> and change the context to PGC_SUSET.
>
> That's surely a step in the good direction. Thanks.

Yep, pushed!

Regards,

--
Fujii Masao