Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes #75

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions backends/guest/common/generate.c
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,11 @@ int create_auth_msg(const uint8_t *new_esl, const size_t new_esl_size,
* @param buffer_size , length of buffer
* @param cert_data, the certificate data
* @param cert_data_size, the length of certificate data
* @param is_CA, CA certificate flag
* @return SUCCESS or err number
*/
int is_x509certificate(const uint8_t *buffer, const size_t buffer_size, uint8_t **cert_data,
size_t *cert_data_size)
size_t *cert_data_size, bool is_CA)
{
int rc;
uint8_t *cert = NULL;
Expand All @@ -434,7 +435,7 @@ int is_x509certificate(const uint8_t *buffer, const size_t buffer_size, uint8_t
return CERT_FAIL;
}

rc = validate_cert(cert, cert_size);
rc = validate_cert(cert, cert_size, is_CA);

if (rc) {
free(cert);
Expand All @@ -454,19 +455,21 @@ int is_x509certificate(const uint8_t *buffer, const size_t buffer_size, uint8_t
* @param buffer_size , length of buffer
* @param hash_funct, index of hash function information to use for ESL GUID,
* also helps in prevalation, if inform is '[c]ert' then this doesn't matter
* @param variable_name, name of the variable
* @param hash_data, the generated hash data, buffer should be allocated before calling
* @param hash_data_size, the length of hash data
* @param esl_guid, signature type of ESL
* @return SUCCESS or err number
*/
int get_hash_data(const uint8_t *buffer, const size_t buffer_size, enum signature_type hash_funct,
uint8_t *hash_data, size_t *hash_data_size)
const char *variable_name, uint8_t *hash_data, size_t *hash_data_size)
{
int rc = SUCCESS;
size_t crt_size = 0;
uint8_t *crt_der = NULL, *out_data = NULL;

rc = is_x509certificate(buffer, buffer_size, &crt_der, &crt_size);
rc = is_x509certificate(buffer, buffer_size, &crt_der, &crt_size,
is_trustedcadb_variable(variable_name));
if (rc == SUCCESS) {
rc = crypto_md_generate_hash(crt_der, crt_size, get_crypto_alg_id(hash_funct),
&out_data, hash_data_size);
Expand Down
6 changes: 0 additions & 6 deletions backends/guest/common/read.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,6 @@
#include "common/util.h"
#include "common/read.h"

void print_timestamp(timestamp_t t)
{
printf("%04d-%02d-%02d %02d:%02d:%02d UTC\n", t.year, t.month, t.day, t.hour, t.minute,
t.second);
}

sv_esl_t *extract_esl_signature_list(const uint8_t *buf, size_t buflen)
{
sv_esl_t *list = NULL;
Expand Down
31 changes: 31 additions & 0 deletions backends/guest/common/util.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,37 @@ const struct signature_type_info signature_type_list[] = {
static uint8_t append[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 };
static uint8_t replace[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };

/*
* check it whether given variable is trustedcadb
*/
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;
}

Comment on lines +44 to +53
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged.

void print_timestamp(timestamp_t t)
{
printf("%04d-%02d-%02d %02d:%02d:%02d UTC\n", t.year, t.month, t.day, t.hour, t.minute,
t.second);
}

void read_timestamp(const uint8_t *esl_data)
{
timestamp_t timestamp;

if (esl_data == NULL)
return;

memcpy(&timestamp, esl_data + 1, TIMESTAMP_LEN - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@soura15 soura15 Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

printf("\tTimestamp: ");
print_timestamp(timestamp);
}

/*
* creates the append header using append flag
*/
Expand Down
11 changes: 9 additions & 2 deletions backends/guest/common/validate.c
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ static int validate_single_esl(const uint8_t *esl_data, size_t esl_data_size, si
print_hex(cert, cert_size);
}
} else if (is_cert(sig_type))
rc = validate_cert(cert, cert_size);
rc = validate_cert(cert, cert_size, false);
else if (is_sbat(sig_type)) {
if (!validate_sbat(cert, cert_size)) {
prlog(PR_ERR, "ERROR: SBAT data format is invalid\n");
Expand Down Expand Up @@ -297,10 +297,11 @@ int validate_pkcs7(const uint8_t *cert_data, size_t cert_data_len)
*
* @param cert_data pointer to certificate data
* @param cert_data_len size of certtificate data
* @param is_CA, CA certificate flag
* @return CERT_FAIL if certificate had incorrect data
* @return SUCCESS if certificate is valid else
*/
int validate_cert(const uint8_t *cert_data, size_t cert_data_len)
int validate_cert(const uint8_t *cert_data, size_t cert_data_len, bool is_CA)
{
int rc;
crypto_x509_t *x509;
Expand Down Expand Up @@ -338,6 +339,12 @@ int validate_cert(const uint8_t *cert_data, size_t cert_data_len)
rc = validate_x509_certificate(x509);
if (rc)
prlog(PR_ERR, "ERROR: x509 certificate is invalid (%d)\n", rc);
else if (is_CA) {
if (!crypto_x509_is_CA(x509)) {
prlog(PR_ERR, "ERROR: it is not CA certificate\n");
rc = CERT_FAIL;
}
}

if (!rc && verbose >= PR_INFO)
rc = print_cert_info(x509);
Expand Down
35 changes: 22 additions & 13 deletions backends/guest/common/verify.c
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ static int update_variable(const char *variable_name, const uint8_t *auth_data,
prlog(PR_INFO, "\tappend update: %s\n\n", (append_update ? "True" : "False"));

if (*new_esl_data != NULL) {
read_timestamp(*new_esl_data);
rc = print_esl_buffer((*new_esl_data + TIMESTAMP_LEN),
(*new_esl_data_size - TIMESTAMP_LEN), variable_name);
if (rc != SUCCESS)
Expand All @@ -91,8 +92,10 @@ static int get_current_esl_data(const uint8_t *esl_file, uint8_t **current_esl_d
size_t buffer_size = 0;
uint8_t *buffer = NULL;

if (is_file((char *)esl_file) != SUCCESS)
if (is_file((char *)esl_file) != SUCCESS) {
prlog(PR_ERR, "ERROR: %s is not a valid file\n", (char *)esl_file);
return INVALID_FILE;
}

buffer = (uint8_t *)get_data_from_file((char *)esl_file, SIZE_MAX, &buffer_size);
if (buffer != NULL) {
Expand All @@ -101,15 +104,17 @@ static int get_current_esl_data(const uint8_t *esl_file, uint8_t **current_esl_d
free(buffer);
buffer = NULL;
buffer_size = 0;
} else if (buffer_size != TIMESTAMP_LEN) {
} else {
if (verbose >= PR_INFO)
read_timestamp(buffer);
rc = validate_esl(buffer + TIMESTAMP_LEN, buffer_size - TIMESTAMP_LEN);
if (rc != SUCCESS) {
free(buffer);
return rc;
}
}
} else
return INVALID_FILE;
prlog(PR_WARNING, "WARNING: %s file does not have data\n", (char *)esl_file);
Comment on lines -112 to +117
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


*current_esl_data = buffer;
*current_esl_data_size = buffer_size;
Expand All @@ -127,8 +132,10 @@ static int get_auth_data(const char *auth_file, uint8_t **auth_data, size_t *aut
size_t buffer_size = 0;
uint8_t *buffer = NULL;

if (is_file((char *)auth_file) != SUCCESS)
if (is_file((char *)auth_file) != SUCCESS) {
prlog(PR_ERR, "ERROR: %s is not a valid file\n", (char *)auth_file);
return INVALID_FILE;
}

buffer = (uint8_t *)get_data_from_file((char *)auth_file, SIZE_MAX, &buffer_size);
if (buffer != NULL) {
Expand All @@ -137,8 +144,10 @@ static int get_auth_data(const char *auth_file, uint8_t **auth_data, size_t *aut
free(buffer);
return rc;
}
} else
} else {
prlog(PR_WARNING, "WARNING: %s file does not have data\n", (char *)auth_file);
return INVALID_FILE;
}

*append_update = extract_append_header(buffer, buffer_size);
*auth_data = buffer;
Expand Down Expand Up @@ -463,17 +472,17 @@ int validate_variables_arguments(struct verify_args *args)
"<var_name 1> <var_auth_file 1>"
"...<var_name N> <var_auth_file N>\n");
return ARG_PARSE_FAIL;
} else if (args->current_variable_size != 0 && args->current_variable_size % 2) {
prlog(PR_ERR, "ERROR: current variable argument should be like -c "
"<var_name 1> <var_ESL_file 1>...<var_name N> <var_ESL_file N>\n");
return ARG_PARSE_FAIL;
}

if (args->current_variable_size != 0) {
if ((args->update_variable_size && args->variable_path == NULL) ||
args->current_variable_size != 0) {
if (args->write_flag) {
prlog(PR_ERR, "ERROR: cannot update files if current variable "
"files are given. remove -w\n");
return ARG_PARSE_FAIL;
} else if (args->current_variable_size % 2) {
prlog(PR_ERR, "ERROR: current variable argument should be like -c "
"<var_name 1> <var_ESL_file 1>"
"...<var_name N> <var_ESL_file N>\n");
prlog(PR_ERR,
"ERROR: cannot update files. remove -w. it is available when you use -u with -p\n");
return ARG_PARSE_FAIL;
}
}
Expand Down
11 changes: 7 additions & 4 deletions backends/guest/guest_svc_generate.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ static int generate_esl(const uint8_t *buffer, size_t buffer_size, struct genera
break;
}

rc = get_hash_data(buffer, buffer_size, hash_funct, hash_data, &hash_data_size);
rc = get_hash_data(buffer, buffer_size, hash_funct, args->variable_name, hash_data,
&hash_data_size);
if (rc != SUCCESS) {
prlog(PR_ERR, "ERROR: failed to generate hash from file\n");
break;
Expand All @@ -91,8 +92,9 @@ static int generate_esl(const uint8_t *buffer, size_t buffer_size, struct genera
esl_guid = get_uuid(hash_funct);
break;
case 'c':
if (is_x509certificate(buffer, buffer_size, &cert_data, &cert_data_size) !=
SUCCESS) {
rc = is_x509certificate(buffer, buffer_size, &cert_data, &cert_data_size,
is_trustedcadb_variable(args->variable_name));
if (rc != SUCCESS) {
prlog(PR_ERR, "ERROR: could not validate certificate\n");
break;
}
Expand Down Expand Up @@ -167,7 +169,8 @@ static int generate_sha256_hash(const uint8_t *data, size_t data_size, struct ge
rc = SUCCESS;
break;
case 'c':
rc = validate_cert(data, data_size);
rc = validate_cert(data, data_size,
is_trustedcadb_variable(args->variable_name));
break;
case 'e':
rc = validate_esl(data, data_size);
Expand Down
21 changes: 12 additions & 9 deletions backends/guest/guest_svc_read.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,12 @@ static int read_cert(const uint8_t *cert_data, const size_t cert_data_len, const
rc = validate_x509_certificate(x509);
if (rc)
prlog(PR_ERR, "ERROR: x509 certificate is invalid (%d)\n", rc);
else
else if (is_trustedcadb_variable(variable_name)) {
if (!crypto_x509_is_CA(x509)) {
prlog(PR_ERR, "ERROR: it is not CA certificate\n");
rc = CERT_FAIL;
}
} else
rc = print_cert_info(x509);

crypto_x509_free(x509);
Expand Down Expand Up @@ -261,12 +266,11 @@ static int read_path(const char *path, const int is_print_raw, const char *varia
if (rc == SUCCESS) {
if (is_print_raw || esl_data_size == DEFAULT_PK_LEN)
print_raw((char *)esl_data, esl_data_size);
else if (esl_data_size >= TIMESTAMP_LEN)
else if (esl_data_size >= TIMESTAMP_LEN) {
read_timestamp(esl_data);
rc = print_esl_buffer(esl_data + TIMESTAMP_LEN,
esl_data_size - TIMESTAMP_LEN, variable_name);
else
prlog(PR_WARNING, "WARNING: The %s database is empty.\n",
variable_name);
Comment on lines -267 to -269
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. esl_data_size >= TIMESTAMP_LEN ➔ normal case, variable data at least (probably) contains a timestamp
  2. 0 < esl_data_size < TIMESTAMP_LEN ➔ malformed variable contents?
  3. esl_data_size == 0 ➔ empty variable that hasn't been written to, might be worth printing that it is empty
  4. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this appears again later in the function.

Copy link

@soura15 soura15 Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if (esl_data != NULL)
free(esl_data);
Expand All @@ -289,13 +293,12 @@ static int read_path(const char *path, const int is_print_raw, const char *varia
(esl_data_size == DEFAULT_PK_LEN &&
strcmp(defined_sb_variables[i], PK_VARIABLE) == 0))
print_raw((char *)esl_data, esl_data_size);
else if (esl_data_size >= TIMESTAMP_LEN)
else if (esl_data_size >= TIMESTAMP_LEN) {
read_timestamp(esl_data);
rc = print_esl_buffer(esl_data + TIMESTAMP_LEN,
esl_data_size - TIMESTAMP_LEN,
defined_sb_variables[i]);
else
prlog(PR_WARNING, "WARNING: The %s database is empty.\n",
defined_sb_variables[i]);
}

if (esl_data != NULL)
free(esl_data);
Expand Down
2 changes: 1 addition & 1 deletion backends/guest/guest_svc_validate.c
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ int guest_validation_command(int argc, char *argv[])

switch (args.input_form) {
case CERT_FILE:
rc = validate_cert(buff, size);
rc = validate_cert(buff, size, false);
break;
case ESL_FILE:
rc = validate_esl(buff, size);
Expand Down
4 changes: 2 additions & 2 deletions backends/guest/guest_svc_verify.c
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ int guest_verify_command(int argc, char *argv[])
"manually set current vars to be contents of CURRENT VAR LIST (see below for format)" },
{ "write", 'w', 0, 0,
"if successful, submit the update to be commited upon reboot."
" quivalent to `secvarctl -m guest write`" },
" equivalent to `secvarctl -m guest write`" },
{ 0, 'u', "{UPDATE LIST}", OPTION_HIDDEN,
"set update variables (see below for format)" },
{ "help", '?', 0, 0, "Give this help list", 1 },
Expand All @@ -130,7 +130,7 @@ int guest_verify_command(int argc, char *argv[])
" Where <var_name> is one of Guest secure boot variable"
" and <var_auth_file> is a properly generated authenticated variable file "
"that is"
" signed by a current variable with priviledges to approve the update\n\n"
" signed by a current variable with privileges to approve the update\n\n"
"CURRENT_VAR_LIST:\nOptional, only used when -c is used. formatted as:"
" ' -c <var_name 1> < var_ESL_file 1>...<var_name N> <var_ESL_file N> '"
" Where <var_name> is one of Guest secure boot variable and"
Expand Down
6 changes: 4 additions & 2 deletions backends/guest/include/common/generate.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,11 @@ int create_auth_msg(const uint8_t *new_esl, const size_t new_esl_size,
* @param buffer_size , length of buffer
* @param cert_data, the certificate data
* @param cert_data_size, the length of certificate data
* @param is_CA, CA certificate flag
* @return SUCCESS or err number
*/
int is_x509certificate(const uint8_t *buffer, const size_t buffer_size, uint8_t **cert_data,
size_t *cert_data_size);
size_t *cert_data_size, bool is_CA);

/*
* generate the hash data using input data
Expand All @@ -139,12 +140,13 @@ int is_x509certificate(const uint8_t *buffer, const size_t buffer_size, uint8_t
* @param buffer_size , length of buffer
* @param hash_funct, index of hash function information to use for ESL GUID,
* also helps in prevalation, if inform is '[c]ert' then this doesn't matter
* @param variable_name, name of the variable
* @param hash_data, the generated hash data, should already be allocated to hold hash
* @param hash_data_size, the length of hash data
* @param esl_guid, signature type of ESL
* @return SUCCESS or err number
*/
int get_hash_data(const uint8_t *buffer, const size_t buffer_size, enum signature_type hash_funct,
uint8_t *hash_data, size_t *hash_data_size);
const char *variable_name, uint8_t *hash_data, size_t *hash_data_size);

#endif
10 changes: 10 additions & 0 deletions backends/guest/include/common/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#define KEK_VARIABLE (char *)"KEK"
#define KEK_LEN 3
#define SBAT_VARIABLE (char *)"sbat"
#define TRUSTEDCADB_VARIABLE (char *)"trustedcadb"

static const uuid_t PKS_CERT_DELETE_GUID = { { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 } };
Expand Down Expand Up @@ -53,6 +54,15 @@ struct signature_type_info {

extern const struct signature_type_info signature_type_list[];

/*
* check it whether given variable is trustedcadb
*/
bool is_trustedcadb_variable(const char *variable_name);

void print_timestamp(timestamp_t t);

void read_timestamp(const uint8_t *esl_data);

/*
* creates the append header using append flag
*/
Expand Down
3 changes: 2 additions & 1 deletion backends/guest/include/common/validate.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,10 @@ int validate_esl(const uint8_t *esl_data, size_t esl_data_len);
*
* @param certBuf pointer to certificate data
* @param buflen length of certBuf
* @param is_CA, CA certificate flag
* @return CERT_FAIL if certificate had incorrect data
* @return SUCCESS if certificate is valid
*/
int validate_cert(const uint8_t *cert_data, size_t cert_data_len);
int validate_cert(const uint8_t *cert_data, size_t cert_data_len, bool is_CA);

#endif