read OPENSSLDIR from registry #24450

nhorman · 2024-05-21T14:48:52Z

The windows installer we have allows for targeting arbitrary locations when installing our software. But we have a build time defined default directory OPENSSLDIR which may not match that target, leading to missing configuration files/etc. Enhance openssl to query the registry for the key that our installer creates allowing for an installation to match the run time defaults. Back off to the build time directory only if the key is not defined

mattcaswell

We need a test for this.

What about MODULESDIR and ENGINESDIR?

Do we also need some documentation?

crypto/x509/x509_def.c

We normally define and use these macros directly, but on windows, thats undesireable as we don't know the real locations of these macros until install time. Add an api set to fetch these values, that just returns the macro for non-windows platform. On windows, redirect to some internal functions to grab the values from the registry (as written by the installer), backing off to the build time macro values in the event they don't exist

Make use of our new defaults api to get the build time macros from the windows registry where needed

mattcaswell

Some documentation and a test would still be good.

crypto/defaults.c

crypto/x509/x509_def.c

mattcaswell

Looks good. Just some minor nits. Still needs documentation/test

crypto/defaults.c

crypto/x509/x509_def.c

Since windows builds now try to pull build time defines from the registry first, lets add a test to setup some dummy registry entries, and run the version command to see if they get pulled out properly

Added a url without the prorper guards on it

mattcaswell · 2024-05-28T07:01:29Z

NOTES-WINDOWS.md

+Note that, on Windows platforms (both 32 and 64 bit), the above build time
+defaults can be overridden by registry keys.  This is done because it is common
+practice for windows based installers to allow users to place the installation
+tree at various locations not defined at run time.  The following keys:


Did you mean "not defined at build time"?

mattcaswell · 2024-05-28T07:01:50Z

NOTES-WINDOWS.md

+build time defaults will be used.
+
+Note the installer available at https://github.com/openssl/installer will set
+theys keys when the installer is run.


typo: these

mattcaswell · 2024-05-28T07:02:15Z

doc/man7/fips_module.pod

@@ -61,7 +61,7 @@ were given during the build process. You can check the location of the config
 file by running this command:

    $ openssl version -d
-    OPENSSLDIR: "/usr/local/ssl"
+    $ /usr/local/ssl


Why this change?

because it seemed odd when specifying -d for openssl dir to explicitly print OPENSSLDIR on the output. I can reinstate that if you like though

Please revert that change. People can have scripts that depend on this.

mattcaswell · 2024-05-28T07:02:46Z

test/recipes/02-test_windows_registry.t

@@ -0,0 +1,42 @@
+#! /usr/bin/env perl
+# Copyright 2015-2021 The OpenSSL Project Authors. All Rights Reserved.


mattcaswell · 2024-05-28T07:04:09Z

test/recipes/02-test_windows_registry.t

+   "Adding ENGINESDIR registry key");
+
+ok(run(cmd(["reg.exe", "add", "HKLM\\SOFTWARE\\WOW6432Node\\OpenSSL", "/t", "REG_EXPAND_SZ", "/v", "MODULESDIR", "/d", "TESTMODULESDIR", "/f"])),
+   "Adding MODULESDIR registry key");


Hmmm.....so this is going to overwrite any existing registry entries?

yes, do you think we need to restore them afterwards?

Possibly skip the test if they already exist - and if we do run the test, then delete them afterwards

This is bad. Although I understand we want to test it, running make test should not make any system-wide changes. Can we test this in a github CI job instead?

well, sure, its getting tested in CI now. But testing it implies that the registry needs to have the keys set. I suppose the most appropriate solution is to do what @mattcaswell suggests and only preform the test if the keys aren't already set, then delete them afterward. That will have the odd side effect of skipping the test on any windows platform that has run the installer already of course, but perhaps thats ok, as having run the installer implies that their not building from source there anyway

What about doing the test passively - i.e. read the registry entries with reg.exe instead of setting them and if they are set then compare the values with the openssl version output? Then, in a CI job definition use the reg.exe to set some values.

We could do that, but in that event the test will be skipped on local systems in which the registry keys aren't already set, I suppose that might be ok though, but its not ideal.

Really, the make test is not supposed to mess up with your system. And I assume if you're an unprivileged user trying to run make test, this test would fail because you should not be able to edit the system wide registry as an unprivileged user. Unless there is some kind of local user override for the SOFTWARE keys - I have no idea if there is such thing, not being a Windows developer for a very long time.

I'm just going to go with the passive approach. Skip the test if the appropriate keys aren't set, and modify the ci workflow to set the right keys. The local tests for windows will get skipped, but we'll get regular testing in CI that way

mattcaswell · 2024-05-28T07:05:13Z

crypto/defaults.c

