Disable loading of unprotected credentials from PKCS#12 files. by martin-barta-sie · Pull Request #31 · siemens/libsecutils

martin-barta-sie · 2022-08-15T13:27:02Z

containers with empty password are rejected if a nonempty
password is provided as parameter (i.e. the empty password can no longer
be used to circumvent UTA-protection)
containers that don't have private keys encrypted with the same algorithm
that is used by "storage" functions (e.g. CREDENTIALS_save_dv) are
rejected

- containers with **empty password** are **rejected** if a nonempty password is provided as parameter (i.e. the empty password can no longer be used to circumvent UTA-protection) - containers that don't have private keys encrypted with the same algorithm that is used by "storage" functions (e.g. `CREDENTIALS_save_dv`) are **rejected**

DDvO

Update: Meanwhile I learned from Benjamin what that background of these checks is. So okay to have them, but not by default.

Sorry to say, but I see little sense in the two restrictions added in this PR.

What does it help if an application refuses to load certain types of PKCS#12 files?
The files are in storage anyway, so an attacker can just use them and load their contents using some other software (such as vanilla OpenSSL), learn their contents, and potentially do havoc with them.

Moreover, the new checks are are not acceptable as mandatory checks because they (needlessly) restrict the general usability of PKCS#12 loading, so if you really want to keep them, make them somehow optional.

DDvO · 2022-08-26T10:17:06Z

src/storage/files.c

    {
-        LOG(FL_WARN, "Unencrypted PKCS#12 file%s%s", for_str, desc_str);
-        pass = "";
+        LOG(FL_WARN, "Rejecting unencrypted PKCS#12 file%s%s", for_str, desc_str);


FL_WARN -> FL_ERR, if this case is really considered an error, but see below.

DDvO · 2022-08-26T10:22:10Z

src/storage/files.c

-        LOG(FL_WARN, "Unencrypted PKCS#12 file%s%s", for_str, desc_str);
-        pass = "";
+        LOG(FL_WARN, "Rejecting unencrypted PKCS#12 file%s%s", for_str, desc_str);
+        return 0;


This PR does not really do what is given it is description.
It rejects PKCS#12 not having a password even if the password provided by the caller is empty or no password was given by the caller. This is too much of a restriction.
In the cases just described, you can give a warning if wanted, but do not stop with an error. Otherwise, this library can no more be used to handle PKCS#12 files that (possibly for a good reason) are not password-protected.

DDvO · 2022-08-26T10:26:38Z

src/storage/files.c

+
+    pass = tpass;
+
+    if(0 is_eq algo_allowed(p12, pass, len, NID_aes_256_cbc))


I wonder about the strange - and not very readable - way of checking a Boolean (return) value here and at many other places. Better use if(not algo_allowed(...))

Here you hard-coded NID_aes_256_cbc, but our PR description says
"the same algorithm that is used by "storage" functions (e.g. CREDENTIALS_save_dv)".
Instead, you should define a symbolic name for this algorithm and use it in the code where appropriate in order to prevent (future) inconsistencies.

The algo check must not be made mandatory because otherwise general usability of PKCS#12 loading is hampered.

DDvO · 2022-08-26T10:37:16Z

src/storage/files.c

+   desired algorithm. */
+/* Iterates over all keybags in the file but only looks inside Shrouded keybags. Returns 1 if the desired algorithm
+   and no other was found, 0 otherwise. */
+static int algo_allowed(const PKCS12* p12, const char* pass, int passlen, int desired_algo_nid)


better: static bool algo_allowed(...) to point out that the result is Boolean
and use false and true rather than 0 and 1 for Boolean values below.

DDvO · 2022-08-26T10:39:14Z

src/storage/files.c

+        {
+            continue;
+        }
+        if(!bags)


better: 0 is_eq bags

DDvO · 2022-08-26T11:09:26Z

src/storage/files.c

+            found_desired_algo = 1;
+            continue;
+        }
+        sk_PKCS7_pop_free(auth_safes, PKCS7_free);


simpler: goto line 482.

DDvO · 2022-08-26T11:17:07Z

src/storage/files.c


 static int password_callback(char* buf, int bufsiz, int verify, void* cb_tmp)
 {
+    if(0 == buf || bufsiz <= 0 || 0 == cb_tmp)


Why you you forbid 0 == cb_tmp ?

Better move the checks on buf and bufsiz below into if(password not_eq 0) {
because they are needed only in this case.

DDvO · 2022-08-26T11:22:57Z

src/storage/files.c

+}
+
+
+static int bag_get_pkey_algo(const PKCS12_SAFEBAG *bag, const char *pass, int passlen)


pass and passlen are unused, so remove them from the function parameter list.

DDvO · 2022-08-26T11:23:10Z

src/storage/files.c

+}
+
+
+static int bags_get_pkey_algo(const STACK_OF(PKCS12_SAFEBAG) *bags, const char *pass, int passlen, int desired_algo_nid)


pass and passlen are effectively unused, so remove them from the function parameter list.

DDvO · 2022-08-26T11:27:56Z

src/storage/files.c

+        return encnid;
+    }
+
+    return NID_undef;


Is it really justified to return ID_undef for all other cases, i.e. pbenid != NID_pbes2?
If you, add a comment why.

DDvO · 2022-08-29T08:31:42Z

I suggest making the implementation and use of the two new checks on PKCS#12 files dependent on a compile-time flag that is not set by default but set for S2L2 Linux, e.g.

#ifdef SECUTILS_PKCS12_STRICT

martin-barta-sie mentioned this pull request Aug 15, 2022

Merge our current version of the code with the OSS libsecutils. #27

Closed

DDvO requested changes Aug 26, 2022

View reviewed changes

DDvO changed the title ~~Disable loading of unprotected credentials.~~ Disable loading of unprotected credentials from PKCS#12 files. Sep 2, 2022


		pass = tpass;

		if(0 is_eq algo_allowed(p12, pass, len, NID_aes_256_cbc))

		}


		static int bag_get_pkey_algo(const PKCS12_SAFEBAG bag, const char pass, int passlen)

		}


		static int bags_get_pkey_algo(const STACK_OF(PKCS12_SAFEBAG) bags, const char pass, int passlen, int desired_algo_nid)

Conversation

martin-barta-sie commented Aug 15, 2022

Uh oh!

DDvO left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DDvO left a comment •

edited

Loading