Thread: Should rolpassword be toastable?
Hello hackers,
When playing with oversized tuples, I've found that it's possible to set
such oversized password for a user, that could not be validated.
For example:
SELECT format('CREATE ROLE test_user LOGIN PASSWORD ''SCRAM-SHA-256$' || repeat('0', 2000000) ||
'4096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM='';')
\gexec
-- the password is "pass"
(One can achieve the same result with a large salt size, for example, 2048.)
psql -U "test_user" -c "SELECT 1"
psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: cannot read pg_class without having
selected a database
I've tried to set attstorage = 'p' for the rolpassword attribute forcefully
by dirty hacking genbki.pl, and as a result I get an error on CREATE ROLE:
ERROR: row is too big: size 2000256, maximum size 8160
Best regards,
Alexander
Alexander Lakhin <exclusion@gmail.com> writes:
> When playing with oversized tuples, I've found that it's possible to set
> such oversized password for a user, that could not be validated.
> For example:
> ...
> psql -U "test_user" -c "SELECT 1"
> psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: cannot read pg_class without having
> selected a database
My inclination is to fix this by removing pg_authid's toast table.
I was not in favor of "let's attach a toast table to every catalog"
to begin with, and I think this failure graphically illustrates
why that was not as great an idea as some people thought.
I also don't think it's worth trying to make it really work.
I'm also now more than just slightly skeptical about whether
pg_database should have a toast table. Has anybody tried,
say, storing a daticurules field wide enough to end up
out-of-line?
regards, tom lane
23.09.2023 17:39, Tom Lane wrote: > I'm also now more than just slightly skeptical about whether > pg_database should have a toast table. Has anybody tried, > say, storing a daticurules field wide enough to end up > out-of-line? I tried, but failed, because pg_database accessed in InitPostgres() before assigning MyDatabaseId only via the function GetDatabaseTupleByOid(), which doesn't unpack the database tuple. Another access to a system catalog with unassigned MyDatabaseId might occur in the has_privs_of_role() call, but pg_auth_members contains no toastable attributes. So for now only pg_authid is worthy of condemnation, AFAICS. Best regards, Alexander
Hello Tom and Nathan, 23.09.2023 21:00, Alexander Lakhin wrote: > 23.09.2023 17:39, Tom Lane wrote: >> I'm also now more than just slightly skeptical about whether >> pg_database should have a toast table. Has anybody tried, >> say, storing a daticurules field wide enough to end up >> out-of-line? > > So for now only pg_authid is worthy of condemnation, AFAICS. > Let me remind you of this issue in light of b52c4fc3c. Yes, it's opposite, but maybe it makes sense to fix it now in the hope that ~1 year of testing will bring something helpful for both changes. Best regards, Alexander
On Thu, Sep 19, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote: > 23.09.2023 21:00, Alexander Lakhin wrote: >> So for now only pg_authid is worthy of condemnation, AFAICS. > > Let me remind you of this issue in light of b52c4fc3c. > Yes, it's opposite, but maybe it makes sense to fix it now in the hope that > ~1 year of testing will bring something helpful for both changes. Hm. It does seem like there's little point in giving pg_authid a TOAST table, as rolpassword is the only varlena column, and it obviously has problems. But wouldn't removing it just trade one unhelpful internal error when trying to log in for another when trying to add a really long password hash (which hopefully nobody is really trying to do in practice)? I wonder if we could make this a little more user-friendly. -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> Hm. It does seem like there's little point in giving pg_authid a TOAST
> table, as rolpassword is the only varlena column, and it obviously has
> problems. But wouldn't removing it just trade one unhelpful internal error
> when trying to log in for another when trying to add a really long password
> hash (which hopefully nobody is really trying to do in practice)? I wonder
> if we could make this a little more user-friendly.
We could put an arbitrary limit (say, half of BLCKSZ) on the length of
passwords.
regards, tom lane
On Thu, Sep 19, 2024 at 10:31:15AM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> Hm. It does seem like there's little point in giving pg_authid a TOAST >> table, as rolpassword is the only varlena column, and it obviously has >> problems. But wouldn't removing it just trade one unhelpful internal error >> when trying to log in for another when trying to add a really long password >> hash (which hopefully nobody is really trying to do in practice)? I wonder >> if we could make this a little more user-friendly. > > We could put an arbitrary limit (say, half of BLCKSZ) on the length of > passwords. Something like that could be good enough. I was thinking about actually validating that the hash had the correct form, but that might be a little more complex than is warranted here. -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> Oh, actually, I see that we are already validating the hash, but you can
> create valid SCRAM-SHA-256 hashes that are really long. So putting an
> arbitrary limit (patch attached) is probably the correct path forward. I'd
> also remove pg_authid's TOAST table while at it.
Shouldn't we enforce the limit in every case in encrypt_password,
not just this one? (I do agree that encrypt_password is an okay
place to enforce it.)
I think you will get pushback from a limit of 256 bytes --- I seem
to recall discussion of actual use-cases where people were using
strings of a couple of kB. Whatever the limit is, the error message
had better cite it explicitly.
Also, the ereport call needs an errcode.
ERRCODE_PROGRAM_LIMIT_EXCEEDED is probably suitable.
regards, tom lane
Nathan Bossart <nathandbossart@gmail.com> writes:
> Here is a v3 patch set that fixes the test comment and a compiler warning
> in cfbot.
Nitpick: the message should say "%d bytes" not "%d characters",
because we're counting bytes. Passes an eyeball check otherwise.
regards, tom lane
On Sat, Sep 21, 2024 at 03:25:54PM -0500, Nathan Bossart wrote: > Thanks for reviewing. I went ahead and committed 0002 since it seems like > there's consensus on that one. I've attached a rebased version of 0001 > with s/characters/bytes. For the reasons discussed upthread [0], I can't bring myself to add an arbitrary limit to the password hash length. I am going to leave 0001 uncommitted for now. [0] https://postgr.es/m/Zu2eT2H8OT3OXauc%40nathan -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes: > For the reasons discussed upthread [0], I can't bring myself to add an > arbitrary limit to the password hash length. I am going to leave 0001 > uncommitted for now. > [0] https://postgr.es/m/Zu2eT2H8OT3OXauc%40nathan I'm confused, as in [0] you said >> ... But on the off-chance >> that someone is building a custom driver that generates long hashes for >> whatever reason, I'd imagine that a clear error would be more helpful than >> "row is too big." I agree with the idea that complaining about the password being too long is far more intelligible than that. Another problem with leaving it as it stands in HEAD is that the effective limit is now platform-specific, if not indeed dependent on the phase of the moon (or at least, the other contents of the pg_authid row). I fear it would be very easy to construct cases where a password is accepted on one machine but fails with "row is too big" on another. A uniform limit seems much less fraught with surprises. regards, tom lane
On Thu, Oct 03, 2024 at 05:39:06PM -0400, Tom Lane wrote: > I agree with the idea that complaining about the password being too > long is far more intelligible than that. Another problem with > leaving it as it stands in HEAD is that the effective limit is now > platform-specific, if not indeed dependent on the phase of the moon > (or at least, the other contents of the pg_authid row). I fear it > would be very easy to construct cases where a password is accepted > on one machine but fails with "row is too big" on another. A > uniform limit seems much less fraught with surprises. I don't mind proceeding with the patch if there is strong support for it. I wavered only because it's hard to be confident that we are choosing the right limit. AFAICT 256 bytes ought to be sufficient to avoid "row is too big" errors independent of BLCKSZ today, but maybe someone will add another varlena column in the future that breaks it. Or maybe we add a new password hashing method that produces longer strings. Or maybe someone is doing something really out there like storing additional information in the salt. I don't have any reason to believe that any of these things are happening or are likely to happen anytime soon, but they seem similar in likelihood to someone building a custom driver that generates ginormous hashes. But I can also buy the argument that none of this is a strong enough reason to avoid making the error message nicer... -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> I don't mind proceeding with the patch if there is strong support for it.
> I wavered only because it's hard to be confident that we are choosing the
> right limit.
I'm not that fussed about it; surely 256 is more than anyone is using?
If not, we'll get push-back and then we can have a discussion about the
correct limit that's informed by more than guesswork.
> ... But I can also buy the argument that none of this is a strong
> enough reason to avoid making the error message nicer...
There's that, and there's also the fact that if you assume someone is
using $sufficiently-long-passwords then we might have broken their
use-case already. We can't have much of a conversation here without
a concrete case to look at.
regards, tom lane
On Thu, Oct 3, 2024 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Nathan Bossart <nathandbossart@gmail.com> writes: > > I don't mind proceeding with the patch if there is strong support for it. > > I wavered only because it's hard to be confident that we are choosing the > > right limit. > > I'm not that fussed about it; surely 256 is more than anyone is using? > If not, we'll get push-back and then we can have a discussion about the > correct limit that's informed by more than guesswork. +1. Next up is probably SCRAM-SHA-512, which should still have smaller entries than that -- 222 bytes, I think, with 128-bit salts and a 5-digit iteration count? --Jacob
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> I think Tom's initial suggestion (BLCKSZ/2) is better than 256, given we
> really don't know what' out there in the wild, and this could end up
> being a breaking change. Every other type in pg_authid is pretty small.
I'm having second thoughts about that though, based on the argument
that we don't really want a platform-dependent limit here.
Admittedly, nobody changes BLCKSZ on production systems, but it's
still theoretically an issue. I don't have a problem with selecting
a larger limit such as 512 or 1024 though.
> That said, I'm also imagining other things we may add that could require
> TOAST support (remembering previous passwords? storing multiple
> passwords options)?
Things like previous passwords probably don't need to be accessed
during authentication, so there are at least a couple of ways we
could do that:
* put the previous passwords in an auxiliary table;
* put back pg_authid's toast table, but mark rolpassword as
"STORAGE MAIN" so it doesn't go to toast, while letting columns
that don't need to be touched at startup go there.
However, if you wanted to allow multiple passwords I'm not
sure about a good way.
regards, tom lane
On Thu, Oct 03, 2024 at 10:33:04PM -0400, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> I think Tom's initial suggestion (BLCKSZ/2) is better than 256, given we >> really don't know what' out there in the wild, and this could end up >> being a breaking change. Every other type in pg_authid is pretty small. > > I'm having second thoughts about that though, based on the argument > that we don't really want a platform-dependent limit here. > Admittedly, nobody changes BLCKSZ on production systems, but it's > still theoretically an issue. I don't have a problem with selecting > a larger limit such as 512 or 1024 though. Since BLCKSZ can be as low as 1024, I think 512 would be a good choice. > However, if you wanted to allow multiple passwords I'm not > sure about a good way. The most recent proposal I'm aware of [0] did seem to target that use-case. One option might be to move rolpassword to a different catalog. In any case, I don't think it matters much for the patch at hand. [0] https://postgr.es/m/CAGB%2BVh5SQQorNDEKP%2B0G%3DsmxHRhbhs%2BVkmQWD5Vh98fmn8X4dg%40mail.gmail.com -- nathan
On Fri, Oct 4, 2024 at 4:48 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > .. > Since BLCKSZ can be as low as 1024, I think 512 would be a good choice. > Where did you get the minimal value of 1024 from ? I vaguely remember someone testing with 256 at some point in the past --- Hannu
On Sun, Oct 06, 2024 at 11:42:53AM +0200, Hannu Krosing wrote:
> On Fri, Oct 4, 2024 at 4:48 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Since BLCKSZ can be as low as 1024, I think 512 would be a good choice.
>
> Where did you get the minimal value of 1024 from ?
https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-OPTION-WITH-BLOCKSIZE
--
nathan
Committed with the limit set to 512 bytes. We have plenty of time to adjust this limit as needed before it takes effect in v18. -- nathan