Prevent dynamic loading of an already loaded engine by eitkin-nvidia · Pull Request #17055 · openssl/openssl

eitkin-nvidia · 2021-11-17T16:29:05Z

Use the engine's engine_id to check if the engine already exists before we attempt to dynamically load it (again). Loading an engine more than once will always fail, and often lead to crashes (#17023).

Hence, it is preferred that we avoid this double load in the first place.

Fixes #17023.

Signed-off-by: Eyal Itkin [email protected]

Use the engine's engine_id to check if the engine already exists before we attempt to dynamically load it (again). Loading an engine more than once will always fail, and often lead to crashes (#17023). Hence, it is preferred that we avoid this double load in the first place. Signed-off-by: Eyal Itkin <[email protected]>

eitkin-nvidia · 2021-11-17T16:30:26Z

Sorry for the mess with git, this is the revision of #17025.

I am still in the process of submitting a CLA, and will update as soon as I have one.
Thanks for your understanding.

crypto/engine/eng_cnf.c

crypto/engine/eng_list.c

include/openssl/engine.h

petrovr · 2021-11-17T21:07:30Z

Hello, Eyal Itkin wrote:

Use the engine's engine_id to check if the engine already exists before we attempt to dynamically load it (again). Loading an engine more than once will always fail, and often lead to crashes (#17023).

[snip] Eyal, did you understand that this is attempt to fix broken configuration? Load by path or by identifier either implicit or explicit are mutually exclusive configurations. Documentation must suggest one of those 3 ways, nevertheless implicit or explicit by identifier is harmless. At application level program code should use engine identifier, i.e. openssl 0.8+ style. Except earlier 1.0.0* releases above works fine on 0.9.7*, 0.9.8* and 1.* releases. Remarks: - on 1.0.0* releases load from configuration is broken as in such case engine never finish. On those versions wrok around is do not use openssl configuration. - some openssl releases has issue with "registration". If I remember well this is not case with 1.0.1 or never. Regards Roumen Petrov

eitkin-nvidia · 2021-11-17T21:18:36Z

I am not trying to solve a case in which a single configuration file specifies multiple load techniques, but a case in which the same configuration file is loaded by mistake more than once. This scenario is apparently way more common than expected, and I though to help fix it in a robust manner instead of sending programs to crash whenever an API change moved the invocation of CONF_modules_load_file().

As I mentioned in #17023, there are countless examples, including popular programs such wget and curl, of programs that by mistake cause the configuration to be loaded twice. These cases lead to segmentation faults in said programs (when engines are used), and #9481 mentions that they should be able to load the config more than once without crashing.

In a recent check Canonical found, on top of wget and curl, that there are at least 4 additional projects that they keep track of that probably suffer from the same bug.

Use the engine's engine_id to check if the engine already exists before we attempt to dynamically load it (again). Loading an engine more than once will always fail, and often lead to crashes (#17023). Hence, it is preferred that we avoid this double load in the first place. Signed-off-by: Eyal Itkin <[email protected]>

Signed-off-by: Eyal Itkin <[email protected]>

eitkin-nvidia · 2021-11-17T21:32:11Z

As I said earlier, I apologize for the git mess in the pull request, once it will undergo all revisions, I will prepare a tidy version. Also still working on the CLA on my side.

petrovr · 2021-11-17T21:38:24Z

Eyal Itkin wrote:

I am not trying to solve a case in which a single configuration file specifies multiple load techniques,[SNIP]

Quote from #17023. .... [engine_section] ibmca = ibmca_section ?????(1) [ibmca_section] # The openssl engine path for ibmca.so. # Set the dynamic_path to where the ibmca.so engine # resides on the system. dynamic_path = /usr/local/lib/ibmca.so ?????(2) engine_id = ibmca ?????(3) .... You sample uses all load techniques. Drop two of them. This is broken configuration! Roumen

eitkin-nvidia · 2021-11-17T22:01:16Z

My sample, like the rest of the examples I've linked to in #17025, are real-life examples from the GitHub of real engines:

We can argue that all these engines use "broken configuration!" or understand that this is how clients of OpenSSL understood that engines should be used, and that braking support for this syntax will probably break 90% of the engines deployed worldwide.

@petrovr I really am trying to help fix a communication issue between engine developers today, and OpenSSL, and I don't fully understand why this is the tone that I get back.

I worked late hours trying to debug a curl and wget issue which caused them both to crash when engines are used. The bug was a result of an API change in OpenSSL that was apparently miscommunicated to many 3rd party developers (wget and curl included) and that Canonical weren't aware of. When checking this issue I found that loading the configuration twice is a repeated issue encountered by engine developers, and the engine devs are blamed that the engine crashes, even though the fault is on the program that used OpenSSL and by mistake loaded the config twice.

I even linked in #17023 to a case from 11 years ago (!) of a crash due to this engine API, and I am now trying to help you fix it so the API will be robust and this error will stop popping to the surface every year. I'm trying to help and resolve this issue, and saying that all engine configs today are "broken!" isn't helping us move forward.

petrovr · 2021-11-17T22:21:36Z

Eyal Itkin wrote:

My sample, like the rest of the examples I've linked to in #17025, are real-life examples from the GitHub of real engines: * PKA - https://github.com/Mellanox/pka * pkcs11 - https://github.com/OpenSC/libp11/tree/709f964f4e89502167aaa3e5782229331ee2b401 * bee2evp - https://github.com/bcrypto/bee2evp * ibmca - https://github.com/opencryptoki/openssl-ibmca/blob/master/src/openssl.cnf.sample * tpm2tss - https://github.com/tpm2-software/tpm2-tss-engine/blob/master/openssl.conf.sample We can argue that all these engines use "broken configuration!" or understand that this is how clients of OpenSSL understood that engines should be used, and that braking support for this syntax will probably break 90% of the engines deployed worldwide.

So please fill defect in all those project to stop to promote "broken configuration". Cut/paste error is so common. In addition pkcs11 is also sample for broken implementation.

@petrovr I really am trying to help fix a communication issue between engine developers today, and OpenSSL, and I don't fully understand why this is the tone that I get back.

You code does not add value. Added value will be to document properly ways to configure engines. But taking into account new provider functionality now is too late. Long time ago I stop to fight with 0.9.7 style samples. OpenSSL 0.9.8 obsoletes them! Many project documentations still show it. About 15 years!? Regards, Roumen Petrov

Will squash together all commits when the final revision will be approved. Signed-off-by: Eyal Itkin <[email protected]>

bernd-edlinger · 2021-11-18T08:55:45Z

crypto/engine/eng_cnf.c

+        goto err;
+    if (!ENGINE_ctrl_cmd_string(e, "SO_PATH", dynamic_path, 0))
+        goto err;
+    if (!ENGINE_ctrl_cmd_string(e, "ID", engine_id, 0))


It is possible that engine_id is NULL here. It is probably better not to call in this case.

I don't think it can happen, see line 78 - name = skip_dot(name);, and there is even a debug print of the name in it.

oh, but then I dont see why you do this at all.
even this example does not need this complication:

[engine_section] ibmca = ibmca_section [ibmca_section] # The openssl engine path for ibmca.so. # Set the dynamic_path to where the ibmca.so engine # resides on the system. dynamic_path = /usr/local/lib/ibmca.so engine_id = ibmca init = 1

the name is ibmca from the beginning and indeed the
engine_id = ibmca is superfluous but benign.

On the other hand, our configure file could do something like that:

openssl_conf = ossl_cnf [ ossl_cnf ] engines = eng_cnf [ eng_cnf ] test = xxx [ xxx ] a.dynamic_path = /usr/local/lib/lib1.so b.dynamic_path = /usr/local/lib/lib2.so c.dynamic_path = /usr/local/lib/lib3.so

works now, but breaks with this patch.

Thanks for feedback, I went over your examples, alongside additional configuration files, and now I better understand the configuration types available in the .conf file.

If split to cases, we have the following cases and current (before my fix) behavior:

A single engine, using only "engine_id"

A single engine, using only "dynamic_path"

A single engine, using "engine_id" and then "dynamic_path" - engine_id won't be used at all

A single engine, using "dynamic_path" and then "engine_id" - engine_id won't be used at all

Multiple engines in same section, using only "dynamic_path"

Multiple engines in same section - "engine_id" would only be used if first, and init/ctrl happens before the rest of the engines

The init sequence and refcount tracking of the engines makes me believe that either there are bugs when multiple engines (5 and 6) are used in the same section, or this case wasn't meant to be supported at all. The init/ctrl will only work if they occur directly after every engine, and the chances for undefined init behavior are very high. In addition, the reference counting for such engines would be flawed (too high) as only a single engine will get decremented at the end of the function.

