Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update OpenSSL to version 3.0.8 in the base system #740

Closed
wants to merge 33 commits into from

Conversation

khorben
Copy link
Contributor

@khorben khorben commented May 11, 2023

This branch is the closest I am currently to a functional update to OpenSSL 3.0.8 in FreeBSD's base system. It was started from the existing vendor/openssl-3.0 branch.
The thorough review expected should include:

  • The configuration applied (default from the corresponding security/openssl30 port with the legacy provider enabled)
  • Security patches up to date (the port fixes CVEs, I have not included them yet)
  • Other patches in the corresponding port may be relevant too (I have not included them)
  • API changes reflected accurately in secure/lib/libcrypto/Version.map and secure/lib/libssl/Version.map
  • Whether the choice of SHLIB_MAJOR is good (currently 30 since 3 is already obsolete)
  • Functional tests on Tier-1 architectures (amd64, aarch64...)
  • Relevance of the ossl-modules providers
  • Patches to software using OpenSSL are correct (especially libradius)
  • The included update of ldns is correct (from the vendor/ldns-1.8.3 tag by YT)
  • This does not touch the OpenSSL files that were moved into sys/crypto/openssl to avoid any trouble with the kernel, and re-imports them into crypto/openssl instead; security fixes beware of both locations

In most software users of OpenSSL, a compatibility compilation flag was used in order to expose and use the former OpenSSL API.

PR: 271615
Sponsored by: FreeBSD Foundation

juikim and others added 3 commits February 28, 2023 19:28
Summary:

Release notes can be found at
https://www.openssl.org/news/openssl-3.0-notes.html .

Obtained from:  https://www.openssl.org/source/openssl-3.0.8.tar.gz
Differential Revision:	https://reviews.freebsd.org/D38835

Test Plan:
```
$ git status
On branch vendor/openssl-3.0
nothing to commit, working tree clean
$ (cd ..; fetch http://www.openssl.org/source/openssl-${OSSLVER}.tar.gz http://www.openssl.org/source/openssl-${OSSLVER}.tar.gz.asc)
openssl-3.0.8.tar.gz                                    14 MB 4507 kBps    04s
openssl-3.0.8.tar.gz.asc                               833  B   10 MBps    00s
$ set | egrep '(XLIST|OSSLVER)='
OSSLVER=3.0.8
XLIST=FREEBSD-Xlist
$ gpg --list-keys
/home/ngie/.gnupg/pubring.kbx
-----------------------------
pub   rsa4096 2014-10-04 [SC]
      7953AC1FBC3DC8B3B292393ED5E9E43F7DF9EE8C
uid           [ unknown] Richard Levitte <[email protected]>
uid           [ unknown] Richard Levitte <[email protected]>
uid           [ unknown] Richard Levitte <[email protected]>
sub   rsa4096 2014-10-04 [E]

$ gpg --verify openssl-${OSSLVER}.tar.gz.asc openssl-${OSSLVER}.tar.gz
gpg: Signature made Tue Feb  7 05:43:55 2023 PST
gpg:                using RSA key 7953AC1FBC3DC8B3B292393ED5E9E43F7DF9EE8C
gpg: Good signature from "Richard Levitte <[email protected]>" [unknown]
gpg:                 aka "Richard Levitte <[email protected]>" [unknown]
gpg:                 aka "Richard Levitte <[email protected]>" [unknown]
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 7953 AC1F BC3D C8B3 B292  393E D5E9 E43F 7DF9 EE8C
$ (cd vendor.checkout/; git status; find . -type f -or -type l | cut -c 3- | sort > ../old)
On branch vendor/openssl-3.0
nothing to commit, working tree clean
$ tar -x -X $XLIST -f ../openssl-${OSSLVER}.tar.gz -C ..
$ rsync --exclude FREEBSD.* --delete -avzz ../openssl-${OSSLVER}/* .
$ cat .git
gitdir: /home/ngie/git/freebsd-src/.git/worktrees/vendor.checkout
$ diff -arq ../openssl-3.0.8  .
Only in .: .git
Only in .: FREEBSD-Xlist
Only in .: FREEBSD-upgrade
$ git status FREEBSD*
On branch vendor/openssl-3.0
nothing to commit, working tree clean
$
```

Reviewers: emaste, jkim

Subscribers: imp, andrew, dab

Differential Revision: https://reviews.freebsd.org/D38835
@ngie-eign
Copy link
Contributor

@khorben : you're awesome! Would you be opposed to me taking bits of your work, cherry-picking/squashing them, then pushing bits and pieces into base?

@ngie-eign
Copy link
Contributor

My brain cells have been under stimulated lately. I'd really like to do some straightforward FreeBSD development.

@ngie-eign
Copy link
Contributor

Hmm.. the build failed with amd64. Let's see if I can track down why and propose a change in the PR.

@khorben
Copy link
Contributor Author

khorben commented May 11, 2023

@khorben : you're awesome! Would you be opposed to me taking bits of your work, cherry-picking/squashing them, then pushing bits and pieces into base?

