Обсуждение: Cancel key now ready

I have added code to the postmaster to generate a random cancel key by
calling gettimeofday() on postmaster startup and on the first invocation
of a backend, and merged the micro-seconds of the two times to seed the
random number generator.

I added a PostmasterRandom() function which returns a random that is
XOR'ed with the original random seed, so it it not possible to take a
given cancel key and predict future random keys.

The only way you could do it would be to call random in your backend,
and somehow find the PREVIOUS random value.  You could XOR it with your
cancel key to find the original seed, and then try going forward to
predict the next cancel value.  Seems impossible to me.

This fulfills two goals, to make the random seed truly random, so the
cancel keys are not guess-able, and to make seeing your own cancel key
almost useless in computing other cancel keys.  Not sure if you can
predict forward, but it is probably impossible to predict randoms
backward on any of our supported platforms.

Patch is posted to patches list.

Now I need help in passing the value to the font-end, and having the
front-end pass it to the backend for a cancel.  I do not recommend
passing the pid because I will store the cancel key in the per-backend
structure, so having the pid does not help me find the backend.  Might
as well just scan the table to find the matching cancel key, and kill
that backend.  We will have to store the pid in the structure, but that
is easy to do.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Now I need help in passing the value to the font-end, and having the
> front-end pass it to the backend for a cancel.

I can work on that.  Have you checked the postmaster changes into cvs?

> I do not recommend passing the pid because I will store the cancel key
> in the per-backend structure, so having the pid does not help me find
> the backend.  Might as well just scan the table to find the matching
> cancel key, and kill that backend.  We will have to store the pid in
> the structure, but that is easy to do.

I don't like this.  Backend PIDs are guaranteed unique (among active
backends); cancel keys are not guaranteed unique, unless you took some
special measure to make them so.  So you could hit the wrong backend
if you only compare cancel keys.  Since you must store the PID anyway to
send the signal, you may as well use both to verify that you have found
the right backend.

            regards, tom lane

>
> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > Now I need help in passing the value to the font-end, and having the
> > front-end pass it to the backend for a cancel.
>
> I can work on that.  Have you checked the postmaster changes into cvs?

Always.  You bet.

>
> > I do not recommend passing the pid because I will store the cancel key
> > in the per-backend structure, so having the pid does not help me find
> > the backend.  Might as well just scan the table to find the matching
> > cancel key, and kill that backend.  We will have to store the pid in
> > the structure, but that is easy to do.
>
> I don't like this.  Backend PIDs are guaranteed unique (among active
> backends); cancel keys are not guaranteed unique, unless you took some
> special measure to make them so.  So you could hit the wrong backend
> if you only compare cancel keys.  Since you must store the PID anyway to
> send the signal, you may as well use both to verify that you have found
> the right backend.

OK, sure, pass the pid.  However, one problem is that the postmaster
does not know the pid until after it forks the backend, so if you want
to send the pid with the cancel key, you will have to send the pid from
the backend.

Also, the odds of two backends have the same cancel key, when random
returns a long() is so astonomically small, that I am willing to live
with the risk, to take the advantage of cleaner, more modular code.

Considering the problem of sending the cancel key from the backend and
not the postmaster, I dropped the pid.  Remember, you have to store the
cancel key in the postmaster so sending it to the client at that point
made sense.  Setting the pid after the fork is easy because there is no
communication required.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Bruce Momjian <maillist@candle.pha.pa.us> writes:
> I have added code to the postmaster to generate a random cancel key by
> calling gettimeofday() on postmaster startup and on the first invocation
> of a backend, and merged the micro-seconds of the two times to seed the
> random number generator.

There were several things I didn't like about Bruce's first cut.
His method for generating a random seed in the postmaster is good,
but there are several security holes elsewhere.

> Not sure if you can
> predict forward, but it is probably impossible to predict randoms
> backward on any of our supported platforms.

Actually, it's not that hard.  Nearly all implementations of random()
are basically just
    seed = (seed * a + b) % c;
    return seed;
for some constants a,b,c --- which a potential attacker could easily
find out.  So if the attacker can execute random() in a backend before
it gets used for anything else, he can back up to the last random value
generated in the postmaster.  The most secure way to prevent this is to
re-execute srandom() during the startup of each backend, so that it will
have a random() sequence that's unrelated to the postmaster's.

Also, Bruce was assuming that the random_seed value wouldn't be visible
in a backend ... but the forked-off backend will have a copy just
sitting there, readily accessible if you can figure out its address.
Backend startup should zero out this variable to be on the safe side.

