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

Authored by jhb on Thu, Aug 7, 9:12 PM.

Details

Reviewers

emaste
des

Commits

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

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 66117
Build 63000: arc lint + arc unit

Event Timeline

jhb created this revision.Thu, Aug 7, 9:12 PM

Herald added a subscriber: imp. · View Herald TranscriptThu, Aug 7, 9:12 PM

jhb requested review of this revision.Thu, Aug 7, 9:12 PM

jhb added a parent revision: D51809: openssh: Include <fcntl.h> explicitly in includes.h.

jhb added a child revision: D51811: krb5: Don't add -Wno-macro-redefined to CFLAGS.

Harbormaster completed remote builds in B66078: Diff 160001.Thu, Aug 7, 9:12 PM

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.

jlduran mentioned this in D49516: openssh: undef EVP_CIPHER_CTX_get_iv prior to defining it.Fri, Aug 8, 10:21 AM

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

Harbormaster completed remote builds in B66112: Diff 160061.Fri, Aug 8, 2:37 PM

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

crypto/openssh/openbsd-compat/libressl-api-compat.c
28–29	I would prefer nested `#ifdef`s.
57	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

jhb added inline comments.Fri, Aug 8, 4:11 PM

crypto/openssh/openbsd-compat/libressl-api-compat.c
57	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.