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

Drop checksum field from signed files db #157

Closed

Conversation

WhyNotHugo
Copy link
Contributor

This is required before moving files from /usr into their proper
locations.

See: #57

This is required before moving files from `/usr` into their proper
locations.

See: #57
@WhyNotHugo
Copy link
Contributor Author

Still in draft due to:

  • One big comment in the middle regarding which feedback is welcome.
  • One test is still failing.
  • Tests need a locally EFI binary as input. I'm using a system one, but then tests will fail in other setups.

@WhyNotHugo
Copy link
Contributor Author

Regarding this bit

sbctl/keys.go

Lines 150 to 188 in e742a7f

func SignFile(key, cert, file, output string) error {
// Check file exists before we do anything
if _, err := os.Stat(file); errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("%s does not exist", file)
}
// Let's check if we have signed it already AND the original file hasn't changed
ok, err := VerifyFile(cert, output)
if errors.Is(err, os.ErrNotExist) && (file != output) {
// if the file does not exist and file is not the same as output
// then we just catch the error and continue. This is expected
// behaviour
} else if err != nil {
return err
}
peExisting, err := os.ReadFile(file)
if err != nil {
return err
}
ctxExisting := pecoff.PECOFFChecksum(peExisting)
peFile, err := os.ReadFile(file)
if err != nil {
return err
}
ctx := pecoff.PECOFFChecksum(peFile)
// XXX: I'm not 100% sure this is the best approach. A file corrupt in
// the right places might not be regenerated. Maybe it's best to strip
// the signature part of the destination and compare the non-signature
// part of the target file with the current one?
// TBH, this comparison is expensive enough that it might just be worth
// re-generating the target file even if it's up to date anyway.
if ok && bytes.Equal(ctxExisting.SigData.Bytes(), ctx.SigData.Bytes()) {
return ErrAlreadySigned
}
, I've doubts.

Is stripping the signature and re-signing a valid way to determine "it's already signed"? If the signature mismatches, we can either bail or overwrite. I think exiting non-zero is best?

@Foxboron
Copy link
Owner

Foxboron commented Nov 1, 2022

Is stripping the signature and re-signing a valid way to determine "it's already signed"? If the signature mismatches, we can either bail or overwrite. I think exiting non-zero is best?

Can't we use pkcs7.VerifySignature? If we can't verify the file then we probably havent signed it. It seems simpler than messing with the implementation details of the weird context struct :)

@Foxboron
Copy link
Owner

Foxboron commented Nov 1, 2022

I realized I might not even correctly remember the contraints we are working with :/ I really need to get back into hacking on all of this.

@WhyNotHugo
Copy link
Contributor Author

We also need to verify whether the file has changed or not. The file might have a valid signature, but be a different file (e.g.: the previous version of the bootloader).

freegnu and others added 10 commits November 27, 2022 13:06
Corrected typo "towards towards" to "towards"
The previous rule setup SHOULD work, but I found several tools messed up
and were ignoring `cmd/sbctl`. E.g.: `ag` did not recurse into it when
searching.
Generating a new bundle with an unmounted ESP would crash trying to find
one, even if one was explicitly specified.

This was due to Bundle instances always being created with a discovered
ESP path. However, this path is always overwritten based on the value
provided by the user, meaning that this default is unused and always
overwritten.

This changes NewBundle to simply use an empty ESP if finding one fails.
This has no impact on `sbctl` itself. This change COULD impact
applications using this codes as a library, but this only affects code
that previously panicked.

Fixes: #132
I asked on #voidlinux, the repo URLs might change (which is causing
failures right now), so need to `-S` again after updating xbps-install.
Check a few errors that were not being handled properly.

Co-authored-by: Morten Linderud <morten@linderud.pw>
If we have a newly created and empty file then the json parsing fails.
We should allow this and return an empty map/slice instead.

Signed-off-by: Morten Linderud <morten@linderud.pw>
@WhyNotHugo WhyNotHugo closed this by deleting the head repository Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants