Date: Thu, 25 Jun 2020 15:45:41 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: tgl@sss.pgh.pa.us, jeff.janes@gmail.com, frank.gagnepain@intm.fr,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16476: pgp_sym_encrypt_bytea with compress-level=6 : Wrong
 key or corrupt data
Message-ID: <20200625064541.GH130132@paquier.xyz>
References: 
 <CAKPRHzJ-dgYbTfNLgbiMR8tUdjpd9=HTHqAcNvSUd5uJCV6M6Q@mail.gmail.com>
 <20200612.105131.63288858615945654.horikyota.ntt@gmail.com>
 <608105.1591927045@sss.pgh.pa.us>
 <20200612.145412.475791851624925277.horikyota.ntt@gmail.com>
 <20200624075921.GC130132@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Rmm1Stw9KgbdL9/H"
Content-Disposition: inline
In-Reply-To: <20200624075921.GC130132@paquier.xyz>
Precedence: bulk


--Rmm1Stw9KgbdL9/H
Content-Type: multipart/mixed; boundary="HuscSE0D68UGttcd"
Content-Disposition: inline


--HuscSE0D68UGttcd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Jun 24, 2020 at 04:59:21PM +0900, Michael Paquier wrote:
> FYI, I have begun looking at this report, and I am reviewing the
> proposed patch.

Okay.  First I found the test case you added a bit hard to parse, so I
have refactored it with a CTE in charge of building the random string,
with the main query doing the compression/decompression and a check
making sure that the original string and the result match.  I quite
liked the logic with lpad() to append zeros if the computation with
random()*256 returned a result less than 16, as well as the use of
string_agg() for that purpose to build the string.  I have also
switched the second arguments of the functions to just use 'key', for
readability.  Using a random string sounds good to me here.  It could
always be possible that we finish with something less random, causing
it to to become compressed but I'll believe in the rule of chaos
here.

Then, the fix you are proposing is to simply make sure that all the
input from the source stream is properly consumed even after the zlib
stream has ended in this corner case thanks to pullf_read(), and that
sounds good to me.  However, I had an idea slightly different than
yours, consisting of simply reading the contents of the source before
checking if there is any available in the decompressed buffer (the
check on buf_data before the goto restart step).  That makes the fix a
bit simpler, without changing the logic.

I am also attaching an extra script I used to validate this stuff
based on the regression test of the patch, that has allowed me to
check the logic for random strings up to 33kB, like that for example
(this one took 8 mins on my laptop):
select count(test_crypto(len)) from generate_series(0, 33000) as len;

The inputs and outputs perfectly matched in all my tests, with the
16kB string being the only one failing in the range I have tested on
HEAD, test passing with the patch of course.  One or more extra pairs
of eyes is welcome, so please feel free to look at the version
attached.

Thanks,
--
Michael

--HuscSE0D68UGttcd
Content-Type: application/x-sql
Content-Disposition: attachment; filename="crypto.sql"
Content-Transfer-Encoding: quoted-printable

=0ACREATE FUNCTION test_crypto(maxlen int) RETURNS bool AS $$=0AWITH random=
_string AS=0A(=0A  -- This generates a random string of 16366 bytes.  This =
is chosen=0A  -- as random to not make it compressible, so as the decompres=
sion=0A  -- would work on a string with the same length, making the test=0A=
  -- predictible.  lpad() ensures that the generated hexadecimal value=0A  =
-- is completed by extra characters if random() has generated a value=0A  -=
- strictly lower than 16.=0A  SELECT string_agg(decode(lpad(to_hex((random(=
)*256)::int), 2, '0'), 'hex'), '') as bytes=0A    FROM generate_series(0, $=
1)=0A)=0ASELECT bytes =3D=0A    pgp_sym_decrypt_bytea(=0A      pgp_sym_encr=
ypt_bytea(bytes, 'key',=0A                            'compress-algo=3D1,co=
mpress-level=3D1'),=0A                            'key', 'expect-compress-a=
lgo=3D1')=0A    AS is_same=0A  FROM random_string;=0A$$ LANGUAGE SQL STRICT=
;=0A
--HuscSE0D68UGttcd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="pgcrypto-zlib-v4.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/pgcrypto/expected/pgp-compression.out b/contrib/pgcryp=
to/expected/pgp-compression.out
index 32b350b8fe..d4c57feba3 100644
--- a/contrib/pgcrypto/expected/pgp-compression.out
+++ b/contrib/pgcrypto/expected/pgp-compression.out
@@ -48,3 +48,33 @@ select pgp_sym_decrypt(
  Secret message
 (1 row)
=20
+-- check corner case involving an input string of 16kB, as per bug #16476.
+SELECT setseed(0);
+ setseed=20
+---------
+=20
+(1 row)
+
+WITH random_string AS
+(
+  -- This generates a random string of 16366 bytes.  This is chosen
+  -- as random so that it does not get compressed, and the decompression
+  -- would work on a string with the same length as the origin, making the
+  -- test behavior more predictible.  lpad() ensures that the generated
+  -- hexadecimal value is completed by extra zero characters if random()
+  -- has generated a value strictly lower than 16.
+  SELECT string_agg(decode(lpad(to_hex((random()*256)::int), 2, '0'), 'hex=
'), '') as bytes
+    FROM generate_series(0, 16365)
+)
+SELECT bytes =3D
+    pgp_sym_decrypt_bytea(
+      pgp_sym_encrypt_bytea(bytes, 'key',
+                            'compress-algo=3D1,compress-level=3D1'),
+                            'key', 'expect-compress-algo=3D1')
+    AS is_same
+  FROM random_string;
+ is_same=20
+---------
+ t
+(1 row)
+
diff --git a/contrib/pgcrypto/pgp-compress.c b/contrib/pgcrypto/pgp-compres=
s.c
index 0505bdee92..a37595dcc9 100644
--- a/contrib/pgcrypto/pgp-compress.c
+++ b/contrib/pgcrypto/pgp-compress.c
@@ -243,6 +243,20 @@ decompress_read(void *priv, PullFilter *src, int len,
 	struct DecomprData *dec =3D priv;
=20
 restart:
+	if (dec->stream.avail_in =3D=3D 0)
+	{
+		uint8	   *tmp;
+
+		res =3D pullf_read(src, 8192, &tmp);
+		if (res < 0)
+			return res;
+
+		Assert(res !=3D Z_STREAM_END || dec->buf_data > 0);
+
+		dec->stream.next_in =3D tmp;
+		dec->stream.avail_in =3D res;
+	}
+
 	if (dec->buf_data > 0)
 	{
 		if (len > dec->buf_data)
@@ -256,17 +270,6 @@ restart:
 	if (dec->eof)
 		return 0;
=20
-	if (dec->stream.avail_in =3D=3D 0)
-	{
-		uint8	   *tmp;
-
-		res =3D pullf_read(src, 8192, &tmp);
-		if (res < 0)
-			return res;
-		dec->stream.next_in =3D tmp;
-		dec->stream.avail_in =3D res;
-	}
-
 	dec->stream.next_out =3D dec->buf;
 	dec->stream.avail_out =3D dec->buf_len;
 	dec->pos =3D dec->buf;
diff --git a/contrib/pgcrypto/sql/pgp-compression.sql b/contrib/pgcrypto/sq=
l/pgp-compression.sql
index ca9ee1fc00..87c59c6cab 100644
--- a/contrib/pgcrypto/sql/pgp-compression.sql
+++ b/contrib/pgcrypto/sql/pgp-compression.sql
@@ -28,3 +28,24 @@ select pgp_sym_decrypt(
 	pgp_sym_encrypt('Secret message', 'key',
 			'compress-algo=3D2, compress-level=3D0'),
 	'key', 'expect-compress-algo=3D0');
+
+-- check corner case involving an input string of 16kB, as per bug #16476.
+SELECT setseed(0);
+WITH random_string AS
+(
+  -- This generates a random string of 16366 bytes.  This is chosen
+  -- as random so that it does not get compressed, and the decompression
+  -- would work on a string with the same length as the origin, making the
+  -- test behavior more predictible.  lpad() ensures that the generated
+  -- hexadecimal value is completed by extra zero characters if random()
+  -- has generated a value strictly lower than 16.
+  SELECT string_agg(decode(lpad(to_hex((random()*256)::int), 2, '0'), 'hex=
'), '') as bytes
+    FROM generate_series(0, 16365)
+)
+SELECT bytes =3D
+    pgp_sym_decrypt_bytea(
+      pgp_sym_encrypt_bytea(bytes, 'key',
+                            'compress-algo=3D1,compress-level=3D1'),
+                            'key', 'expect-compress-algo=3D1')
+    AS is_same
+  FROM random_string;

--HuscSE0D68UGttcd--

--Rmm1Stw9KgbdL9/H
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl70SBUACgkQnvQgOdby
QH3rwBAAkeyGQxtbaBhRxONzb7aTr5fkePTFOdpENLHspMjsxM2TJgTmpQexfvAC
0dDd8XtcpgiVIdGlBbjJdxGm7TU/po9W5ov6U9jd+4tnJ3+y3IpVPh2Jj/bDVTi8
TUxXznHayQoaKo/XygqCqZdKgUom+WzpgogFuuSYL7/FEKV8Nhsv8qv+cYW2Jlwh
61CJj76BpIx0E7js1ym94aX+WbJnbg3lWMcfnEYsFvTB4/bp2656SERV32ADibKo
KBHxCA26I6x3ZtQYJ3we1eaKD81g4mvSTtApDW9e23z0dm1W/xNHRNGQilCy8iuM
hD/LbHseofGrKec/Sm6AqNT1MiL8tP5/YOvHmoOlipFNtHisWH+NwqyI0l1a4dgT
0lqALHbgGMWuGXal+N9/y7mEuOPELxx6wvFOBY307VDXb9P/x7hlZY6CiVBpzz60
xgMS+qMejXH9NE668L8YuSEkeK9YNbaf1M4TZvcVHv7lS6YDcYIWDkDKEmoT8HEk
NIVpDMCV/ZTZ4PkpglxZbngM1Hiha5nXudiI5tv3BoFcfqfgxIqE5p+L7KhCGGIo
BxLK/tPXxluxG0uAmYHU7kZvng/95ub4eUaS0/fdwt1nwpeM6xUjbQgLwZipbYVf
dsvAKYIddUmCqt6Q5mfAdknj+htU0RgBZKbKm2eiN6HsOyLzVgc=
=5ZfF
-----END PGP SIGNATURE-----

--Rmm1Stw9KgbdL9/H--