Attached is a patch that fixes these leaks, and does a couple other
things as well:
  * Computes and saves a cancel key for each backend.
  * fflush before forking, to eliminate double-buffering problems
    between postmaster and backends.
  * Go back to two random() calls instead of one to generate random
    salt.  I'm not sure why Bruce changed that, but it looks much
    less secure to me; the one-call way is exposing twice as many
    bits of the current random seed.
  * Fix "ramdom" typo.

Next is to transmit the PID + cancel key to the frontend and modify
libpq's cancel routine to send it back.  I'll work on that later.

            regards, tom lane


*** postmaster.c~    Mon Jun  8 14:10:39 1998
--- postmaster.c    Mon Jun  8 14:44:15 1998
***************
*** 113,118 ****
--- 113,119 ----
  typedef struct bkend
  {
      int            pid;            /* process id of backend */
+     long        cancel_key;        /* cancel key for cancels for this backend */
  } Backend;

  /* list of active backends.  For garbage collection only now. */
***************
*** 198,204 ****
--- 199,212 ----
  static    int            orgsigmask = sigblock(0);
  #endif

+ /*
+  * State for assigning random salts and cancel keys.
+  * Also, the global MyCancelKey passes the cancel key assigned to a given
+  * backend from the postmaster to that backend (via fork).
+  */
+
  static unsigned int random_seed = 0;
+ long MyCancelKey = 0;

  extern char *optarg;
  extern int    optind,
***************
*** 602,618 ****
              return (STATUS_ERROR);
          }

!         if (random_seed == 0)
          {
              gettimeofday(&later, &tz);

              /*
               *    We are not sure how much precision is in tv_usec, so we
!              *    swap the nibbles of 'later' and XOR them with 'now'
               */
              random_seed = now.tv_usec ^
                      ((later.tv_usec << 16) |
!                     ((unsigned int)(later.tv_usec & 0xffff0000) >> 16));
          }

          /*
--- 610,631 ----
              return (STATUS_ERROR);
          }

!         /*
!          * Select a random seed at the time of first receiving a request.
!          */
!         while (random_seed == 0)
          {
              gettimeofday(&later, &tz);

              /*
               *    We are not sure how much precision is in tv_usec, so we
!              *    swap the nibbles of 'later' and XOR them with 'now'.
!              *  On the off chance that the result is 0, we loop until
!              *  it isn't.
               */
              random_seed = now.tv_usec ^
                      ((later.tv_usec << 16) |
!                     ((later.tv_usec >> 16) & 0xffff));
          }

          /*
***************
*** 1075,1080 ****
--- 1088,1101 ----
      }
  #endif

+     /*
+      * Compute the cancel key that will be assigned to this backend.
+      * The backend will have its own copy in the forked-off process'
+      * value of MyCancelKey, so that it can transmit the key to the
+      * frontend.
+      */
+     MyCancelKey = PostmasterRandom();
+
      if (DebugLvl > 2)
      {
          char      **p;
***************
*** 1088,1104 ****
          fprintf(stderr, "-----------------------------------------\n");
      }

      if ((pid = fork()) == 0)
      {  /* child */
          if (DoBackend(port))
          {
              fprintf(stderr, "%s child[%d]: BackendStartup: backend startup failed\n",
!                     progname, pid);
!             /* use _exit to keep from double-flushing stdio */
!              _exit(1);
          }
          else
!             _exit(0);
      }

      /* in parent */
--- 1109,1129 ----
          fprintf(stderr, "-----------------------------------------\n");
      }

+     /* Flush all stdio channels just before fork,
+      * to avoid double-output problems.
+      */
+     fflush(NULL);
+
      if ((pid = fork()) == 0)
      {  /* child */
          if (DoBackend(port))
          {
              fprintf(stderr, "%s child[%d]: BackendStartup: backend startup failed\n",
!                     progname, (int) getpid());
!              exit(1);
          }
          else
!             exit(0);
      }

      /* in parent */
***************
*** 1130,1135 ****
--- 1155,1161 ----
      }

      bn->pid = pid;
+     bn->cancel_key = MyCancelKey;
      DLAddHead(BackendList, DLNewElem(bn));

      ActiveBackends = TRUE;
***************
*** 1192,1197 ****
--- 1218,1225 ----
      char        dbbuf[ARGV_SIZE + 1];
      int            ac = 0;
      int            i;
