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

CreateVault: Fix sign.command and update signing docs #568

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

mikebeaton
Copy link
Contributor

@mikebeaton mikebeaton commented Nov 24, 2024

Fixes acidanthera/bugtracker#2443.

Fix operation of sign.command when printable characters occur immediately before =BEGIN OC VAULT=. strings finds the location of the first printable character in such a sequence. hexdump automatically works on 16 byte boundaries, so still finds the correct offset.

Use BASE_ALIGNAS to enforce the required alignment, which will not be correct on all builds unless enforced (note alignment is required purely for locating the structure correctly from external script as above, not for reading in C).

Remove struct packing, since structs had better be naturally packed anyway (if not, reading from them without arbitrary-alignment-safe code, as we do, would be undefined behaviour). Add static asserts to confirm correct naturally packed layout.

Update the docs to refer to sign.command rather than to include the signing commands explicitly - otherwise we have two places that need to be kept in sync for signing commands, and note that the commands in the two places were already out of sync.

@mikebeaton mikebeaton force-pushed the create-vault branch 12 times, most recently from 8359b3f to 79eee8b Compare November 26, 2024 02:11
Comment on lines 44 to 47
STATIC_ASSERT (
sizeof (OC_RSA_PUBLIC_KEY_2048) == sizeof (((OC_RSA_PUBLIC_KEY_2048 *)NULL)->Hdr) + sizeof (((OC_RSA_PUBLIC_KEY_2048 *)NULL)->Data),
"OC_RSA_PUBLIC_KEY_2048 is not naturally packed"
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, maybe I should add another(?) assert that the size of this is 528, as expected by the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, actually just added this.

Copy link
Member

Choose a reason for hiding this comment

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

Actually that should be the only ASSERT. Yours is more “obvious” why that size is chosen, but less readable. :( I think we use only the size constant everywhere.

Copy link
Contributor Author

@mikebeaton mikebeaton Nov 26, 2024

Choose a reason for hiding this comment

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

Which one(s) are you saying I should keep, of the four currently in here? (Btw I added the one for 528 because the script uses 528 hard coded - expand down a bit from where I edited the script in the PR.)

Copy link
Member

Choose a reason for hiding this comment

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

528

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, yes, of the two for the size of that struct just 528.

But about the other two, for the containing struct? Maybe keep the +32 size one, since we removed packing on that struct as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sgtm.

Fix operation of `sign.command` when printable characters occur
immediately before `=BEGIN OC VAULT=`. `strings` finds the location of
the first printable character in such a sequence. `hexdump` automatically
works on 16 byte boundaries, so still finds the correct offset.

Use `BASE_ALIGNAS` to enforce the required alignment, which will not be
correct on all builds unless enforced (note alignment is required purely
for locating the structure correctly from external script as above, not
for reading in C).

Remove struct packing, since structs had better be naturally packed anyway
(if not, reading from them without arbitrary-alignment-safe code, as we
do, would be undefined behaviour). Add static asserts to confirm expected
size as required by `sign.command`.

Update the docs to refer to `sign.command` rather than to include the
signing commands explicitly - otherwise we have two places that need to
be kept in sync for signing commands, and note that the commands in the
two places were already out of sync.

Signed-off-by: Mike Beaton <[email protected]>
@mikebeaton mikebeaton merged commit 35bcb13 into master Nov 26, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Vault and signature is corrupted in OC 1.0.2
2 participants