OpenSSL servers can send a non-empty status_request in a CertificateRequest

Test package at https://launchpad.net/~nicolasbock/+archive/ubuntu/sf00317240

The attachment "bionic.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

for later reference, i'd discussed this with nick and asked him to check if the 'status_request' reply contained any kind of valid data in the specific cases where this patch will disable it; my concern is if there is valid data in it, it's possible there are applications out there that might currently expect and/or use it, even if it's against the RFC, which might result in a regression after this patch. However, if the reply is empty or just has garbage, it's unlikely that any application is using it for anything currently, so there would be less chance of causing a regression.

Hi Dan,

Before the upstream openssl fix [1] the behavior was to send a proper CertificateStatus if the client requested the status_request extension from the server in a CertificateRequest. In other words, the additional information contains valid data.

[1] https://github.com/openssl/openssl/pull/9780

> the additional information contains valid data.

then I think SRU'ing this would cause a behavior change that could possibly break someone, which isn't something we should do.

I'd suggest putting the fix behind some opt-in mechanism, so anyone who is affected can opt-in to the fixed behavior, but there's no change by default.

For posterity, here is an example of the relevant part of the client trace output when the bug is active, i.e. data is sent in response to the CertificateRequest:

$ grep -B1 -A4 CertificateRequest s_client.log
  Inner Content Type = Handshake (22)
    CertificateRequest, Length=1570
      request_context (len=0):
      extensions, length = 1567
        extension_type=status_request(5), length=1521
          0000 - 01 00 05 ed 30 82 05 e9-0a 01 00 a0 82 05 e2 ....0..........

Here is an example of the relevant part of the client trace output when this is fixed:

$ grep -B1 -A4 CertificateRequest s_client.log
  Inner Content Type = Handshake (22)
    CertificateRequest, Length=45
      request_context (len=0):
      extensions, length = 42
        extension_type=signature_algorithms(13), length=38
          ecdsa_secp256r1_sha256 (0x0403)

openssl (1.1.1-1ubuntu2.1~18.04.15ubuntu1) bionic; urgency=medium

  * Backport pr9780 but make it optional:
    - d/p/pr9780_0002-Teach-TLSProxy-how-to-parse-CertificateRequest-messa.patch
    - d/p/pr9780_0001-Don-t-send-a-status_request-extension-in-a-Certifica.patch
    - d/p/lp1940141-make-pr-9780-optional.patch
    - d/p/lp1940141-fix-tests-accordingly.patch
    - d/p/lp1940141-update-manpages.patch

 -- Bruce Elrick <email address hidden> Wed, 16 Mar 2022 17:05:32 +0000

Hey Dan Streetman (ddstreet), I believe the above patch satisfies your valid concerns about backporting. Would you please analyse for acceptance?

The 2nd upstream patch appears to add new functionality, for actually parsing a certificate request; is that actually needed (outside of the self-tests)? If not, it shouldn't be included in the backport.

