OpenSSL 3.0 has removed ERR_GET_FUNC

[omajid]

It was removed in a beta release of OpenSSL 3.0: openssl/openssl@561e5cd

But runtime is still using it:

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c

Line 1081 in 49c74ee

if (err != 0 && ERR_GET_FUNC(err) != ASN1_F_A2D_ASN1_OBJECT)

cc @bartonjs

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

It was removed in a beta release of OpenSSL 3.0: openssl/openssl@561e5cd

But runtime is still using it:

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c

Line 1081 in 49c74ee

if (err != 0 && ERR_GET_FUNC(err) != ASN1_F_A2D_ASN1_OBJECT)

cc @bartonjs

Author:	omajid
Assignees:	-
Labels:	area-System.Security, untriaged
Milestone:	-

[omajid]

See openssl/openssl#16254 for more information about why this removal still lets builds succeed (with warnings) but breaks .NET at runtime.

[bartonjs]

Huh, I'm rather surprised that we're not failing the build in that case.

[omajid]

Huh, I'm rather surprised that we're not failing the build in that case.

See openssl/openssl#16254 for why. It's not just us, httpd (mod_ssl) broke too: openssl/openssl#16004 (comment).

[bartonjs]

Yeah, I mean "I'm surprised we're not already compiling with no-implicit-functions" (or whatever the flag would be)

[omajid]

Oh. Well, the not-build failures I got were with my own WIP PR (dotnet/corefx#43078), so maybe I messed up somewhere?

Edit: only libunwind is using that flag atm:

$ git rev-parse HEAD
57e1c232ee4ce5a5a4413de4fc66544e4e346a62
$ ag no-implicit
src/libraries/Native/Unix/System.IO.Compression.Native/CMakeLists.txt
55:    set_source_files_properties(${NATIVECOMPRESSION_SOURCES} PROPERTIES COMPILE_FLAGS -Wno-implicit-fallthrough)

src/libraries/Native/Unix/CMakeLists.txt
76:    add_compile_options(-Wno-implicit-int-float-conversion)

src/coreclr/pal/src/libunwind/CMakeLists.txt
37:    add_compile_options(-Wno-implicit-fallthrough)
44:      add_compile_options(-Wno-implicit-function-declaration)
58:        add_compile_options(-Wno-implicit-function-declaration)
78:        add_compile_options(-Wno-implicit-function-declaration)

[fweimer-rh]

@omajid Not sure about the libunwind part: -Wno-implicit-function-declaration suppresses the warning that is enabled by default. Everyone who can should be building with -Werror=implicit-function-declaration to get a reliable failure at compile time.

(There's a fairly long and boring story why GCC defaults haven't changed yet. It's very hard to do without causing major and totally non-obvious breakage because of the way the changed default causes configure checks to fail unexpectedly. And of course those failing checks automatically disable the tests for the disabled feature, too, so you end up with clean builds that pass the test suite, but can miss rather important functionality.)

[omajid]

@bartonjs a CI job that builds runtime against OpenSSL 3.0 beta (without any build flag customizations) fails as expected:

  /home/tester/runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c:1081:25: error: implicit declaration of function 'ERR_GET_FUNC' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
          if (err != 0 && ERR_GET_FUNC(err) != ASN1_F_A2D_ASN1_OBJECT)
                          ^
  /home/tester/runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c:1081:25: note: did you mean 'ERR_GET_LIB'?
  /usr/include/openssl/err.h:241:36: note: 'ERR_GET_LIB' declared here
  static ossl_unused ossl_inline int ERR_GET_LIB(unsigned long errcode)
                                     ^
  1 error generated.
  make[2]: *** [Native.Unix/System.Security.Cryptography.Native/CMakeFiles/objlib.dir/build.make:104: Native.Unix/System.Security.Cryptography.Native/CMakeFiles/objlib.dir/openssl.c.o] Error 1
  make[1]: *** [CMakeFiles/Makefile2:2061: Native.Unix/System.Security.Cryptography.Native/CMakeFiles/objlib.dir/all] Error 2
  make[1]: *** Waiting for unfinished jobs....

I think the silent failures were a 3.1-specific thing due to my backport.

[bartonjs]

A bit of a look inside my brain here.

• Maybe we should just delete this check and remove the throw.
  • No, that doesn't sound right.
• OK, what's this really trying to check?
  • If the OID doesn't parse return 0, if something weird happens (like OOM) return -1 so we throw instead of caching failure.
• Hm. If we checked ERR_GET_LIB against ERR_LIB_ASN1 would that work?
  • It might. Ye..s. It will. Do that.
• Cool. That compiled, but now tests fail if it builds against 3.0 but runs against 1.0.
  • What?!
• Oh, looks like ERR_GET_LIB was a macro >> 24, and now it's an inline function that's complicated, then >> 23. Error codes are packed differently in 3.0
  • Ugh. OK, let's bridge that.
• 50 lines of code later, with lots of hard coded constants, I'm not sure this is a good idea.
  • Quit whining. OK, you might be right.
• Why are we doing this again?
  • If a2d_ASN1_OBJECT fails we want to return 0, and we identify that by the error code.
• Why don't we just call a2d_ASN1_OBJECT directly if it fails?
  • Um. Um. Error queue preservation?
• Drat. Why don't we call it first?
  • Perf? Doing work twice?
• It's not much work, and the results are cached.
  • Why don't we call it first?
• Done.

PR coming soon.

[bartonjs]

Here's the gist of it, if you want to try it out early. It's missing the portable function binding step, and I'll probably restore part of the comment.

@@ -1068,22 +1070,22 @@ int32_t CryptoNative_LookupFriendlyNameByOid(const char* oidValue, const char**
         return -2;
     }

+    // First, check if oidValue parses as an OID dotted decimal.
+    int i = a2d_ASN1_OBJECT(NULL, 0, oidValue, -1);
+
+    if (i <= 0)
+    {
+        ERR_clear_error();
+        return 0;
+    }
+
     // Do a lookup with no_name set. The purpose of this function is to map only the
     // dotted decimal to the friendly name. "sha1" in should not result in "sha1" out.
     oid = OBJ_txt2obj(oidValue, 1);

-    if (!oid)
+    if (oid == NULL)
     {
-        unsigned long err = ERR_peek_last_error();
-
-        // If the most recent error pushed onto the error queue is NOT from OID parsing
-        // then signal for an exception to be thrown.
-        if (err != 0 && ERR_GET_FUNC(err) != ASN1_F_A2D_ASN1_OBJECT)
-        {
-            return -1;
-        }
-
-        return 0;
+        return -1;
     }

[omajid]

Thanks for the fix! I can confirm the change fixes the build for me!