Bug fixes #75

SudhakarKuppusamy1 · 2023-10-26T08:35:19Z

It has the following bug fixes

In verify/read, reading of timestamp from variable
In generate, trustedcadb allow only CA cert
misbehavior of verify with -p and write

when read variable from path, read timestamp from variable and show it as humanreadble form Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Moved read_timestamp to util and reads the timestamp when use -v Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

-w option available only when use -u with -p. otherwise throwing error message Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

In generating trustedcadb auth file, checked to have basicConstraints CA:TRUE for any certificate added to trustedcadb, else returned as invalid certificate. Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

issue: verify with -u, -p and -w, verify of variable auth file failed if variable not available in /sys/firmware/secvar/vars/. fix: allowing the variable auth file verify and write if variable not available in /sys/firmware/secvar/vars/. Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

erichte-ibm · 2023-10-26T20:25:27Z

backends/guest/common/util.c

+bool is_trustedcadb_variable(const char *variable_name)
+{
+	int len = strlen(variable_name);
+
+	if (memcmp(variable_name, TRUSTEDCADB_VARIABLE, len) == 0)
+		return true;
+
+	return false;
+}
+


bool is_trustedcadb_variable(const char *variable_name) { return !strcmp(variable_name, TRUSTEDCADB_VARIABLE); }

Acknowledged.

erichte-ibm · 2023-10-26T20:38:15Z

backends/guest/common/verify.c

-		return INVALID_FILE;
+		prlog(PR_WARNING, "WARNING: %s file does not have data\n", (char *)esl_file);


Why are we not returning an error code here? It seems like this can only be reached if we fail to allocate memory for the file, or fail to read from the file (is_file checks that it exists and can be opened, which does return an error code). In both cases, this should be signaled to the calling function that something went wrong.

Additional note: it appears get_auth_data() does return INVALID_FILE in its similar error else case, around ~L147

Acknowledged, we should return INVALID_FILE in this case as it will avoid unnecessary operations at a later stage.

erichte-ibm · 2023-10-26T20:43:09Z

backends/guest/common/util.c

+	if (esl_data == NULL)
+		return;
+
+	memcpy(&timestamp, esl_data + 1, TIMESTAMP_LEN - 1);


Why do we skip the first byte of the timestamp here?

Also, since this function does not do any check on the size of the buffer *esl_data points to, please include a comment that states that the caller must confirm the input buffer contains at least TIMESTAMP_LEN bytes.

From the structure of the plpks_get_variable under linux/arch/powerpc/platforms/pseries/plpks-secvar.c, it seems the esl_data size will always be fixed to align with timestamp_len, my question is that do we support variable key sizes for the secure variables?

This doesn't warrant a comment as per my understanding because we have used TIMESTAMP_LEN at multiple places and I don't think there can be a scenario where we will get esl_data_size less than the TIMESTAMP_LEN.

erichte-ibm · 2023-10-26T20:47:09Z

backends/guest/guest_svc_read.c

-			else
-				prlog(PR_WARNING, "WARNING: The %s database is empty.\n",
-				      variable_name);


I'm a little unclear on the change to the conditionals here. I assume a variable will be completely empty if it hadn't yet stored data or freshly reset, but otherwise it should at least contain a data size of TIMESTAMP_LEN. Removing the else condition would remove some error cases that should handled.

It seems like there are four conditions:

esl_data_size >= TIMESTAMP_LEN ➔ normal case, variable data at least (probably) contains a timestamp

0 < esl_data_size < TIMESTAMP_LEN ➔ malformed variable contents?

esl_data_size == 0 ➔ empty variable that hasn't been written to, might be worth printing that it is empty

esl_data_size == -1 ➔ an error that should be printed, although I don't think this can be hit if variable_from_path() call returns SUCCESS.

Note this appears again later in the function.

I don't think conditions 2 and 4 can ever occur? We can remove the esl_data_size != -1 check here.

erichte-ibm · 2023-12-19T20:37:58Z

Closing this PR, as these commits were merged in #77

SudhakarKuppusamy1 added 6 commits October 25, 2023 16:18

guest-read: read timestamp from variable

12cc2cd

when read variable from path, read timestamp from variable and show it as humanreadble form Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Guest/Verify: reading timestamp from variable when use -v

2058d5e

Moved read_timestamp to util and reads the timestamp when use -v Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Guest/Verify: -w option allowed when use -u with -p

7e9274c

-w option available only when use -u with -p. otherwise throwing error message Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Guest/generate: trustedcadb variable allow only CA certificates

2d7dcdb

In generating trustedcadb auth file, checked to have basicConstraints CA:TRUE for any certificate added to trustedcadb, else returned as invalid certificate. Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

Guest/verify: corrected typo error

e0fe9bf

Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

SudhakarKuppusamy1 force-pushed the bug_fixes branch from 04a1f81 to 9eaefe2 Compare October 26, 2023 09:37

Guest:fixed cppcheck failure

210aaea

Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

SudhakarKuppusamy1 force-pushed the bug_fixes branch from 9eaefe2 to 210aaea Compare October 26, 2023 10:06

erichte-ibm requested changes Oct 26, 2023

View reviewed changes

This was referenced Nov 17, 2023

guest: clean up and unify reading the timestamp from a variable buffer #76

Closed

Add missing patches from internal version, redux #77

Merged

erichte-ibm closed this Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes #75

Bug fixes #75

SudhakarKuppusamy1 commented Oct 26, 2023 •

edited

Loading

erichte-ibm Oct 26, 2023

soura15 Nov 28, 2023

erichte-ibm Oct 26, 2023

erichte-ibm Oct 26, 2023

soura15 Nov 28, 2023

erichte-ibm Oct 26, 2023

soura15 Nov 24, 2023 •

edited

Loading

soura15 Nov 28, 2023

erichte-ibm Oct 26, 2023

erichte-ibm Oct 26, 2023

soura15 Nov 28, 2023 •

edited

Loading

erichte-ibm commented Dec 19, 2023

		return INVALID_FILE;
		prlog(PR_WARNING, "WARNING: %s file does not have data\n", (char *)esl_file);

Bug fixes #75

Bug fixes #75

Conversation

SudhakarKuppusamy1 commented Oct 26, 2023 • edited Loading

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

soura15 Nov 24, 2023 • edited Loading

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

soura15 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

erichte-ibm commented Dec 19, 2023

SudhakarKuppusamy1 commented Oct 26, 2023 •

edited

Loading

soura15 Nov 24, 2023 •

edited

Loading

soura15 Nov 28, 2023 •

edited

Loading