It's not needed for actual functionality of the backport but that assumes that any future backports or fixes don't break this backport. By adding the tests (and wrapping the one test that fails when the backport-enabling environment variable is not set with the enabling of that environmetn variable, we ensure that and future changes don't break this change.

"variable, we" -> "variable) we"
"and future" -> "any future"

> It's not needed for actual functionality of the backport but that assumes that any future backports or fixes don't break this backport

yes i get that, my comment is about whether or not the patch changes any code *outside* of the self-tests, e.g. the TLSProxy perl code changes. If that's *only* used for self-tests then including in the backport shouldn't cause any regression.

Remember that people reviewing/sponsoring patches may not have deep experience with the code so it's good to more clearly explain things that aren't necessarily obvious, such as the 2nd patch only affecting test code (if that is indeed the case), since at first glance that isn't what it looks like.

@ubuntu-security since this is openssl, could you give the debdiff a review? I can sponsor it as a normal SRU if you have no objections (and the changes look ok to you), as it doesn't really seem like it would specifically need to go to the -security pocket.

ACK on the two PR9780* patches, but I must say I'm a bit uncomfortable making a bug fix optional (which is what is done in the lp1940141* patches).

While it does change what is returned to the client, that part shouldn't be there in the first place. While it's nice to be overly cautious, we don't typically make bug fixes optional unless there is an unwanted behaviour change, and I don't think that is applicable here. This was fixed in all later releases without causing regressions that I know of, and having it by default will fix more users than having it be optional.

If you'd rather remove the opt-in part, that's fine with me; I can sponsor the debdiff then with the opt-in parts left out, if that works for you Bruce and Nicolas.

uploaded to bionic queue, thanks!

Good point, Dan. A caveat to my work here on this patch: I did some C programming and played with the Perl test harness in the early 90s which gave me enough abililty to develop this patch set but I would benefit from any deeper knowledge someone else may have, especially anyone with openssl knowledge.

In terms of due dilligance, I've searched through the source files for references to 'TSProxy' which is the parent directory for a number of the test hardness perl modules changes in the commit and nothing I find indicates it is referenced by the actual openssl C code but only is part of the build/test process.

For extra due diligence I built with only the tests change backported and two tests fail. Tests failing is expected because the actual functional backport is missing. When I add the functional backport then all tests succeed, as expected. When I then make the test optional, the same two tests fail, as expected. See the test-results.txt attachment for more.

I fix the tests by enabling the fix for 'Test 6' only in openssl-1.1.1/test/recipes/70-test_tls13messages.t by wrapping the test with perl code to set and delete the FIX_LP_1940141 environment variable. As an aside, I now see that other such env var setting does not use single-quoted strings but rather just bare strings; this is allowed in perl and my patch does not followed the style in the rest of the test file.

My understanding is that there are several flags for subtests, thus accounting for the two passing or failing.

But more to your point about the risk of backporting the tests, my assessment
is that the perl modules are only in testing and while there are a non-trivial
number of tests added along with the one that tests what is being backported,
the fact that they all pass suggests that they are relevant here.

That is, it looks to me these additional tests are not-inappropriate, are
actually appropriate, and in fact the reason they were added later was this
fix forced the issue on the lack of capability in the test coverage.

Marc, I lean your way in terms of my feel for the likelihood that someone is relying on incorrect behaviour in bionic/openssl-1.1.1.

I can say that while the lp1940141* patches rely on the pr9780 ones, the reverse is absolutely not the case and thus the lp1940141 ones could be dropped as a whole to achieve the functional portion of the backport without it being optional.

But I leave that decision to those with more experience.

Per comments from Marc, let's proceed without making this fix optional. Let's just keep our eyes and ears open for any possible regressions caused by this change landing in bionic.

Hello Nicolas, or anyone else affected,

Accepted openssl into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/openssl/1.1.1-1ubuntu2.1~18.04.16 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

I pulled the proposed source:

  $ pull-lp-source openssl bionic

built with tracing enabled:

  $ cd openssl-1.1.1
  $ sed -i -e '/^CONFARGS =/a CONFARGS += enable-ssl-trace' debian/rules
  $ debuild -us -uc -b 2>&1 | tee ../debuild.log
  $ cd ..

installed:
  $ sudo dpkg -i libssl1.1_1.1.1-1ubuntu2.1~18.04.16_amd64.deb openssl_1.1.1-1ubuntu2.1~18.04.16_amd64.deb

tested:

  $ openssl req -x509 -sha256 -nodes -days 365 -newkey rsa:2048 -keyout key.pem -out cert.pem
  $ openssl s_server -key key.pem -cert cert.pem -status_file openssl-1.1.1/test/recipes/ocsp-response.der -Verify 5 2>&1 | tee s_server.log &
  $ openssl s_client -status -trace -cert cert.pem -key key.pem 2>&1 | tee s_client.log (^c)
  $ grep -B1 -A4 CertificateRequest s_client.log
    Inner Content Type = Handshake (22)
      CertificateRequest, Length=45
        request_context (len=0):
        extensions, length = 42
          extension_type=signature_algorithms(13), length=38
            ecdsa_secp256r1_sha256 (0x0403)
  $ tail -6 s_server.log
  ---
  No server certificate CA names sent
  CIPHER is TLS_AES_256_GCM_SHA384
  Secure Renegotiation IS supported

  ERROR

I should add, the results in comment 22 indicate that the fix is active and working.

All autopkgtests for the newly accepted openssl (1.1.1-1ubuntu2.1~18.04.16) for bionic have finished running.
The following regressions have been reported in tests triggered by the package:

openvswitch/2.9.8-0ubuntu0.18.04.2 (amd64, i386)
dovecot/1:2.2.33.2-1ubuntu4.7 (armhf)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/bionic/update_excuses.html#openssl

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

The user who discovered this bug using the golang TLS implementation will be testing the proposed package within the week.

I re-ran the test as follows to actually test the server side with only the packaged executables.

I used this command for the server side:

/usr/bin/openssl s_server -key key.pem -cert cert.pem -status_file openssl-1.1.1/test/recipes/ocsp-response.der -Verify 5

with ldd showing it loading its ssl & crypt libraries from /usr/lib/x86_64-linux-gnu:

ldd /usr/bin/openmssl
        linux-vdso.so.1 (0x00007ffe6ee65000)
        libssl.so.1.1 => /usr/lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f44027c4000)
        libcrypto.so.1.1 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f44022f9000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f44020da000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4401ce9000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f4401ae5000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f4402d04000)

I built the client from 18.06.16 source with tracing enabled and used it for both tests and ran it with this command:

LD_LIBRARY_PATH=openssl-1.1.1/build_shared openssl-1.1.1/build_shared/apps/openssl s_client -status -trace -cert cert.pem -key key.pem

with ldd showing it loading its own libraries:

LD_LIBRARY_PATH=openssl-1.1.1/build_shared ldd openssl-1.1.1/build_shared/apps/openssl
        linux-vdso.so.1 (0x00007fff51ff9000)
        libssl.so.1.1 => openssl-1.1.1/build_shared/libssl.so.1.1 (0x00007fc8c28b4000)
        libcrypto.so.1.1 => openssl-1.1.1/build_shared/libcrypto.so.1.1 (0x00007fc8c23e9000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fc8c21ca000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fc8c1dd9000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fc8c1bd5000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fc8c2dfe000)

The following logs show the difference between having:
ii openssl 1.1.1-1ubuntu2.1~18.04.15 amd64 Secure Sockets Layer toolkit - cryptographic utility
and having:
ii openssl 1.1.1-1ubuntu2.1~18.04.16 amd64 Secure Sockets Layer toolkit - cryptographic utility

ubuntu@bionic-lp-1940141:~$ grep -B1 -A4 CertificateRequest s_client-18.04.16-from-source-with-trace_server-18.04.15-from-package.log
  Inner Content Type = Handshake (22)
    CertificateRequest, Length=1570
      request_context (len=0):
      extensions, length = 1567
        extension_type=status_request(5), length=1521
          0000 - 01 00 05 ed 30 82 05 e9-0a 01 00 a0 82 05 e2 ....0..........

ubuntu@bionic-lp-1940141:~$ grep -B1 -A4 CertificateRequest s_client-18.04.16-from-source-with-trace_server-18.04.16-propo...

Unfortunately the package in bionic-proposed got superseded by a security update and will need to be re-uploaded.

Thanks for the update, Marc. I will work on producing the dsc against 1.1.1-1ubuntu2.1~18.04.17.

Attach debdiff relative to 18.04.17:

openssl (1.1.1-1ubuntu2.1~18.04.17ubuntu1) bionic; urgency=medium

  * Backport pr9780:
    - d/p/pr9780_0001-Don-t-send-a-status_request-extension-in-a-Certifica.patch
    - d/p/pr9780_0002-Teach-TLSProxy-how-to-parse-CertificateRequest-messa.patch

 -- Bruce Elrick <email address hidden> Mon, 09 May 2022 19:38:43 +0000

Please note the manual testing using the trace option shows the same results.

Thank you to all involved in the discussion and analysis for carefully considering the regression risk there. Regardless of the final decision, I think the thoughtful consideration makes this an exemplary SRU.

I confirmed that the new upload is simply a straightforward review on top of the security update, so Łukasz's SRU review and decision stands and I'm accepting this into bionic-proposed.

Hello Nicolas, or anyone else affected,

Accepted openssl into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/openssl/1.1.1-1ubuntu2.1~18.04.18 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

All autopkgtests for the newly accepted openssl (1.1.1-1ubuntu2.1~18.04.18) for bionic have finished running.
The following regressions have been reported in tests triggered by the package:

mysql-5.7/5.7.38-0ubuntu0.18.04.1 (amd64)
streamlink/0.10.0+dfsg-1 (armhf, arm64)
pgbouncer/1.8.1-1build1 (amd64)
openvswitch/2.9.8-0ubuntu0.18.04.2 (i386)
diaspora-installer/0.7.3.1+debian2ubuntu2 (ppc64el, s390x, arm64)
linux-hwe-5.0/5.0.0-65.71 (ppc64el, s390x, armhf, arm64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/bionic/update_excuses.html#openssl

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Several of the autopkgtest failures are due to response issues (e.g.curl: (7) Failed to connect to launchpad.net port 443: Connection timed out) so I'm going to ask that they be run again.

All autopackage tests succeeded after multiple trials. Canonical customer with golang client against openssl server, where bug was first noticed, will be testing.

There is an openssl security update announced for June 21st. If the package in -proposed doesn't get tested and switched to "verification-done", it will be superseded by a security update once again.

I tested this update per the test plan above, and am satisfied it is working correctly.

This bug was fixed in the package openssl - 1.1.1-1ubuntu2.1~18.04.19

---------------
openssl (1.1.1-1ubuntu2.1~18.04.19) bionic-security; urgency=medium

  [ Simon Chopin ]
  * d/p/lp1978093/*: renew some expiring test certificates (LP: #1978093)

  [ Marc Deslauriers ]
  * SECURITY UPDATE: c_rehash script allows command injection
    - debian/patches/CVE-2022-1292.patch: switch to upstream patch, and
      apply it before c_rehash-compat.patch.
    - debian/patches/CVE-2022-2068.patch: fix file operations in
      tools/c_rehash.in.
    - debian/patches/c_rehash-compat.patch: updated patch to apply after
      the security updates.
    - CVE-2022-2068

 -- Simon Chopin <email address hidden> Tue, 14 Jun 2022 13:37:45 +0200