Track container/guestOS CAs as symlink #332

licosec · 2022-10-25T15:20:07Z

We create a symlink to the CA which successfully verified a container/guestos for the first time. If the container/guestos is updated, only updates signed with the same CA will be accepted.

Apart from that, there are two small bug fixes.

daemon/cmld.c

k0ch4lo · 2022-11-16T20:18:07Z

daemon/cmld.c

@@ -418,13 +418,15 @@ cmld_container_new(const char *store_path, const uuid_t *existing_uuid, const ui
 	/********************************
 	 * Translate High Level Config into low-level parameters for internal
 	 * constructor */
+	char *ca_symlink_path = guestos_get_ca_file_new(images_dir);


i'd suggest moving this up next to "config_filename = " and adding a DEBUG statement as for config_filename and images_dir

k0ch4lo · 2022-11-16T20:27:03Z

daemon/cmld.c

 	container_config_t *conf = container_config_new(config_filename, config, config_len, sig,
-							sig_len, cert, cert_len);
+							sig_len, cert, cert_len, ca_symlink_path);


instead of adding a parameter to container_config_new requireing on memory allocation on each calling funtion,
i'd suggest to generate the CA path in container_config_new using the container_dir+UUID path ('prefix' variable)

k0ch4lo · 2022-11-16T20:30:57Z

daemon/container_config.c

@@ -176,7 +177,7 @@ container_config_verify(const char *prefix, uint8_t *conf_buf, size_t conf_len,

 container_config_t *
 container_config_new(const char *file, const uint8_t *buf, size_t len, uint8_t *sig_buf,
-		     size_t sig_len, uint8_t *cert_buf, size_t cert_len)
+		     size_t sig_len, uint8_t *cert_buf, size_t cert_len, const char *ca_symlink)


k0ch4lo · 2022-11-16T20:32:14Z

daemon/container_config.c

@@ -111,7 +111,7 @@ container_config_proto_to_usb_type(ContainerUsbType type)
 */
 static bool
 container_config_verify(const char *prefix, uint8_t *conf_buf, size_t conf_len, uint8_t *sig_buf,
-			size_t sig_len, uint8_t *cert_buf, size_t cert_len)
+			size_t sig_len, uint8_t *cert_buf, size_t cert_len, const char *ca_symlink)


like for config_config_new, 'prefix" contains the necessary information, i'd suggest not to extend the function signature here

k0ch4lo · 2022-11-16T20:44:34Z

daemon/guestos_mgr.c

+
+		//read Path to CA from old_os_ca_symlink
+		if (readlink(old_os_ca_symlink, old_os_ca_path, sizeof(old_os_ca_path)) < 0) {
+			ERROR("Symlink destination of %s could not been read. Creating new symlink not possible.",


severe ERROR, new GuestOS installation should be aborted in this case

(+ add mem_frees before early return)

k0ch4lo · 2022-11-16T20:46:59Z

daemon/guestos_mgr.c

+			//update ca symlink name
+			if (symlink(old_os_ca_path, new_os_ca_symlink)) {
+				ERROR("FAILED to create symlink %s -> %s.", new_os_ca_symlink,
+				      old_os_ca_path);


severe error, see above

k0ch4lo · 2022-11-16T20:47:44Z

daemon/guestos_mgr.c

@@ -518,9 +542,23 @@ guestos_mgr_push_config(unsigned char *cfg, size_t cfglen, unsigned char *sig, s
 		audit_log_event(NULL, FSA, CMLD, GUESTOS_MGMT, "push-os-missing-certificate",
 				guestos_basepath, 0);
 	} else {
-		res = crypto_verify_buf(cfg, cfglen, sig, siglen, cert, certlen,
+		//Construct os to get CA filepath


k0ch4lo · 2022-11-16T20:52:27Z

daemon/guestos_mgr.c

@@ -518,9 +542,23 @@ guestos_mgr_push_config(unsigned char *cfg, size_t cfglen, unsigned char *sig, s
 		audit_log_event(NULL, FSA, CMLD, GUESTOS_MGMT, "push-os-missing-certificate",
 				guestos_basepath, 0);
 	} else {
-		res = crypto_verify_buf(cfg, cfglen, sig, siglen, cert, certlen,
+		//Construct os to get CA filepath
+		guestos_t *os = guestos_new_from_buffer(cfg, cfglen, guestos_basepath);


This parses unverified data => buffer can be parsed after successful signature verification only.
We should either stick with current approach in the SCD and do symlink processing based on the returned CA or pass the CA path if already exiting => let's discuss this

k0ch4lo · 2022-11-16T20:54:16Z

daemon/scd.proto

@@ -159,5 +160,6 @@ message TokenToDaemon {

 	optional bytes device_csr = 40;		// device csr in response to PULL_CSR
 	optional bytes hash_value = 50;		// hash_value in reponse to CRYPTO_HASH_FILE
+


quitschbo · 2023-01-09T07:24:36Z

can you be a little bit more verbose in the commit messages. The commit heading should, be short
and contain the main subsystems which are affected in form of:
daemon/cmld: <heading> if you contributed code to daemon/cmld.c

Thnx,
Michael

…rom first successful verification as symlink After the first successful signature verification of a guestos or container, a symlink pointing to the CA used for verification is created. In case of an update to the entity, it is checked whether the CA remains unchanged. If the update was signed using a different PKI, the update is rejected, even if the CA is known to the system. To achieve this, crypto_verify_result_t is extended to contain the path of the CA used for verification. The path is provided by scd during signature verification. If there has not been a symlink for the container/guestos under verification, cmld creates a symlink to the path given in crypto_verify_result_t. If there already is a symlink available, cmld compares the destination of the symlink against the CA given in the result. If they do not match, the verification is considered failed. Signed-off-by: Corinna Lingstädt <[email protected]>

Signed-off-by: Corinna Lingstädt <[email protected]>

k0ch4lo · 2023-01-26T16:13:25Z

daemon/crypto.c

@@ -223,6 +223,8 @@ crypto_cb(int fd, unsigned events, event_io_t *io, void *data)

 	TRACE("Received message crypto msg from SCD");

+	crypto_verify_result_t verify_result = { .code = VERIFY_ERROR, .matched_ca = str_new("") };


imho .matched_ca should only be initialized if a matching CA is actually found.
If there was a VERIFY_ERROR it's ok for .matched_ca to be NULL.

k0ch4lo · 2023-01-26T16:15:24Z

daemon/crypto.c

+				ERROR("TokenToDaemon ca_path null after successful verification");
+				break;
+			} else {
+				str_append(verify_result.matched_ca, msg->ca_path);


see above, would use str_new here in case a matching CA was returned

k0ch4lo · 2023-01-26T16:23:59Z

daemon/crypto.c

@@ -531,7 +541,7 @@ crypto_verify_file_block(const char *datafile, const char *sigfile, const char *
 	ASSERT(sigfile);
 	ASSERT(certfile);

-	crypto_verify_result_t ret = VERIFY_ERROR;
+	crypto_verify_result_t ret = { .code = VERIFY_ERROR, .matched_ca = str_new("") };


see above, should imho only be initialized if a CA was actually returned

k0ch4lo · 2023-01-26T16:26:08Z

daemon/crypto.c

-		ret = crypto_verify_result_from_proto(msg->code);
+		ret.code = crypto_verify_result_from_proto(msg->code);
+		if (msg->ca_path != NULL) {
+			str_append(ret.matched_ca, msg->ca_path);


see above, would use str_new here

k0ch4lo · 2023-01-26T16:26:44Z

daemon/crypto.c

@@ -578,7 +595,7 @@ crypto_verify_buf_block(unsigned char *data_buf, size_t data_buf_len, unsigned c
 	ASSERT(sig_buf);
 	ASSERT(cert_buf);

-	crypto_verify_result_t ret = VERIFY_ERROR;
+	crypto_verify_result_t ret = { .code = VERIFY_ERROR, .matched_ca = str_new("") };


k0ch4lo · 2023-01-26T17:15:06Z

scd/control.c

@@ -119,6 +121,9 @@ scd_control_verify_cert_ca_cb(const char *path, const char *file, void *data)
 	} else {
 		INFO("Certificate validation succeeded using ca: %s", ca_file);
 		cb_data->verified = true;
+		//Give back path to successful CA
+		str_append(cb_data->ca_path, ca_file);


should this be str_new?

k0ch4lo · 2023-01-26T17:18:47Z

scd/control.c

 {
+	ASSERT(verify_ca_file);


should return an error code instead of ASSERTing (analogously to other pointer arguments passed to ssl_verify_cert below)

k0ch4lo · 2023-01-26T17:19:24Z

scd/control.c

 	// At first, we explicitly assume that the file to be verified is a software update file,
 	// and we thus use the software signing root CA.
 	if ((ret = ssl_verify_certificate(verify_cert_file, SSIG_ROOT_CERT, ignore_time)) == 0) {
 		verified = true;
+
+		//track matched CA
+		str_append(verify_ca_file, SSIG_ROOT_CERT);


imho we should return a pointer (possibly via a str_t** parameter)

k0ch4lo · 2023-01-26T17:19:57Z

scd/control.c

@@ -166,6 +177,7 @@ scd_control_handle_verify(const char *verify_data_file, const char *verify_sig_f

 	// Retry with Local CA
 	if ((ret = ssl_verify_certificate(verify_cert_file, LOCALCA_ROOT_CERT, ignore_time)) == 0) {
+		str_append(verify_ca_file, LOCALCA_ROOT_CERT);


k0ch4lo · 2023-01-26T17:24:20Z

scd/control.c

@@ -529,6 +545,12 @@ scd_control_handle_message(const DaemonToToken *msg, int fd)
 			unlink(tmp_cert_file);
 			mem_free0(tmp_cert_file);
 		}
+		if (out.ca_path) {


plz verify value assigned by TOKEN_TO_DAEMON__INIT to ensure NULL is assigned to out.ca_path

CLAassistant · 2023-11-30T13:07:11Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

licosec force-pushed the symlink-cas branch from 4e9138f to 529a6b5 Compare October 28, 2022 09:03

licosec force-pushed the symlink-cas branch 2 times, most recently from 64179a6 to 5fce586 Compare November 10, 2022 14:48

k0ch4lo reviewed Nov 16, 2022

View reviewed changes

licosec force-pushed the symlink-cas branch from 9616c6e to 1785c37 Compare January 11, 2023 09:09

licosec added 3 commits January 11, 2023 15:42

cmld.c: Bugfix: do not check if guestos is from c0 if in hosted mode

ed0c5ac

Signed-off-by: Corinna Lingstädt <[email protected]>

cmld.c: Fix copy paste error

48ff80e

Signed-off-by: Corinna Lingstädt <[email protected]>

licosec force-pushed the symlink-cas branch from 1785c37 to 48ff80e Compare January 11, 2023 14:43

k0ch4lo reviewed Jan 27, 2023

View reviewed changes

k0ch4lo changed the base branch from dunfell to kirkstone April 21, 2023 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track container/guestOS CAs as symlink #332

Track container/guestOS CAs as symlink #332

licosec commented Oct 25, 2022

k0ch4lo Nov 16, 2022

k0ch4lo Nov 16, 2022

k0ch4lo Nov 16, 2022

k0ch4lo Nov 16, 2022

k0ch4lo Nov 16, 2022

licosec Jan 11, 2023

k0ch4lo Nov 16, 2022

licosec Jan 11, 2023

k0ch4lo Nov 16, 2022

k0ch4lo Nov 16, 2022

k0ch4lo Nov 16, 2022

quitschbo commented Jan 9, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

k0ch4lo Jan 26, 2023

CLAassistant commented Nov 30, 2023

		@@ -159,5 +160,6 @@ message TokenToDaemon {

		optional bytes device_csr = 40; // device csr in response to PULL_CSR
		optional bytes hash_value = 50; // hash_value in reponse to CRYPTO_HASH_FILE

		@@ -223,6 +223,8 @@ crypto_cb(int fd, unsigned events, event_io_t io, void data)

		TRACE("Received message crypto msg from SCD");

		crypto_verify_result_t verify_result = { .code = VERIFY_ERROR, .matched_ca = str_new("") };

Track container/guestOS CAs as symlink #332

Are you sure you want to change the base?

Track container/guestOS CAs as symlink #332

Conversation

licosec commented Oct 25, 2022

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

quitschbo commented Jan 9, 2023

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

CLAassistant commented Nov 30, 2023