Page MenuHomeFreeBSD

openssh: Don't include an unused EVP_CIPHER_CTX_get_iv() stub
ClosedPublic

Authored by jhb on Thu, Aug 7, 9:12 PM.
Tags
None
Referenced Files
F126774363: D51810.id160001.diff
Sat, Aug 23, 8:59 PM
Unknown Object (File)
Fri, Aug 22, 9:25 PM
Unknown Object (File)
Thu, Aug 21, 7:39 PM
Unknown Object (File)
Thu, Aug 21, 7:38 AM
Unknown Object (File)
Wed, Aug 20, 8:27 PM
Unknown Object (File)
Wed, Aug 20, 6:50 PM
Unknown Object (File)
Mon, Aug 18, 6:51 AM
Unknown Object (File)
Mon, Aug 18, 5:47 AM
Subscribers

Details

Summary

This stub isn't actually used on modern versions of OpenSSL for which
OpenSSH uses EVP_CIPHER_CTX_get_updated_iv instead via a wrapper macro.

However, the wrapper macro conflicted with the existing namespace
macro triggering an error on GCC:

In file included from crypto/openssh/sshd-session.c:65:
crypto/openssh/openbsd-compat/openssl-compat.h:71:11: error: "EVP_CIPHER_CTX_get_iv" redefined [-Werror]

71 | #  define EVP_CIPHER_CTX_get_iv EVP_CIPHER_CTX_get_updated_iv
   |           ^~~~~~~~~~~~~~~~~~~~~

In file included from <command-line>:
crypto/openssh/ssh_namespace.h:12:9: note: this is the location of the previous definition

12 | #define EVP_CIPHER_CTX_get_iv                   Fssh_EVP_CIPHER_CTX_get_iv
   |         ^~~~~~~~~~~~~~~~~~~~~

The error was masked on clang due to MIT krb5 adding a blanket
-Wno-macro-redefined. Building sshd-session without Kerberos support
was sufficient to trigger a warning from clang.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 66078
Build 62961: arc lint + arc unit

Event Timeline

des requested changes to this revision.Thu, Aug 7, 11:17 PM
des added a subscriber: des.

This is wrong. The #define in ssh_namespace.h is there because OpenSSH provides its own EVP_CIPHER_CTX_get_iv(), but it's not supposed to do that when EVP_CIPHER_CTX_get_updated_iv() is available. Instead of this, you need to fix openbsd-compat/libressl-api-compat.c to not provide a useless EVP_CIPHER_CTX_get_iv(), and remove it from ssh_namespace.h.

This revision now requires changes to proceed.Thu, Aug 7, 11:17 PM

Retiring D51810 in favor of this revision.

jhb retitled this revision from openssh: Workaround macro redefinition to openssh: Don't include an unused EVP_CIPHER_CTX_get_iv() stub.Fri, Aug 8, 2:36 PM
jhb edited the summary of this revision. (Show Details)

Remove the unused stub instead

des requested changes to this revision.Fri, Aug 8, 2:50 PM

You also need to regenerate ssh_namespace.h in the same commit.

crypto/openssh/openbsd-compat/libressl-api-compat.c
28 ↗(On Diff #160061)

I would prefer nested #ifdefs.

55 ↗(On Diff #160061)

This is now incorrect.

This revision now requires changes to proceed.Fri, Aug 8, 2:50 PM
In D51810#1183897, @des wrote:

You also need to regenerate ssh_namespace.h in the same commit.

sorry, you actually did, but phab hid it from me

crypto/openssh/openbsd-compat/libressl-api-compat.c
55 ↗(On Diff #160061)

I kind off hope/assume our openssh maintainer will upstream this to OpenBSD and it might get cleaned up then (in part, I wasn't sure what format they use for compound expressions). But nested #ifdef's would make that more obvious how to resolve.

Thank you. We should consider upstreaming this, @emaste.

This revision is now accepted and ready to land.Fri, Aug 8, 5:35 PM