Обсуждение: Encoding protection for pgcrypto

Hi hackers,

Currently, pgp_sym_decrypt_text and pgp_pub_decrypt_text doesn't
enforce database encoding validation even when returning text. This
patch adds validation and dedicated  tests to verify its
effectiveness. Additionally, some existing unit tests were moved to
the new tests as they failed in some encoding.

Thanks,
SHihao

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hello

I had a look at your patch, which applies fine to PostgreSQL master. I noticed that the new regression tests you have
addedto test utf-8 encoding fails on my setup (make check) with the following diffs:
 

---------------------------------------
@ -13,6 +13,7 @@
 =ivrD
 -----END PGP MESSAGE-----
 '), '0123456789abcdefghij'), 'sha1');
+ERROR:  invalid byte sequence for encoding "UTF8": 0x91
 -- Database encoding protection.  Ciphertext source:
 -- printf '\xe0\xe0\xbff' | gpg --batch --passphrase mykey --textmode --armor --symmetric
 select pgp_sym_decrypt(dearmor('
@@ -23,5 +24,5 @@
 =QKy4
 -----END PGP MESSAGE-----
 '), 'mykey', 'debug=1');
+ERROR:  invalid byte sequence for encoding "UTF8": 0xe0 0xe0 0xbf
 \quit
-\endif
---------------------------------------

I am not sure but it seems that you intentionally provide a text input that would produce a non-utf-8 compliant
decryptedoutput, which triggers the error from within "pg_verifymbstr()" call that you have added in pgp-pgsql.c? Are
theerrors expected in your new test case? If so, then the tests shall pass instead because it has caught a invalid
encodingin decrypted output.
 

Generally, I am ok with the extra encoding check after text decryption but do not think if it is a good idea to just
errorout and abort the transaction when it detects invalid encoding character. text decryption routines may be used
quitefrequently and users generally do not expect them to abort transaction. It may be ok to just give them a warning
aboutinvalid character encoding. 
 

thanks
--------------------
Cary Huang
Highgo Software - Canada
www.highgo.ca

On Fri, Feb 9, 2024 at 5:34 PM cary huang <hcary328@gmail.com> wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, failed
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
>
> Hello
>
> I had a look at your patch, which applies fine to PostgreSQL master. I noticed that the new regression tests you have
addedto test utf-8 encoding fails on my setup (make check) with the following diffs: 
>
> ---------------------------------------
> @ -13,6 +13,7 @@
>  =ivrD
>  -----END PGP MESSAGE-----
>  '), '0123456789abcdefghij'), 'sha1');
> +ERROR:  invalid byte sequence for encoding "UTF8": 0x91
>  -- Database encoding protection.  Ciphertext source:
>  -- printf '\xe0\xe0\xbff' | gpg --batch --passphrase mykey --textmode --armor --symmetric
>  select pgp_sym_decrypt(dearmor('
> @@ -23,5 +24,5 @@
>  =QKy4
>  -----END PGP MESSAGE-----
>  '), 'mykey', 'debug=1');
> +ERROR:  invalid byte sequence for encoding "UTF8": 0xe0 0xe0 0xbf
>  \quit
> -\endif
> ---------------------------------------
>
> I am not sure but it seems that you intentionally provide a text input that would produce a non-utf-8 compliant
decryptedoutput, which triggers the error from within "pg_verifymbstr()" call that you have added in pgp-pgsql.c? Are
theerrors expected in your new test case? If so, then the tests shall pass instead because it has caught a invalid
encodingin decrypted output. 

Thanks for sharing that, I had updated the pgp-decrypt_utf8.out in the
v2.patch which will pass the `make -C contrib/pgcrypto check`.

> Generally, I am ok with the extra encoding check after text decryption but do not think if it is a good idea to just
errorout and abort the transaction when it detects invalid encoding character. text decryption routines may be used
quitefrequently and users generally do not expect them to abort transaction. It may be ok to just give them a warning
aboutinvalid character encoding. 

Thanks for pointing that out. The goal for this patch is to fix the
encoding for the TEXT return value because by default the PostgreSQL
TEXT type should have the same encoding as the database encoding. So I
only added mbverify for the pgp_sym_decrypt_text and
pgp_pub_decrypt_text functions. If customers want to use these two
functions without encoding, they should use pgp_pub_decrypt_bytea and
pgp_sym_decrypt_bytea because BYTEA is represented as a binary string
in PostgreSQL.

Please let me know if you have more questions or concerns. Thanks!

> thanks
> --------------------
> Cary Huang
> Highgo Software - Canada
> www.highgo.ca

On Mon, Feb 12, 2024 at 11:21:41PM -0500, shihao zhong wrote:
> The goal for this patch is to fix the
> encoding for the TEXT return value because by default the PostgreSQL
> TEXT type should have the same encoding as the database encoding.

Pushed as commits d536aee and c5dc754.  A report about a security exploit of
invalid values of type "text", CVE-2026-2006, brought me to this after the
long delay.  After the main fix for CVE-2026-2006, invalid text in a database
is no longer a vulnerability.  Even so, we took the opportunity to adopt
$SUBJECT, too.

Thanks,
nm