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.

Sign up for GitHub

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

Parse SBAT variable data and perform verification #3

Open
wants to merge 1 commit into
base: parse-sbat-section
Choose a base branch
from
Open

Parse SBAT variable data and perform verification #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions include/sbat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@

#include "shim.h"

struct sbat_var {
const CHAR8 *component_generation;
const CHAR8 *component_name_size;
const CHAR8 *component_name;
struct sbat_var *next;
};

struct sbat_entry {
const CHAR8 *component_name;
const CHAR8 *component_generation;
Expand All @@ -22,6 +29,15 @@ struct sbat {
struct sbat_entry **entries;
};

struct sbat_var* parse_sbat_var();
Copy link

Choose a reason for hiding this comment

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

This should be parse_sbat_var(void).


struct sbat_var* add_entry(struct sbat_var*, const CHAR8 *comp_gen,
const CHAR8 *comp_name_size, const CHAR8 *comp_name);

struct sbat_var* new_entry(const CHAR8 *comp_gen, const CHAR8 *comp_name_size,
const CHAR8 *comp_name);
Comment on lines +34 to +38
Copy link

Choose a reason for hiding this comment

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

I don't think these need to be exposed?

EFI_STATUS verify_sbat (struct sbat *sbat, struct sbat_var *sbat_var);

EFI_STATUS parse_sbat(char *sbat_base, size_t sbat_size, char *buffer,
struct sbat *sbat);

Expand Down
18 changes: 16 additions & 2 deletions pe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1043,6 +1043,7 @@ handle_image (void *data, unsigned int datasize,
unsigned int i;
struct sbat sbat = { 0 };
struct sbat_entry *entry = NULL;
struct sbat_var *var = NULL;

if (SBATBase && SBATSize) {
char *sbat_data;
Expand Down Expand Up @@ -1074,17 +1075,30 @@ handle_image (void *data, unsigned int datasize,
entry->vendor_version,
entry->vendor_url);
}
var = parse_sbat_var();
if (var == NULL)
console_print(L"SBAT variable not read");
} else {
perror(L"SBAT data not found\n");
return EFI_UNSUPPORTED;
}

efi_status = verify_buffer(data, datasize,
&context, sha256hash, sha1hash);

if (sbat.entries && var )
efi_status = verify_sbat(&sbat, var);
/* Add EFI_SECURITY_VIOLATION if variable is set,
and binary has no SBAT section ?*/
if (sbat.entries)
for (i = 0; i < sbat.size; i++)
FreePool(sbat.entries[i]);
if (var) {
Copy link

Choose a reason for hiding this comment

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

The "if" here isn't needed.

while (var != NULL) {
struct sbat_var* tmp;
tmp = var;
var = var->next;
FreePool(tmp);
}
}

if (EFI_ERROR(efi_status)) {
if (verbose)
Expand Down
95 changes: 95 additions & 0 deletions sbat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

#include "sbat.h"
#include <string.h>

CHAR8 *
Copy link

Choose a reason for hiding this comment

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

This should be static.

get_sbat_field(CHAR8 *current, CHAR8 *end, const CHAR8 ** field, char delim)
Expand Down Expand Up @@ -120,4 +121,98 @@ EFI_STATUS parse_sbat(char *sbat_base, size_t sbat_size, char *buffer,
return efi_status;
}

EFI_STATUS verify_sbat (struct sbat *sbat, struct sbat_var *sbat_var_root)
Copy link

Choose a reason for hiding this comment

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

This can be static. Also please run "clang-format --style=file -i include/sbat.h sbat.c".

{
unsigned int i;
struct sbat_entry *entry = NULL;
for (i = 0; i < sbat->size; i++) {
entry = sbat->entries[i];
struct sbat_var *sbat_var_entry = sbat_var_root;
while (sbat_var_entry != NULL) {
if (strcmp(entry->component_name,sbat_var_entry->component_name) == 0) {
dprint(L"component %a has a matching SBAT variable entry, verifying\n", entry->component_name);
/* atoi returns zero for failed conversion, so essentially
badly parsed component_generation will be treated as zero
*/
if (atoi(entry->component_generation) < atoi(sbat_var_entry->component_generation)) {
Copy link

Choose a reason for hiding this comment

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

Hrm. Not having strtoul() available might be a good argument for saying we always start them at 1. @martinezjavier @jsetje, yall have thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry @vathpela, I missed this comment before. That's a good question, maybe we could add strtoul() to shim ? Or use the one provided by Cryptlib ?

dprint(L"component's %a generation: %d. Conflicts with SBAT variable generation %d\n",
entry->component_name,
atoi(entry->component_generation),
Copy link

Choose a reason for hiding this comment

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

If we're going to do this twice, just put them in some locals.

atoi(sbat_var_entry->component_generation));
LogError(L"image did not pass SBAT verification\n");
return EFI_SECURITY_VIOLATION;
}
}
sbat_var_entry = sbat_var_entry->next;
}
}
dprint(L"all entries from SBAT section verified\n");
return EFI_SUCCESS;
}

static BOOLEAN is_utf8_bom(CHAR8 *buf)
{
unsigned char bom[] = { 0xEF,0xBB,0xBF };
return !!CompareMem(buf, bom, MIN(sizeof(bom), sizeof(buf)));
Copy link

Choose a reason for hiding this comment

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

I'd rather see != 0 than !!...

}

struct sbat_var* new_entry(const CHAR8 *comp_gen, const CHAR8 *comp_name_size,
const CHAR8 *comp_name)
{
struct sbat_var *new_entry = AllocatePool(sizeof(*new_entry));
Copy link

Choose a reason for hiding this comment

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

Needs to be error checked.

new_entry->next = NULL;
new_entry->component_generation = comp_gen;
new_entry->component_name_size = comp_name_size;
new_entry->component_name = comp_name;
return new_entry;
}

struct sbat_var* add_entry(struct sbat_var *n, const CHAR8 *comp_gen, const CHAR8 *comp_name_size,
const CHAR8 *comp_name)
Comment on lines +170 to +171
Copy link

Choose a reason for hiding this comment

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

This can be static as well.

Also I'd really rather something like:

static int
add_entry(list_t *list, const char *const CHAR8 *comp_gen, const CHAR8 *comp_name_size,
	  const CHAR8 *comp_name)

which follows the -1 on error, 0 on success pattern. (See my broader comment on list_t.)

{
if ( n == NULL )
return NULL;
while (n->next)
n = n->next;
return (n->next = new_entry(comp_gen, comp_name_size, comp_name));
}

struct sbat_var* parse_sbat_var()
{
UINT8 *data = 0;
UINTN datasize;
EFI_STATUS efi_status;

efi_status = get_variable(L"SBAT", &data, &datasize, SHIM_LOCK_GUID);
if (EFI_ERROR(efi_status)) {
return NULL;
}

struct sbat_var *root = new_entry((CHAR8 *)"0",(CHAR8 *)"0",(CHAR8 *)"entries");
Copy link

Choose a reason for hiding this comment

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

This feels really weird to me, and we don't need it if we use the typical generic list implementation instead.

struct sbat_var *nodename = root;
CHAR8 *start = (CHAR8 *) data;
CHAR8 *end = (CHAR8 *) data + datasize;
while ((*end == '\r' || *end == '\n') && end < start)
end--;
*end = '\0';
if (is_utf8_bom(start))
start += 3;
dprint(L"SBAT variable data:\n");
while (start[0] != '\0') {
const CHAR8 *comp_name_size, *comp_gen, *comp_name;

start = get_sbat_field(start, end, &comp_gen, ',');

start = get_sbat_field(start, end, &comp_name_size, ',');

start = get_sbat_field(start, end, &comp_name, '\n');
Comment on lines +204 to +208
Copy link

Choose a reason for hiding this comment

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

These all need to be error checked.

I think they should also all be looking for ',', never newline, as that allows us to treat trailing fields as comments, though it doesn't really make much difference for this specific usage due to our size constraints - but it would allow you to put it in a loop with a single error handler.


dprint(L"component %a with generation %a\n",comp_name, comp_gen);

add_entry(nodename, comp_gen, comp_name_size, comp_name);
Copy link

Choose a reason for hiding this comment

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

Needs error handling.

nodename = nodename->next;
}
return root;
}

// vim:fenc=utf-8:tw=75:noet