Обсуждение: [PATCH] OpenSSL: Mark underlying BIO with the appropriate type flags
Hi Everyone,
I’ve ran into an issue where the OpenSSL API function “ssl_get_fd” fails, due to the underlying BIO object created by Postgres is not being flagged properly.
Previous to OpenSSL version 1.1.0, the BIO methods object would be copied directly from the existing socket type and then its read\write functions would be replaced.
With 1.1.0 and up, the object is created from scratch and then all its methods are initialized to be the ones of the socket type, except read/write which are custom.
In this newer way, a new type is given to it by calling “BIO_get_new_index”, but the related type flags aren’t added.
For more information please see: https://www.openssl.org/docs/man1.1.0/man3/BIO_get_new_index.html
In this case, the type should have both BIO_TYPE_SOURCE_SINK and BIO_TYPE_DESCRIPTOR.
The proposed patch will add these flags to the BIO type on creation. I have compiled it with OpenSSL, enabled encryption and verified that basic queries work fine.
I don’t believe this affects Postgres, since the code has been this way for 5 years. I ran into it because I’m writing auditing code that hooks on Postgres calls. I’ve already found a workaround by adding these flags myself with an additional hook, but thought it would be worth bringing up here and see if you think it’s worth patching.
Regards,
Itamar Gafni
Imperva
NOTICE:
This email and all attachments are confidential, may be proprietary, and may be privileged or otherwise protected from disclosure. They are intended solely for the individual or entity to whom the email is addressed. However, mistakes sometimes happen in addressing emails. If you believe that you are not an intended recipient, please stop reading immediately. Do not copy, forward, or rely on the contents in any way. Notify the sender and/or Imperva, Inc. by telephone at +1 (650) 832-6006 and then delete or destroy any copy of this email and its attachments. The sender reserves and asserts all rights to confidentiality, as well as any privileges that may apply. Any disclosure, copying, distribution or action taken or omitted to be taken by an unintended recipient in reliance on this message is prohibited and may be unlawful.
Please consider the environment before printing this email.
Вложения
> On 6 Aug 2021, at 12:16, Itamar Gafni <itamar.gafni@imperva.com> wrote: > Previous to OpenSSL version 1.1.0, the BIO methods object would be copied directly from the existing socket type and thenits read\write functions would be replaced. > With 1.1.0 and up, the object is created from scratch and then all its methods are initialized to be the ones of the sockettype, except read/write which are custom. > In this newer way, a new type is given to it by calling “BIO_get_new_index”, but the related type flags aren’t added. According to the documentation (I haven't tested it yet but will) I think you are right that the type should be set with the appropriate BIO_TYPE_ flags. For OpenSSL 1.0.1 and 1.0.2, wouldn't we need to set the .type with a randomly chosen index anded with BIO_TYPE_DESCRIPTOR and BIO_TYPE_SOURCE_SINK as well? -- Daniel Gustafsson https://vmware.com/
Not sure what is the expected use with previous versions. It used to be that the BIO_s_socket() would return a non-constpointer, so the original socket methods object could be edited. Copying it means there are two BIO_METHOD objects with the same type (ortig socket and its copy) but it's unclear if that'san issue. Itamar Gafni Imperva -----Original Message----- From: Daniel Gustafsson <daniel@yesql.se> Sent: Monday, 9 August 2021 18:36 To: Itamar Gafni <itamar.gafni@imperva.com> Cc: pgsql-hackers@lists.postgresql.org Subject: Re: [PATCH] OpenSSL: Mark underlying BIO with the appropriate type flags CAUTION: This message was sent from outside the company. Do not click links or open attachments unless you recognize thesender and know the content is safe. > On 6 Aug 2021, at 12:16, Itamar Gafni <itamar.gafni@imperva.com> wrote: > Previous to OpenSSL version 1.1.0, the BIO methods object would be copied directly from the existing socket type and thenits read\write functions would be replaced. > With 1.1.0 and up, the object is created from scratch and then all its methods are initialized to be the ones of the sockettype, except read/write which are custom. > In this newer way, a new type is given to it by calling “BIO_get_new_index”, but the related type flags aren’t added. According to the documentation (I haven't tested it yet but will) I think you are right that the type should be set withthe appropriate BIO_TYPE_ flags. For OpenSSL 1.0.1 and 1.0.2, wouldn't we need to set the .type with a randomly chosen index anded with BIO_TYPE_DESCRIPTORand BIO_TYPE_SOURCE_SINK as well? -- Daniel Gustafsson https://vmware.com/ ------------------------------------------- NOTICE: This email and all attachments are confidential, may be proprietary, and may be privileged or otherwise protected from disclosure.They are intended solely for the individual or entity to whom the email is addressed. However, mistakes sometimeshappen in addressing emails. If you believe that you are not an intended recipient, please stop reading immediately.Do not copy, forward, or rely on the contents in any way. Notify the sender and/or Imperva, Inc. by telephoneat +1 (650) 832-6006 and then delete or destroy any copy of this email and its attachments. The sender reservesand asserts all rights to confidentiality, as well as any privileges that may apply. Any disclosure, copying, distributionor action taken or omitted to be taken by an unintended recipient in reliance on this message is prohibited andmay be unlawful. Please consider the environment before printing this email.
> On 12 Aug 2021, at 12:00, Itamar Gafni <itamar.gafni@imperva.com> wrote: > Copying it means there are two BIO_METHOD objects with the same type (ortig > socket and its copy) but it's unclear if that's an issue. Reading code there doesn't seem to be a concensus really, Apple is doing it in the Swift TLS code but otheres aren't. Considering there's been no complaints on this and OpenSSL < 1.1.0 is EOL I say we leave it for now. Barring objections I will go ahead with your proposed patch on HEAD and backpatch it all the way, once I've done more testing on it. -- Daniel Gustafsson https://vmware.com/
> On 12 Aug 2021, at 15:32, Daniel Gustafsson <daniel@yesql.se> wrote: > Barring objections I will go ahead with your proposed patch on HEAD and > backpatch it all the way, once I've done more testing on it. I’ve tested this with old and new OpenSSL versions, and have now applied it backpatched to 9.6 as it’s been wrong for a long time. Thanks! -- Daniel Gustafsson https://vmware.com/