+     struct timeval now;
+     struct timezone tz;

      /*
       *    Let's clean up ourselves as the postmaster child
***************
*** 1225,1231 ****
      if (NetServer)
          StreamClose(ServerSock_INET);
      StreamClose(ServerSock_UNIX);
!
      /* Now, on to standard postgres stuff */

      MyProcPid = getpid();
--- 1253,1268 ----
      if (NetServer)
          StreamClose(ServerSock_INET);
      StreamClose(ServerSock_UNIX);
!
!     /*
!      * Don't want backend to be able to see the postmaster random number
!      * generator state.  We have to clobber the static random_seed *and*
!      * start a new random sequence in the random() library function.
!      */
!     random_seed = 0;
!     gettimeofday(&now, &tz);
!     srandom(now.tv_usec);
!
      /* Now, on to standard postgres stuff */

      MyProcPid = getpid();
***************
*** 1365,1374 ****
  static void
  RandomSalt(char *salt)
  {
!     long rand = PostmasterRandom();
!
!     *salt = CharRemap(rand % 62);
!     *(salt + 1) = CharRemap(rand / 62);
  }

  /*
--- 1402,1409 ----
  static void
  RandomSalt(char *salt)
  {
!     *salt = CharRemap(PostmasterRandom());
!     *(salt + 1) = CharRemap(PostmasterRandom());
  }

  /*
***************
*** 1387,1391 ****
          initialized = true;
      }

!     return ramdom() ^ random_seed;
  }
--- 1422,1426 ----
          initialized = true;
      }

!     return random() ^ random_seed;
  }

In the current CVS may be a little bug or a big typo. IMHO should line
1390 in postmaster.c return random and not ramdom.

Since the change around 06/04 I have trouble starting the postmaster. I
can only access db's with postgres.

-Egon

On Mon, 8 Jun 1998, Bruce Momjian wrote:

> >
> > Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > > Now I need help in passing the value to the font-end, and having the
> > > front-end pass it to the backend for a cancel.
> >
> > I can work on that.  Have you checked the postmaster changes into cvs?
>
> Always.  You bet.

>
> In the current CVS may be a little bug or a big typo. IMHO should line
> 1390 in postmaster.c return random and not ramdom.
>
> Since the change around 06/04 I have trouble starting the postmaster. I
> can only access db's with postgres.

Fixing now.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

>
> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > I have added code to the postmaster to generate a random cancel key by
> > calling gettimeofday() on postmaster startup and on the first invocation
> > of a backend, and merged the micro-seconds of the two times to seed the
> > random number generator.
>
> There were several things I didn't like about Bruce's first cut.
> His method for generating a random seed in the postmaster is good,
> but there are several security holes elsewhere.
>
> > Not sure if you can
> > predict forward, but it is probably impossible to predict randoms
> > backward on any of our supported platforms.
>
> Actually, it's not that hard.  Nearly all implementations of random()
> are basically just
>     seed = (seed * a + b) % c;
>     return seed;
> for some constants a,b,c --- which a potential attacker could easily
> find out.  So if the attacker can execute random() in a backend before
> it gets used for anything else, he can back up to the last random value
> generated in the postmaster.  The most secure way to prevent this is to
> re-execute srandom() during the startup of each backend, so that it will
> have a random() sequence that's unrelated to the postmaster's.

I thought about this.  I can force a re-seeding of random in the
backend on first use.  Didn't get that far yet.  Could re-seed on every
startup, but again, could be an expensive function.

>
> Also, Bruce was assuming that the random_seed value wouldn't be visible
> in a backend ... but the forked-off backend will have a copy just
> sitting there, readily accessible if you can figure out its address.
> Backend startup should zero out this variable to be on the safe side.

If they have access the backend address space, they can see the entire
postmaster backend structure at time of fork(), so seeing the seed is
meanless.  Basically, for any user who is installing their own functions
or stuff is already able to do more severe damage than just cancel.
They can write directly into the database.


>
> Attached is a patch that fixes these leaks, and does a couple other
> things as well:
>   * Computes and saves a cancel key for each backend.
>   * fflush before forking, to eliminate double-buffering problems
>     between postmaster and backends.

Can you elaborate on what this fixes?

>   * Go back to two random() calls instead of one to generate random
>     salt.  I'm not sure why Bruce changed that, but it looks much
>     less secure to me; the one-call way is exposing twice as many
>     bits of the current random seed.

The code is similar to taking a random() and doing:

    rand % 10

    (rand / 10) % 10

which for a random of 123456 returns 6 and 5.  In the postmaster case
the values are 62 and not 10, but the concept is the same.  No reason to
call random() twice.  May be an expensive function on some platforms.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Bruce Momjian <maillist@candle.pha.pa.us> writes:
> OK, sure, pass the pid.  However, one problem is that the postmaster
> does not know the pid until after it forks the backend, so if you want
> to send the pid with the cancel key, you will have to send the pid from
> the backend.

Ah, I see: you were planning to send the cancel authorization data to
the FE as part of the AuthenticationOK message.  I was assuming it
should be sent by the backend as part of backend startup.
It could be done either way I suppose.  The transmission of the cancel
key to the backend is essentially free (see my recent patch), so really
it boils down to whether we'd rather add version-dependent fields to
AuthenticationOK or just add a whole new message type.

> Also, the odds of two backends have the same cancel key, when random
> returns a long() is so astonomically small, that I am willing to live
> with the risk, to take the advantage of cleaner, more modular code.

It would only take a few more lines to make it safe: generate a key,
check for a duplicate in the list of active backends, repeat if there
is a duplicate.  However I think that using PID+key is better, because
it makes it that much harder for an attacker to guess a valid cancel
request.

            regards, tom lane

Already fixed, compiled and started postmaster I see the following:

marliesle$ postmaster -i &
[1] 22619
marliesle$ No such file or directory
PostmasterMain execv failed on postmaster

[1]+  Exit 1                  postmaster -i &

I'm on debian (bo) and till July 4 it worked everyday :)

-Egon

On Mon, 8 Jun 1998, Bruce Momjian wrote:
> Fixing now.

> It would only take a few more lines to make it safe: generate a key,
> check for a duplicate in the list of active backends, repeat if there
> is a duplicate.  However I think that using PID+key is better, because
> it makes it that much harder for an attacker to guess a valid cancel
> request.

Another way to do it is that when scanning looking for a match of a
cancel key, do not execute the cancel if there is more than one match.
Simple, and we are already scannig the structure.  I see no reason to
scan it at cancel assignment time because the odds are so small.

But the PID is clearly visible to an attacker, so I see no benefit.  If
it can be sent easily, lets do it.  I am not sure where/how to send it,
so do it the way you think is best.  Again, if you send the pid, you
can't have duplicates, you are right.  Also, remember if we send the
cancel and the pid, we have to store each value in every interface.  It
is not just libpq.


--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

>
> Already fixed, compiled and started postmaster I see the following:
>
> marliesle$ postmaster -i &
> [1] 22619
> marliesle$ No such file or directory
> PostmasterMain execv failed on postmaster
>
> [1]+  Exit 1                  postmaster -i &
>
> I'm on debian (bo) and till July 4 it worked everyday :)

OK, this is another issue.  To properly show status on the command line,
I have the postmaster re-exec() itself with at least three args.  For
some reason, on your platform, this is not working.  I have just
committed a patch that shows the file name on exec failure.  Please
re-sync and tell me what it displays.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Sadly to say the same.

-Egon

On Mon, 8 Jun 1998, Bruce Momjian wrote:

> > marliesle$ postmaster -i &
> > [1] 22619
> > marliesle$ No such file or directory
> > PostmasterMain execv failed on postmaster
>
> OK, this is another issue.  To properly show status on the command line,
> I have the postmaster re-exec() itself with at least three args.  For
> some reason, on your platform, this is not working.  I have just
> committed a patch that shows the file name on exec failure.  Please
> re-sync and tell me what it displays.

>
> Sadly to say the same.
>
> -Egon
>
> On Mon, 8 Jun 1998, Bruce Momjian wrote:
>
> > > marliesle$ postmaster -i &
> > > [1] 22619
> > > marliesle$ No such file or directory
                ^^^^^^^^^^^^^^

There should be some more information in the error message at this
point.


--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Bruce Momjian <maillist@candle.pha.pa.us> writes:
> I thought about this.  I can force a re-seeding of random in the
> backend on first use.

No you can't; you might make PostmasterRandom behave that way, but
that doesn't stop an installed function from just calling random()
directly.  You really need to wipe out the state saved by the random
library function.

> Could re-seed on every
> startup, but again, could be an expensive function.

srandom() is generally not much more than a store into a
static variable.  If there's anything expensive about this,
it'd be the gettimeofday() call to produce the new seed value.

> If they have access the backend address space, they can see the entire
> postmaster backend structure at time of fork(), so seeing the seed is
> meanless.

That's a good point --- in particular, they could trace the postmaster
backend-process list to find out everyone else's cancel keys.  This
sort of thing is one of the disadvantages of not using an exec().

What do you think of freeing that process list as part of backend startup?

> Basically, for any user who is installing their own functions
> or stuff is already able to do more severe damage than just cancel.
> They can write directly into the database.

That's certainly true ... but last week we were trying to make the
cancel protocol proof against someone with the ability to spy on
TCP packets in transit (which is not that easy) and now you seem
to be unworried about attacks that only require loading a function
into one of the backends.  I'd prefer to do whatever we can easily
do to defend against that.

>> * fflush before forking, to eliminate double-buffering problems
>> between postmaster and backends.

> Can you elaborate on what this fixes?

I have not seen any failure cases, if that's what you mean; but I
haven't yet done anything with the new no-exec code.  The risk is
that if any data is waiting in a postmaster stdio output buffer,
it will eventually get written twice, once by the postmaster and
once by the backend.  You want to flush it out before forking
to ensure that doesn't happen.  This wasn't an issue before with
the exec-based code, because the child process' copy of the postmaster's
stdio buffers got thrown away when the exec() occurred.  With no
exec, the unflushed buffers are still there and still valid as far
as the stdio library in the child knows.

> The code is similar to taking a random() and doing:
>     rand % 10
>     (rand / 10) % 10
> which for a random of 123456 returns 6 and 5.  In the postmaster case
> the values are 62 and not 10, but the concept is the same.  No reason to
> call random() twice.  May be an expensive function on some platforms.

It's not that expensive (you were doing it twice before, with no visible
problem).  I'm concerned that the new way exposes more info about the
current state of the postmaster's random sequence.  For that matter,
I'm probably going to want to change the computation of the cancel key
later on --- the code I just sent in was only
    MyCancelKey = PostmasterRandom();
but I think it would be better to synthesize the cancel key from
fragments of a couple of random values.  This code will do to get the
protocol working but I don't think it's cryptographically secure.

            regards, tom lane

No, postmaster jumps from main/main.c

    if (len >= 10 && !strcmp(argv[0] + len - 10, "postmaster"))
        exit(PostmasterMain(argc, argv));

to postmaster/postmaster.c

  int
  PostmasterMain(int argc, char *argv[])
  { ..
    if (argc < 4)
    .. /* How did we get here, error! */
       fprintf(stderr, "PostmasterMain execv failed on %s\n", argv[0]);

I tried this today and after the fix the message is the same. Will start a
next time tomorrow.

-Egon

On Mon, 8 Jun 1998, Bruce Momjian wrote:

> >
> > Sadly to say the same.
> >
> > -Egon
> >
> > On Mon, 8 Jun 1998, Bruce Momjian wrote:
> >
> > > > marliesle$ postmaster -i &
> > > > [1] 22619
> > > > marliesle$ No such file or directory
>                 ^^^^^^^^^^^^^^
>
> There should be some more information in the error message at this
> point.

>
> No, postmaster jumps from main/main.c
>
>     if (len >= 10 && !strcmp(argv[0] + len - 10, "postmaster"))
>         exit(PostmasterMain(argc, argv));
>
> to postmaster/postmaster.c
>
>   int
>   PostmasterMain(int argc, char *argv[])
>   { ..
>     if (argc < 4)
>     .. /* How did we get here, error! */
>        fprintf(stderr, "PostmasterMain execv failed on %s\n", argv[0]);
>
> I tried this today and after the fix the message is the same. Will start a
> next time tomorrow.
>
> -Egon

OK, I can recreate it here now.  Fixing now.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

>
> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > I thought about this.  I can force a re-seeding of random in the
> > backend on first use.
>
> No you can't; you might make PostmasterRandom behave that way, but
> that doesn't stop an installed function from just calling random()
> directly.  You really need to wipe out the state saved by the random
> library function.

You can't just call random directly.  You have to call an install
function with a pg_proc entry for it to work.  If we set that one up to
initialize itself, it should work.

>
> > Could re-seed on every
> > startup, but again, could be an expensive function.
>
> srandom() is generally not much more than a store into a
> static variable.  If there's anything expensive about this,
> it'd be the gettimeofday() call to produce the new seed value.

But we don't call that for every backend startup, just twice for the
life of the postmaster, once for postmaster startup, and once for the
startup of the first backend.  What I don't want is to profile the
backend and find that random/srandom() is showing up as significant.

>
> > If they have access the backend address space, they can see the entire
> > postmaster backend structure at time of fork(), so seeing the seed is
> > meanless.
>
> That's a good point --- in particular, they could trace the postmaster
> backend-process list to find out everyone else's cancel keys.  This
> sort of thing is one of the disadvantages of not using an exec().
>
> What do you think of freeing that process list as part of backend startup?

Again, being able to connect to the backend, and accessing its address
space are two separate privs.  Only the postgres user can do such
access.

>
> > Basically, for any user who is installing their own functions
> > or stuff is already able to do more severe damage than just cancel.
> > They can write directly into the database.
>
> That's certainly true ... but last week we were trying to make the
> cancel protocol proof against someone with the ability to spy on
> TCP packets in transit (which is not that easy) and now you seem
> to be unworried about attacks that only require loading a function
> into one of the backends.  I'd prefer to do whatever we can easily
> do to defend against that.

Only the postgres super-user can load functions.  This is something we
have protection against.  Someone snooping the wire may not even have
permissions to access the database.

> p
> >> * fflush before forking, to eliminate double-buffering problems
> >> between postmaster and backends.
>
> > Can you elaborate on what this fixes?
>
> I have not seen any failure cases, if that's what you mean; but I
> haven't yet done anything with the new no-exec code.  The risk is
> that if any data is waiting in a postmaster stdio output buffer,
> it will eventually get written twice, once by the postmaster and
> once by the backend.  You want to flush it out before forking
> to ensure that doesn't happen.  This wasn't an issue before with
> the exec-based code, because the child process' copy of the postmaster's
> stdio buffers got thrown away when the exec() occurred.  With no
> exec, the unflushed buffers are still there and still valid as far
> as the stdio library in the child knows.

Yes.  Excellent point.

>
> > The code is similar to taking a random() and doing:
> >     rand % 10
> >     (rand / 10) % 10
> > which for a random of 123456 returns 6 and 5.  In the postmaster case
> > the values are 62 and not 10, but the concept is the same.  No reason to
> > call random() twice.  May be an expensive function on some platforms.
>
> It's not that expensive (you were doing it twice before, with no visible
> problem).  I'm concerned that the new way exposes more info about the
> current state of the postmaster's random sequence.  For that matter,
> I'm probably going to want to change the computation of the cancel key
> later on --- the code I just sent in was only
>     MyCancelKey = PostmasterRandom();
> but I think it would be better to synthesize the cancel key from
> fragments of a couple of random values.  This code will do to get the
> protocol working but I don't think it's cryptographically secure.