All in all, one can load multiple engines, but it is hard to say it will work well in regards to refcounts and init of the engines, and with very high probability "engine_id" (if passed) won't ever be used. I agree that in such cases, "engine_id" isn't well defined anyway, and shouldn't be used.

In the first case, we know the "engine_id", and "ENGINE_by_id()" will use the "ID" command for the dynamic load. Hence, the dynamic load could easily check the engine by name and avoid duplications.

In the second case, we can't really "know" in advance what will be the engine ID, and I agree the dynamic load shouldn't use the "ID" cmd.

In all other cases, "engine_id" won't ever be used, unless the flow is "engine_id", "init", "dynamic_path", and I don't think it was ever meant to be supported. Hence, I argue that in cases 3 and 4, "engine_id" is actually used by users as a "hint" and we could use it when loading the engine by supplying the "ID" command.

If going according to this logic, the only case in which we can insert errors, is in cases 3 and 4: if for some reason there is a mismatch between the "engine_id" and the one of the real loaded engine, and I don't find it very likely. In the multi-load scenario (in a single section), or in case 2, the only thing we lose is the protection against double-load. As the entire fix is a best-effort mechanism, and considering the circumstances, I think it is something we can live with.

According to the documentation (https://www.openssl.org/docs/man1.1.1/man5/config.html) each section is an "engine section", and only a single engine is supported per such section. This means the example you used above isn't supported, and thus cases 5 and 6 weren't supported even before my fix.

Again, this makes sense in regards to the init logic, and to the reference count issues I pointed to above.

According to the same documentation, the engine name is indeed set by the configuration section (again, singularity), and can be overriden by "engine_id". Hence, in case 2 we still know what the engine ID should be, and the dynamic engine could use it for the check.

I start to agree with @petrovr
A perfectly valid configuration will just use "dynamic_path", and not necessarily have an "engine_id" at all.
The section name may or may not be identical with the engine id, this name is currently just ignored.
Now this configuration will start to fail, while other really invalid configurations will still fail, but more gracefully.

crypto/engine/eng_list.c

crypto/engine/eng_dyn.c

Signed-off-by: Eyal Itkin <[email protected]>

bernd-edlinger · 2021-11-19T12:29:37Z

@eitkin-nvidia see #17073 for how I would fix the duplicate engine loading.

eitkin-nvidia · 2021-11-21T11:06:28Z

I saw your suggestion and I think it is indeed better than mine. Aborting this pull request.

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Nov 17, 2021

FdaSilvaYY reviewed Nov 17, 2021

View reviewed changes

eitkin-nvidia added 2 commits November 17, 2021 21:23

Merge branch 'master' of https://github.com/eitkin-nvidia/openssl-1

56de4a6

Signed-off-by: Eyal Itkin <[email protected]>

Engine fix revision #3

0633731

Will squash together all commits when the final revision will be approved. Signed-off-by: Eyal Itkin <[email protected]>

bernd-edlinger reviewed Nov 18, 2021

View reviewed changes

FdaSilvaYY reviewed Nov 18, 2021

View reviewed changes

crypto/engine/eng_list.c Show resolved Hide resolved

crypto/engine/eng_list.c Outdated Show resolved Hide resolved

crypto/engine/eng_dyn.c Outdated Show resolved Hide resolved

Git revision #4 for this commit

ea2c967

Signed-off-by: Eyal Itkin <[email protected]>

bernd-edlinger mentioned this pull request Nov 19, 2021

Avoid loading of a dynamic engine twice #17073

Closed

eitkin-nvidia closed this Nov 21, 2021

Uh oh!

Conversation

eitkin-nvidia commented Nov 17, 2021

Uh oh!

eitkin-nvidia commented Nov 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrovr commented Nov 17, 2021 via email

Uh oh!

eitkin-nvidia commented Nov 17, 2021

Uh oh!

eitkin-nvidia commented Nov 17, 2021

Uh oh!

petrovr commented Nov 17, 2021 via email

Uh oh!

eitkin-nvidia commented Nov 17, 2021

Uh oh!

petrovr commented Nov 17, 2021 via email

Uh oh!

bernd-edlinger Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

eitkin-nvidia Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

eitkin-nvidia Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

eitkin-nvidia Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bernd-edlinger commented Nov 19, 2021

Uh oh!

eitkin-nvidia commented Nov 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants