Thread: [DOCS] gen_random_uuid security not explicit in documentation
The following documentation comment has been logged on the website: Page: https://www.postgresql.org/docs/9.6/static/pgcrypto.html Description: Hello, The C source code of gen_random_uuid reads: /* * Generate random bits. pg_backend_random() will do here, we don't * promis UUIDs to be cryptographically random, when built with * --disable-strong-random. */ However, the pgcrypto documentation does not mention --disable-strong-random at all. I think the documentation should mention under which conditions the function returns secure data. Radek Slupik P.S. there is also a typo in the C comment: "promis" should be "promise".
(Adding Heikki in CC who committed this code) On Mon, Jan 2, 2017 at 8:20 AM, <rightfold@gmail.com> wrote: > The C source code of gen_random_uuid reads: > > /* > * Generate random bits. pg_backend_random() will do here, we don't > * promis UUIDs to be cryptographically random, when built with > * --disable-strong-random. > */ > > However, the pgcrypto documentation does not mention > --disable-strong-random > at all. I think the documentation should mention under which conditions > the function returns secure data. That's actually a good idea. But as it does not only apply to get_random_uuid(), I would think that a notice at the top of the pgcrypto documentation would make the most sense. Something like: "If PostgreSQL is built with --disable-strong-random, the data generated by the functions is not guaranteed to be cryptographically random." > P.S. there is also a typo in the C comment: "promis" should be "promise". Indeed. -- Michael
On 01/03/2017 02:47 PM, Michael Paquier wrote: > (Adding Heikki in CC who committed this code) > > On Mon, Jan 2, 2017 at 8:20 AM, <rightfold@gmail.com> wrote: >> The C source code of gen_random_uuid reads: >> >> /* >> * Generate random bits. pg_backend_random() will do here, we don't >> * promis UUIDs to be cryptographically random, when built with >> * --disable-strong-random. >> */ >> >> However, the pgcrypto documentation does not mention >> --disable-strong-random >> at all. I think the documentation should mention under which conditions >> the function returns secure data. > > That's actually a good idea. But as it does not only apply to > get_random_uuid(), I would think that a notice at the top of the > pgcrypto documentation would make the most sense. Something like: > "If PostgreSQL is built with --disable-strong-random, the data > generated by the functions is not guaranteed to be cryptographically > random." Hmm, not sure what to do here. --disable-strong-random is similar to e.g. --disable-spinlocks; no reasonable production platform would use it. So I'm not inclined to sprinkle references to it across the docs, it seems better to document what it changes, in the description of --disable-strong-random itself. To be pedantic, the documentation only claims that gen_random_bytes() returns cryptographically strong values. For gen_random_uuid(), it just says that it's "random". But yeah, it's subtle. By the feat of having them side-by-side, and a similar name, you'd think that they behave the same. And with --enable-strong-random, they do. I'm inclined to change gen_random_uuid() to throw an error if the server is built with --disable-strong-random, like gen_random_bytes() does. That way, they would behave the same. Thoughts? - Heikki
On Fri, Jun 23, 2017 at 3:02 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> I'm inclined to change gen_random_uuid() to throw an error if the server is
> built with --disable-strong-random, like gen_random_bytes() does. That way,
> they would behave the same.
No objections to do that. I guess you don't need a patch. As this is
new to 10, I have added an open item.
> Thoughts?
There is this comment in pgcrypto.c with a typo:
* Generate random bits. pg_backend_random() will do here, we don't promis
s/promis/promise/.
--
Michael
On Fri, Jun 23, 2017 at 10:23:36AM +0900, Michael Paquier wrote: > On Fri, Jun 23, 2017 at 3:02 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > I'm inclined to change gen_random_uuid() to throw an error if the server is > > built with --disable-strong-random, like gen_random_bytes() does. That way, > > they would behave the same. > > No objections to do that. I guess you don't need a patch. As this is > new to 10, I have added an open item. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Heikki, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Sun, Jun 25, 2017 at 09:26:28PM -0700, Noah Misch wrote: > On Fri, Jun 23, 2017 at 10:23:36AM +0900, Michael Paquier wrote: > > On Fri, Jun 23, 2017 at 3:02 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > > I'm inclined to change gen_random_uuid() to throw an error if the server is > > > built with --disable-strong-random, like gen_random_bytes() does. That way, > > > they would behave the same. > > > > No objections to do that. I guess you don't need a patch. As this is > > new to 10, I have added an open item. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Heikki, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 30 June 2017 06:45:04 EEST, Noah Misch <noah@leadboat.com> wrote: >This PostgreSQL 10 open item is past due for your status update. >Kindly send >a status update within 24 hours, and include a date for your subsequent >status >update. I'll fix this some time next week. (I'm on vacation right now) - Heikki
On 06/23/2017 04:23 AM, Michael Paquier wrote: > On Fri, Jun 23, 2017 at 3:02 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> I'm inclined to change gen_random_uuid() to throw an error if the server is >> built with --disable-strong-random, like gen_random_bytes() does. That way, >> they would behave the same. > > No objections to do that. I guess you don't need a patch. As this is > new to 10, I have added an open item. Ok, pushed a fix to do that. Thanks for bringing this up, Radek! - Heikki