Again, XOR'ing with the seed should do what we need.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

>
> No, postmaster jumps from main/main.c
>
>     if (len >= 10 && !strcmp(argv[0] + len - 10, "postmaster"))
>         exit(PostmasterMain(argc, argv));
>
> to postmaster/postmaster.c
>
>   int
>   PostmasterMain(int argc, char *argv[])
>   { ..
>     if (argc < 4)
>     .. /* How did we get here, error! */
>        fprintf(stderr, "PostmasterMain execv failed on %s\n", argv[0]);
>
> I tried this today and after the fix the message is the same. Will start a
> next time tomorrow.

Fixed.  Thank you for finding this.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Here is a patch that will auto-seed any request for random from the
user.  This will prevent users from seeing random values that use our
postmaster cancel seed.


---------------------------------------------------------------------------

Index: src/backend/utils/adt/misc.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/misc.c,v
retrieving revision 1.12
diff -c -r1.12 misc.c
*** misc.c    1998/02/24 03:47:26    1.12
--- misc.c    1998/06/09 19:16:16
***************
*** 13,18 ****
--- 13,19 ----
   */
  #include <sys/types.h>
  #include <sys/file.h>
+ #include <time.h>
  #include "postgres.h"
  #include "utils/datum.h"
  #include "catalog/pg_type.h"
***************
*** 60,65 ****
--- 61,69 ----
   * will return about 1/10 of the tuples in TEMP
   *
   */
+
+ static bool random_initialized = false;
+
  bool
  oidrand(Oid o, int32 X)
  {
***************
*** 68,73 ****
--- 72,88 ----
      if (X == 0)
          return true;

+     /*
+      *    We do this because the cancel key is actually a random, so we don't
+      *    want them to be able to request random numbers using our postmaster
+      *    seeded value.
+      */
+     if (!random_initialized)
+     {
+         srandom((unsigned int)time(NULL));
+         random_initialized = true;
+     }
+
      result = (random() % X == 0);
      return result;
  }
***************
*** 81,86 ****
--- 96,102 ----
  oidsrand(int32 X)
  {
      srand(X);
+     random_initialized = true;
      return true;
  }


--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)