[defect]: potential null dereference when calling openssl API

[icy17]

What type of defect/bug is this?

incorrect 3rd party API usage

How can the issue be reproduced?

There are some potential NULL dereference bugs when calling EVP_DigestSignInit in src/modules/rlm_eap/types/rlm_eap_fast/eap_fast_crypto.c: 237 and 238.

And in src/lib/tls/verify.c, calling X509_STORE_CTX_set_ex_data might cause a null dereference.

It's better to add a null pointer check before using the pointer.

Additional, I want to make sure if I report a bug using an issue like this is OK. Thanks!

Log output from the FreeRADIUS daemon

None

Relevant log output from client utilities

No response

Backtrace from LLDB or GDB

No response

[alandekok]

There is a balance between the "correctness" of checking every single call for errors, and the effort required to add those checks.

In most cases, adding checks like this aren't worth the effort. If the OpenSSL calls fail, then something is massively wrong with the OpenSSL library. The server will fail quickly, and often. The administrator will then fix the OpenSSL library.

Even if we did add these checks, there is very little that the server can do to work around them. It can stop doing the current TLS work, but that means it's not working as intended. The administrator will have to figure out why the server isn't doing TLS (or EAP-FAST).

Which means that any changes not only have to check for failures, they have to log a descriptive error tells the administrator how to fix the issue.

Fixing the issue means upgrading to a version of OpenSSL which isn't completely broken.

I've seen similar requests to check the return value for functions like pthread_mutex_lock(). Such checks are almost always useless. Any error return from pthread_mutex_lock() means that either the pthread implementation is broken, or the program is using the locks incorrectly. i.e. it has bugs.

The solution is to fix the bugs, not to have the program keep doing "something".

The same goes for here. It isn't efficient to go through the code looking for every possible minor error and catching it. If the error cannot be worked around, then there's no point in checking it. The server should just exit.

[icy17]

Thank you for your reply. In fact, memory allocation errors can cause some apis to return null, and I'm not sure if this is a bug that need to fix openssl library. I found that these places do not use MEM function to check the return value, while some other apis do use MEM to check the return value. I don't know if it is a missing. Also, since I'm not sure about the severity of some bugs are for freeradius-server, if I found some other bugs, I may continue to raise some potential security bugs.

[alandekok]

The MEM macro is there largely to quiet the static analysis tools. If memory allocation fails, the server calls exit(), and dies.

Without the MEM macro, the static analysis tools would complain. But the end result would be the same: the server would dereference a NULL pointer, and exit.

So there isn't a lot of point in adding NULL checks everywhere. As I said before, the server can't do anything about most of these errors. They're not like a DB connection failure, where the server can wait a bit and retry. Failures of the OpenSSL API mean (a) there's no more memory, so the server should just crash, or (b) the OpenSSL libraries are buggy, and the server can't do anything about it.

Fixes for security bugs are welcome. But in between static analysis tools and automated fuzzer tests, I'm hoping that we have very few of those.

If there's an issue where sending the server packets causes it to crash, that's a serious issue which should be fixed.

But adding checks to every single API call is likely not worth the effort. Find a real bug and fix it. We're likely to not merge patches which add NULL checks that don't do anything.