Thread: minor auth code cleanup
The attached patch makes the following changes:
- add an assertion to check that the size of the packet
specified in the protocol is the same amount of data that we
actually read off the wire.
- fix some bizarre indentation & grammar mistake in the newly
added libpq timeout code. Also make some minor cleanup.
- add a comment noting the intent to replace the fixed size
user name & database name fields in the protocol with
variable width ones.
Unless anyone sees a problem, please apply.
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes:
> ! /*
> ! * We don't actually use the startup packet length the frontend sent
> ! * us; however, it's a reasonable sanity check to ensure that we
> ! * read as much data as we expected to.
> ! *
> ! * The actual startup packet size is the length of the buffer, plus
> ! * the size part of the message (4 bytes), plus a terminator.
> ! */
> ! Assert(len == (buf.len + 4 + 1));
This takes a non-problem and converts it into a problem, no?
There may be existing clients out there that miscompute the password
packet length. Right now that does no harm. With an Assert in place
in the backend, it will cause a database system restart.
Sir Mordred would be quite justified in labeling this a DOS
vulnerability...
On the pqcomm.h comment changes, I would like to see the options field
be variable-length too, with a fairly high upper limit since you might
want to fit several constructs like "-c guc_variable_name=value" in
there. While at it we may as well get rid of the tty field, which is
unused since a long time.
On the subject of the timeout calculations, this code still looks
utterly bizarre:
> ! if (0 > (finish_time.tv_usec -= start_time.tv_usec))
> ! {
> ! remains.tv_sec++;
> ! finish_time.tv_usec += 1000000;
> ! }
> ! if (0 > (remains.tv_usec -= finish_time.tv_usec))
> ! {
> ! remains.tv_sec--;
> ! remains.tv_usec += 1000000;
> ! }
> ! remains.tv_sec -= finish_time.tv_sec - start_time.tv_sec;
It might be correct, I'm not sure; it's definitely going out of its way
to be confusing. A more serious objection is that the code is actively
wrong on systems where tv_sec is unsigned, as for instance HPUX (dunno
whether that's standard or not). If you manage to underflow
remains.tv_sec then you continue to wait forever ... or at least till
the long wraps around again...
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Neil Conway <neilc@samurai.com> writes: > > > ! /* > > ! * We don't actually use the startup packet length the frontend sent > > ! * us; however, it's a reasonable sanity check to ensure that we > > ! * read as much data as we expected to. > > ! * > > ! * The actual startup packet size is the length of the buffer, plus > > ! * the size part of the message (4 bytes), plus a terminator. > > ! */ > > ! Assert(len == (buf.len + 4 + 1)); > > This takes a non-problem and converts it into a problem, no? > > There may be existing clients out there that miscompute the password > packet length. Right now that does no harm. With an Assert in place > in the backend, it will cause a database system restart. Good point. However, I still think a sanity check would be appropriate here. How about an elog(WARNING) ? > On the subject of the timeout calculations, this code still looks > utterly bizarre: [...] Yes, I agree. I tried to improve things a little bit, but there's still some code that I was scratching my head over. If you'd like to take a shot at rewriting it, go ahead; otherwise I might do it eventually... Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
I just cleaned up the connection timeout code a little. --------------------------------------------------------------------------- Neil Conway wrote: > The attached patch makes the following changes: > > - add an assertion to check that the size of the packet > specified in the protocol is the same amount of data that we > actually read off the wire. > > - fix some bizarre indentation & grammar mistake in the newly > added libpq timeout code. Also make some minor cleanup. > > - add a comment noting the intent to replace the fixed size > user name & database name fields in the protocol with > variable width ones. > > Unless anyone sees a problem, please apply. > > Cheers, > > Neil > > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Neil Conway <neilc@samurai.com> writes:
>> There may be existing clients out there that miscompute the password
>> packet length. Right now that does no harm. With an Assert in place
>> in the backend, it will cause a database system restart.
> Good point. However, I still think a sanity check would be appropriate
> here. How about an elog(WARNING) ?
I think that elog(LOG) is probably the right thing. IIRC, at that point
in startup we will not send anything short of ERROR to the client, so
elog(WARNING) is pointless from the client's point of view --- and a LOG
is actually more likely to get into the server's log than a WARNING.
regards, tom lane
OK, I added your comment improvements, where appropriate. I also did a little more cleanup of the connection timeout code. --------------------------------------------------------------------------- Neil Conway wrote: > The attached patch makes the following changes: > > - add an assertion to check that the size of the packet > specified in the protocol is the same amount of data that we > actually read off the wire. > > - fix some bizarre indentation & grammar mistake in the newly > added libpq timeout code. Also make some minor cleanup. > > - add a comment noting the intent to replace the fixed size > user name & database name fields in the protocol with > variable width ones. > > Unless anyone sees a problem, please apply. > > Cheers, > > Neil > > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: > >> There may be existing clients out there that miscompute the password > >> packet length. Right now that does no harm. With an Assert in place > >> in the backend, it will cause a database system restart. > > > Good point. However, I still think a sanity check would be appropriate > > here. How about an elog(WARNING) ? > > I think that elog(LOG) is probably the right thing. IIRC, at that point > in startup we will not send anything short of ERROR to the client, so > elog(WARNING) is pointless from the client's point of view --- and a LOG > is actually more likely to get into the server's log than a WARNING. Agreed. I did not add any of that code. The actual place you were testing was not the startup packet but the password packet. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes: > OK, I added your comment improvements, where appropriate. In the future, can you either apply, reject, or "apply with editorializing", please? Applying some but not all of my changes means I've got to look at diffs and find the stuff you missed or chose not to apply... The attached patch implements the password packet length sanity check (using an elog(LOG) ), as well as includes a few more comment fixes. Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Yea, I usually don't pick through a patch. Patch applied. Thanks. --------------------------------------------------------------------------- Neil Conway wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > OK, I added your comment improvements, where appropriate. > > In the future, can you either apply, reject, or "apply with > editorializing", please? Applying some but not all of my changes means > I've got to look at diffs and find the stuff you missed or chose not > to apply... > > The attached patch implements the password packet length sanity check > (using an elog(LOG) ), as well as includes a few more comment fixes. > > Cheers, > > Neil > > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 3: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073