+    char *retval = NULL;
+
+    ret = RegOpenKeyEx(HKEY_LOCAL_MACHINE,
+                       TEXT("SOFTWARE\\Wow6432Node\\OpenSSL"), 0,


Should this somehow include the version number in the key? i.e. so that you can have multiple instances of OpenSSL installed?

I was trying to decide on that, we could certainly do that, but it creates another level of indirection, given that these can already be overriden by the OPENSSL_CONF, OPENSSL_MODULES, and OPENSSL_ENGINES environment variable. It also means that we have to co-ordinate the version string built into the binary with the version string used to create the installer. which is feasible, but needs to be done over in the installer repo

I think it is very likely that people will want multiple installations and to keep them separate. I also think the installer should be aware of what version it is installing. So I think it would be a really good idea to do this. I probably would not keep different patch versions distinct, but IMO different minor/major versions definitely should be.

Suggested read order :

HKEY_CURRENT_USER\SOFTWARE\Wow6432Node\OpenSSL\3.2

HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\OpenSSL\3.2

HKEY_CURRENT_USER\SOFTWARE\Wow6432Node\OpenSSL

HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\OpenSSL

where :

3.2 should be replaced by compiled version no.

Wow6432Node should be removed on WIN64 define presence.

Writing into HKEY_LOCAL_MACHINE implies "admin rights" for actual user and current running shell also.

why the multi-tiered fallback? Individual users can already override specific default values with OPENSSL_CONF / OPENSSL_ENGINES / OPENSSL_MODULES in the environment. This seems like unneeded complication

mattcaswell · 2024-05-28T07:12:03Z

Also this needs a rebase

mattcaswell · 2024-05-28T07:18:05Z

I'm wondering whether the registry key look up should be a compile time option. Consider the example where someone installs OpenSSL on their machine using the installer. But they also have an application which has statically linked OpenSSL (or maybe just bundled its own version of OpenSSL). Suddenly that application will start getting its locations from the registry and maybe break.

mattcaswell · 2024-05-28T07:32:15Z

I'm wondering whether the registry key look up should be a compile time option. Consider the example where someone installs OpenSSL on their machine using the installer. But they also have an application which has statically linked OpenSSL (or maybe just bundled its own version of OpenSSL). Suddenly that application will start getting its locations from the registry and maybe break.

Maybe the compile time option could be set by setting an installer "context" define "-DINSTALLCONTEXT=blah", which could be included in the registry key and stop different builds of OpenSSL (maybe produced by different people) from interfering with each other.

FdaSilvaYY

Few comments about Unicode possible issues.

FdaSilvaYY · 2024-05-28T10:34:45Z

crypto/defaults.c

+ *
+ * @return A pointer to a char array containing the registry directories.
+ */
+static char *get_windows_regdirs(char *dst, LPCWSTR valuename)


Should be LPCTSTR to be coherent with TCHAR declaration and TEXT macro usage.

FdaSilvaYY · 2024-05-28T10:37:43Z

crypto/defaults.c

+/**
+ * @brief The directory where OpenSSL is installed.
+ */
+static char openssldir[MAX_PATH];


I advise you to declare its size as [MAX_PATH + 1] here, IMHO.
This will spare some MAX.. - 1 arithmetic thereafter.

FdaSilvaYY · 2024-05-28T10:41:11Z

crypto/defaults.c

+    DWORD ktype;
+    HKEY hkey;
+    LSTATUS ret;
+    LPCWCH tempstr = NULL;


LPCTCH, if it exist or any T equivalent.

LPTSTR tempstr = NULL

crypto/defaults.c

t8m · 2024-05-29T09:23:40Z

crypto/defaults.c

+
+/**
+ * @brief Function to setup default values to run once.
+ * Only used in WIN32 environments.  Does run time initalization


WIN32 -> Windows?

t8m · 2024-05-29T09:28:53Z

doc/man7/fips_module.pod

@@ -61,7 +61,7 @@ were given during the build process. You can check the location of the config
 file by running this command:

    $ openssl version -d
-    OPENSSLDIR: "/usr/local/ssl"
+    $ /usr/local/ssl


Please revert that change. People can have scripts that depend on this.

nhorman · 2024-05-29T13:01:37Z

@mattcaswell I think adding a version prefix to the key would be the less confusing way to go, then each installation would just use their respective key. Ostensibly if someone has release installed via the installer, then builds openssl manually the version string should have been bumped, and there will be no conflict. the corner case there is if someone builds from an old tag against an already installed version, but at that point they have the environment variables to allow redirection. Presumably, it also seems unlikely that someone interested in using the windows installer will also be building locally that often, given that the installer is being created so that users don't have to build openssl themselves

mattcaswell · 2024-05-29T13:39:55Z

@mattcaswell I think adding a version prefix to the key would be the less confusing way to go, then each installation would just use their respective key. Ostensibly if someone has release installed via the installer, then builds openssl manually the version string should have been bumped, and there will be no conflict. the corner case there is if someone builds from an old tag against an already installed version, but at that point they have the environment variables to allow redirection. Presumably, it also seems unlikely that someone interested in using the windows installer will also be building locally that often, given that the installer is being created so that users don't have to build openssl themselves

I disagree. I think it is very likely that people will end up a directly installed version of OpenSSL and a version of OpenSSL coming from somewhere else - quite possible of the same version (e.g. the static linking case where OpenSSL is being used in an application and maybe the end user is not even aware of it - plus a directly installed version). I think there is potential for huge confusion here and opportunity for things to be messed up. This is why I think having the registry keys thing being a build time option (off by default) is going to be essential. Having an installer specific context as an element in the key name in addition to the version would be a good thing too IMO to further reduce the possibility of problems.

FdaSilvaYY

2 small comments

FdaSilvaYY · 2024-05-29T16:04:34Z

crypto/defaults.c

+                          &keysize);
+    if (ret != ERROR_SUCCESS)
+        goto out;
+    if (ktype != REG_EXPAND_SZ)


REG_EXPAND_SZ means that registry value can contains some enviroment variables (like %PATH%), who will be expand and returned.
You need to accept REG_SZ key type too.

FdaSilvaYY · 2024-05-29T16:08:13Z

crypto/x509/x509_def.c

+static char x509_cert_dir[MAX_PATH];
+static char x509_cert_file[MAX_PATH];
+
+static void get_windows_default_path(char *pathname, char *suffix)


const char *suffix ?

nhorman self-assigned this May 21, 2024

nhorman linked an issue May 21, 2024 that may be closed by this pull request

Implement relocatable binaries for windows installer openssl/project#550

Open

2 tasks

mattcaswell requested changes May 21, 2024

View reviewed changes

nhorman added 3 commits May 23, 2024 15:14

Modify call sites that use OPENSSDIR/ENGINESDIR/MODULESDIR

52501e1

Make use of our new defaults api to get the build time macros from the windows registry where needed

Add some comments to the new internal api

d125bc4

nhorman force-pushed the read-openssldir-from-registry branch from 995c03b to d125bc4 Compare May 23, 2024 19:51

fixup! Add some comments to the new internal api

b40f326

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 23, 2024

mattcaswell requested changes May 24, 2024

View reviewed changes

crypto/x509/x509_def.c Show resolved Hide resolved

nhorman added 2 commits May 24, 2024 11:28

Various fixups/refactors

3fbd16a

fix up typing for get_windows_regdirs

1e38456

mattcaswell requested changes May 27, 2024

View reviewed changes

crypto/defaults.c Outdated Show resolved Hide resolved

crypto/defaults.c Outdated Show resolved Hide resolved

crypto/x509/x509_def.c Outdated Show resolved Hide resolved

nhorman added 4 commits May 27, 2024 10:50

fixup! fix up typing for get_windows_regdirs

18ea146

Add documentation

52f3d2f

Adding Registry test for windows

5618c96

Since windows builds now try to pull build time defines from the registry first, lets add a test to setup some dummy registry entries, and run the version command to see if they get pulled out properly

Fix up bare url in NOTES-WINDOWS.md

f4f5d6f

Added a url without the prorper guards on it

nhorman requested a review from mattcaswell May 27, 2024 19:07

nhorman mentioned this pull request May 27, 2024

Create and Deploy Windows installer for Release openssl/project#574

Open

mattcaswell requested changes May 28, 2024

View reviewed changes

FdaSilvaYY reviewed May 28, 2024

View reviewed changes

t8m requested changes May 29, 2024

View reviewed changes

mattcaswell mentioned this pull request May 29, 2024

Windows, world-writable config path (re: CVE-2019-5443) #24528

Open

FdaSilvaYY reviewed May 29, 2024

View reviewed changes

		@@ -0,0 +1,42 @@
		#! /usr/bin/env perl
		# Copyright 2015-2021 The OpenSSL Project Authors. All Rights Reserved.

read OPENSSLDIR from registry #24450

Are you sure you want to change the base?

read OPENSSLDIR from registry #24450

Conversation

nhorman commented May 21, 2024

mattcaswell left a comment

Choose a reason for hiding this comment

mattcaswell left a comment

Choose a reason for hiding this comment

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8m May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8m May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhorman May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdaSilvaYY May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented May 28, 2024

mattcaswell commented May 28, 2024

mattcaswell commented May 28, 2024

FdaSilvaYY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdaSilvaYY May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhorman commented May 29, 2024 • edited

mattcaswell commented May 29, 2024

FdaSilvaYY left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8m May 29, 2024 •

edited

t8m May 29, 2024 •

edited

nhorman May 28, 2024 •

edited

FdaSilvaYY May 29, 2024 •

edited

FdaSilvaYY May 28, 2024 •

edited

nhorman commented May 29, 2024 •

edited

FdaSilvaYY left a comment •

edited