This is great to hear and absolutely, feel free to sort it out and clear the way :)
I tried to keep the commit history relatively clean, I just couldn't put all of the relevant changes directly in the merge commit.

@khorben khorben force-pushed the khorben/openssl-3.0-ldns-modules branch from d5b355b to 1508a74 Compare May 11, 2023 04:39
@khorben
Copy link
Contributor Author

khorben commented May 11, 2023

Hmm.. the build failed with amd64. Let's see if I can track down why and propose a change in the PR.

Classic, I forgot to git add a file; I have updated the branch.

@@ -110,6 +110,7 @@ LIBADD+= netgraph
.if ${MK_OPENSSL} == "no"
CFLAGS+=-DNO_OPENSSL
.else
CFLAGS+= -DOPENSSL_API_COMPAT=0x10100000L
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this one in 9c6f3df and libfido2 in 469c325 already.

@@ -68,6 +68,7 @@ CFLAGS+= -DTLS=__thread
CFLAGS+= -D_FIDO_MAJOR=1
CFLAGS+= -D_FIDO_MINOR=10
CFLAGS+= -D_FIDO_PATCH=0
CFLAGS+= -DOPENSSL_API_COMPAT=0x10100000L
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one done in 469c325

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does updating libfido2 fix this issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libfido2's upstream build does:

        if(CRYPTO_VERSION VERSION_GREATER_EQUAL 3.0)
                add_definitions(-DOPENSSL_API_COMPAT=0x10100000L)
        endif()

with a few special case #if OPENSSL_VERSION_NUMBER >= 0x30000000 here and there, but I imagine a future update will address this.

@emaste
Copy link
Member

emaste commented May 11, 2023

@ngie-eign I think all of the CFLAGS+=-DOPENSSL_API_COMPAT=0x10100000L additions should be good to go now.

I used "specify OpenSSL 1.1 API" instead of "fix building with OpenSSL 3.0" in the commit message subjects, and these commits should have a Sponsored by: The FreeBSD Foundation.

@emaste
Copy link
Member

emaste commented May 11, 2023

Hmm, dma fails with OPENSSL_API_COMPAT set, with:

/usr/home/emaste/src/freebsd-git/main/contrib/dma/crypto.c:118:2: error: call to undeclared function 'SSL_library_init'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
        SSL_library_init();
        ^
/usr/home/emaste/src/freebsd-git/main/contrib/dma/crypto.c:119:2: error: call to undeclared function 'SSL_load_error_strings'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
        SSL_load_error_strings();
        ^
/usr/home/emaste/src/freebsd-git/main/contrib/dma/crypto.c:119:2: note: did you mean 'ERR_lib_error_string'?
/usr/include/openssl/err.h:239:13: note: 'ERR_lib_error_string' declared here
const char *ERR_lib_error_string(unsigned long e);
            ^
2 errors generated.

Indeed,

The SSL_library_init() and OpenSSL_add_ssl_algorithms() functions were deprecated in OpenSSL 1.1.0 by OPENSSL_init_ssl().

so IMO we should update dma and add the OPENSSL_API_COMPAT #define first

@ngie-eign
Copy link
Contributor

Hmm, dma fails with OPENSSL_API_COMPAT set, with:

/usr/home/emaste/src/freebsd-git/main/contrib/dma/crypto.c:118:2: error: call to undeclared function 'SSL_library_init'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
        SSL_library_init();
        ^
/usr/home/emaste/src/freebsd-git/main/contrib/dma/crypto.c:119:2: error: call to undeclared function 'SSL_load_error_strings'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
        SSL_load_error_strings();
        ^
/usr/home/emaste/src/freebsd-git/main/contrib/dma/crypto.c:119:2: note: did you mean 'ERR_lib_error_string'?
/usr/include/openssl/err.h:239:13: note: 'ERR_lib_error_string' declared here
const char *ERR_lib_error_string(unsigned long e);
            ^
2 errors generated.

Indeed,

The SSL_library_init() and OpenSSL_add_ssl_algorithms() functions were deprecated in OpenSSL 1.1.0 by OPENSSL_init_ssl().

so IMO we should update dma and add the OPENSSL_API_COMPAT #define first

Approved internal change for merge with FreeBSD.
I provided a complete patch upstream to corecode/dma#126 to leverage the OpenSSL 3 APIs if/when possible.

@emaste
Copy link
Member

emaste commented May 12, 2023

Approved internal change for merge with FreeBSD.

I've committed that for now, will replace with upstream dma after your change is in.

Copy link
Contributor

@ngie-eign ngie-eign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submitted upstream as libarchive/libarchive#1869 .

@@ -10,7 +10,7 @@ CFLAGS+= -DPLATFORM_CONFIG_H=\"${.CURDIR}/config_freebsd.h\"

.if ${MK_OPENSSL} != "no"
CFLAGS+= -DWITH_OPENSSL
LIBADD+= crypto
LIBADD+= ssl crypto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong. Bringing in libssl as a dependency seems like overlinking to me.
What issues were you running into before?

Copy link
Contributor

@ngie-eign ngie-eign May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on my comment... libssl should not be required here. None of the libssl-related APIs are used in libarchive.
My guess is that this change was made based on similar code in the upstream pkg-config. I submitted libarchive/libarchive#1870 to correct the overlinking in the spec.

Copy link
Contributor

@ngie-eign ngie-eign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@ngie-eign ngie-eign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't right. libmd and libcrypto shouldn't be mixed.

@ngie-eign
Copy link
Contributor

This isn't right. libmd and libcrypto shouldn't be mixed.

To be clear (because my last statement wasn't potentially): the implementations can mix by design, but there's no reason why there should be 2 crypto implementations linked into a library.

This effectively overlinks libmd into libfetch when MK_OPENSSL != no.

Copy link
Contributor

@ngie-eign ngie-eign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: https://reviews.freebsd.org/D40082

The lib/ldns change was submitted as c6750dd.

@ngie-eign
Copy link
Contributor

Review: https://reviews.freebsd.org/D40082

The lib/ldns change was submitted as c6750dd.

The above commit broke the build; I reverted it in 902dc54.

This is something that will likely need to come in as part of the entire OpenSSL 3.0 deliverable, or adjusted to work with main, as shipped today.

@emaste
Copy link
Member

emaste commented May 20, 2023

Review: https://reviews.freebsd.org/D40082

The lib/ldns change was submitted as c6750dd.

The above commit broke the build; I reverted it in 902dc54.

This is perhaps the same sort of issue we encountered with dma, but a more involved example?

Merge commit 'e4520c8bd1d300a7a338d0ed4af171a2d0e583ef' into khorben/openssl-3.0
With the update to OpenSSL 3.0, engines are installed into a different
folder, and modular providers can be installed into a dedicated folder
as well.
Ed has a better commit message for this in commit
3e98230.
khorben added 19 commits May 23, 2023 05:24
This also requires lowering the level of warnings for archive_hmac.c
when building with OpenSSL 3.
This disables warning-errors for:

    archive_hmac.c:241:64: error: passing argument 2 of
    'OSSL_PARAM_construct_utf8_string' discards 'const' qualifier from
    pointer target type [-Werror=discarded-qualifiers]
This commit leveraged the updated Makefile.asm in order to re-generate
the assembly files provided by OpenSSL.
This casts the second parameter to OSSL_PARAM_construct_utf8_string() as
a char * for a string litteral, as documented in EVP_MAC(3).
ec_nistp_64_gcc_128 is only supported on 64-bit systems, but also only
on little-endian systems.

This fixes the build on PowerPC 64 (big-endian).
@khorben khorben force-pushed the khorben/openssl-3.0-ldns-modules branch 2 times, most recently from c4f45c1 to ac32131 Compare May 26, 2023 21:27
@khorben khorben marked this pull request as ready for review May 26, 2023 21:28
@khorben khorben force-pushed the khorben/openssl-3.0-ldns-modules branch from ac32131 to c9f9966 Compare May 26, 2023 21:58
@khorben
Copy link
Contributor Author

khorben commented May 26, 2023

I have now marked this PR as "ready", with the following to keep in mind:

  • The configuration applied is as per the security/openssl30 port, with the legacy provider enabled
  • This is strictly the 3.0.8 release, security patches issued since that release are not included; the CVE fixes found in the security/openssl30 port are not included here (they probably deserve their own respective commits in order to keep track)
  • Other patches in the corresponding port may be relevant too (I have not included them)
  • I believe the API changes to be reflected accurately in secure/lib/libcrypto/Version.map and secure/lib/libssl/Version.map
  • The choice of SHLIB_MAJOR is 3 in OpenSSL's header files (as per upstream) but 30 for the .so files (since .3 has already been used by FreeBSD before in base)
  • The following architectures are believed to build: amd64, aarch64, i386, powerpc64
  • The ossl-modules providers have not been tested yet
  • This does not touch the OpenSSL files that were moved in bc3d569 into sys/crypto/openssl to avoid any trouble with the kernel, and re-imports them into crypto/openssl instead; security fixes beware of both locations
  • The assembly files for amd64, i386, aarch64, powerpc64 were obtained by re-generating them with Makefile.asm

@emaste
Copy link
Member

emaste commented May 26, 2023

cc @bsdjhb

@khorben khorben force-pushed the khorben/openssl-3.0-ldns-modules branch from cd93b73 to 069e79e Compare May 31, 2023 04:54
@khorben khorben marked this pull request as draft May 31, 2023 04:54
@khorben
Copy link
Contributor Author

khorben commented May 31, 2023

Unfortunately this pull-up request has to go back to draft after the update to 1.1.1u in main.

@khorben
Copy link
Contributor Author

khorben commented Jun 1, 2023

Closing this pull-up request now that the direct update to OpenSSL 3.0.9 is almost ready in #760.

@khorben khorben closed this Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants