Securelaunch verify policy #2850
Conversation
Add a function to read a file (mounting the containing device if necessary). While we're at it, clean up the code for writing a file as well. Currently, WriteToFile() takes a default file name and expects the target location to be mounted. Instead, create WriteFile() that will mount the device if needed and write the specified file. Modify persist() (currently the only user of WriteToFile()) to handle the case where we might need a default file name (as that's the only one that cares about that). Signed-off-by: Patrick Colp <[email protected]>
…licy' flag Currently, if the user doesn't specify the 'sl_policy' kernel command line argument or if it can't load what the user specified, u-root scans all the block devices looking in '/', '/efi', and '/boot' for a securelaunch.policy file. This makes it too easy to load the wrong policy file. Instead, just use whatever was supplied on the kernel command line. If the flag isn't set or the policy file cannot be located or read, return an error. This is also the first step in preparing for verification of the policy file by using a using a public-private key pair to sign the policy file. Signed-off-by: Patrick Colp <[email protected]>
The policy code used to locate, read, measure, and parse the policy file all in one function (named Get()). However, as we separate out code (e.g., measuring the policy file) and only "locate" it from a kernel command-line argument, it makes less sense to keep the rest of the functionality bundled. This will be even more true once verifying the policy file is also added. Split the functionality of the Get() function into Load(), Parse(), and Measure() functions. Signed-off-by: Patrick Colp <[email protected]>
…specified If a section doesn't exist in the policy file and there's a config option to disable it, then do so. That is, we treat missing sections in the policy file as if a "missing_option = false" config option was set. Signed-off-by: Patrick Colp <[email protected]>
… be done If certain steps were explicitly disabled in the config section of the policy file or implicitly due to missing sections in the policy file then skip those steps. Signed-off-by: Patrick Colp <[email protected]>
In order to seal secrets in the TPM that only the securelaunch kernel can unseal, we need to know the values of the PCRs while in the securelaunch kernel. Dump out all the PCR values when starting u-root so they can be used later to provision the system and seal secrets in the TPM. This will be useful for future functionality. For example, storing the hash of a public key file used to verify the securelaunch policy file or storing a LUKS key that can be injected into the target initrd to decrypt disks automatically without leaking the key (or even needing to know what the key is). Signed-off-by: Patrick Colp <[email protected]>
Currently, the policy file provided is trusted implicitly. However, an attacker could simply swap the policy file out with another and it will be processed. The first step in addressing this issue is to verify the policy file using a public key and signature. Currently, the key and signature are both trusted and the attacker could simply switch out all the files. A future commit will use a hash of the public key file, stored in the TPM, to verify that the public key provided is the expected one. Signed-off-by: Patrick Colp <[email protected]>
We verify the policy file by using a public key and signature, however we currently do not verify the public key file in any way. An attacker could cause an unwanted policy file to be used by also replacing the public key and signature files. Instead, read a stored copy of the hash of the public key from the TPM and use that to verify that the public key file is the expected file. This lets us trust it and thus trust that the policy file hasn't been tampered with (assuming it verifies OK). Signed-off-by: Patrick Colp <[email protected]>
Currently, the policy file has a "params" map under the "launcher"
settings that is used to specify the target kernel, initrd, and
command-line to use. However, this only allows for specifying one
target kernel/initrd/cmdline per policy file.
Switch the policy file to use a new "boot entries" entry in the
"launcher" section. The "boot entries" section is a map of boot entry
name to boot entry, where each boot entry not only specifies the kernel,
initrd, and cmdline, but also the expected SHA-256 hashes of the kernel
and initrd. These hashes can be used to verify that the kernel and
initrd files loaded into memory are the correct (unmodified) ones.
An example of the new "boot entries" section in the policy file:
```
"boot entries": {
"0": {
"kernel name":"vmlinuz-5.4.17-2036.103.3.el7uek.x86_64",
"kernel hash":"59c762615cdb09558bcd80d3025d023b436386fd9ab6e09d709418fbbce7680c",
"initrd name":"initramfs-5.4.17-2036.103.3.el7uek.x86_64.img",
"initrd hash":"a39a6ba3e35dffd0b91ca0f0dee2a7bfb16a447353746cf83d6dc7139dc99c4a",
"cmdline":"BOOT_IMAGE=/vmlinuz-5.4.17-2036.103.3.el7uek.x86_64 root=/dev/mapper/ol_ol7--sl-root ro crashkernel=auto rd.luks.uuid=luks-06f28824-6b55-4219-b1a4-69a466af670b rd.lvm.lv=ol_ol7-sl/root rd.lvm.lv=ol_ol7-sl/swap console=ttyS0,115200"
},
"1": {
"kernel name":"vmlinuz-4.14.35-1902.303.5.3.el7uek.x86_64",
"kernel hash":"fa17071a44c0c185de9f879cddf6823f4d64a0c26604657655dad7c1d2fae39c",
"initrd name":"initramfs-4.14.35-1902.303.5.3.el7uek.x86_64.img",
"initrd hash":"c409a5118dacb1c2c71b9dab078ff670f15cae5219475fba902b508dea616187",
"cmdline":"console=ttyS0,115200n8 earlyprintk=serial,ttyS0,115200n8,keep printk.time=y"
}
}
```
The entry to boot is passed in via the `securelaunch_entry` kernel
parameter and is the name of the entry to boot (e.g., "0", "1",
"kernel-5.18.13"). The names can contain any alphanumeric character as
well as dot, dash, and underscore (i.e., a-z A-Z 0-9 . - _).
Signed-off-by: Patrick Colp <[email protected]>
pjcolp
force-pushed
the
securelaunch-verify_policy
branch
from
cfa225a
to
268a3d7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2850 +/- ##
==========================================
- Coverage 77.36% 71.38% -5.98%
==========================================
Files 430 129 -301
Lines 43122 7318 -35804
==========================================
- Hits 33360 5224 -28136
+ Misses 9762 2094 -7668
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| func checkPolicyLocationFlag() (string, error) { | ||
| location, present := cmdline.Flag(policyLocationFlag) | ||
| if !present { | ||
| return "", fmt.Errorf("'%s' command line flag is not set", policyLocationFlag) |
There was a problem hiding this comment.
For writing tests and such, it's useful to use errors that can be wrapped.
You can make your own new error messages:
var ErrFlagNotSet = errors.New("command line flag is not set")
and then use it
return "", fmt.Errorf("%q:%w", policyLocationFlag, ErrFlagNotSet)
Also, if you are doing this
'%s'
you can use %q instead.
Finally, option 2 is something like this:
return "", fmt.Errorf("%q:%v", policyLocationFlag, os.ErrNotExist)
but then that may not give you a message you like.
| func parseBootEntryFlag() (string, error) { | ||
| bootEntry, present := cmdline.Flag(bootEntryFlag) | ||
| if !present { | ||
| return "", fmt.Errorf("no boot entry found") |
There was a problem hiding this comment.
same thing here. You might want to make a variable
var ErrNoBootEntry = fmt.Errorf("no boot entry found")
Also, on errors, I prefer more info, so
return "", fmt.Errorf("%q:%w", bootEntryFlag, ErrNoBootEntry)
it just make it easier to figure out what's going wrongt.
| } | ||
|
|
||
| if !launcher.IsValidBootEntry(bootEntry) { | ||
| return "", fmt.Errorf("boot entry contains bad characters: '%s'", bootEntry) |
There was a problem hiding this comment.
here's where instead of '%s' you really want %q
s := "\x03\x45ABD"
fmt.Printf("'%s' %q\n", s, s)
'�EABD' "\x03EABD"
is way more useful
| @@ -114,92 +176,146 @@ func initialize() error { | |||
| return fmt.Errorf("failed to get TPM device: %w", err) | |||
| } | |||
|
|
|||
| // Write out all the PCRs values. | |||
| pcrSelection := []uint32{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23} | |||
| pcrLogLocation := policyLocation + "/" + pcrLogFilename | |||
There was a problem hiding this comment.
here you want to use filepath.Join
| func verifyAndParsePolicy(policyLocation string) (*policy.Policy, error) { | ||
| printStep("Verify and parse SL policy") | ||
|
|
||
| policyFileLocation := policyLocation + "/" + policyFilename |
There was a problem hiding this comment.
All these need to be filepath.Join
| target = filepath.Join(dst, defFileName) | ||
| Debug("New target=%s", target) | ||
| } | ||
| Debug("ReadFile: reading file '%s' (mounted at '%s')", fileLocation, mountedFilePath) |
There was a problem hiding this comment.
could use %q here too I bet, instead of '%s'?
| if err != nil { | ||
| return "", fmt.Errorf("failed to write date to file =%s, err=%v", target, err) | ||
| return nil, fmt.Errorf("could not read file '%s' (mounted at '%s')", fileLocation, mountedFilePath) |
There was a problem hiding this comment.
Why not return the wrapper error? You were using err before?
| // MeasureKernel hashes the kernel and extends the measurement into a TPM PCR. | ||
| func MeasureKernel() error { | ||
| if bootEntry == nil { | ||
| return fmt.Errorf("boot entry not yet selected") |
There was a problem hiding this comment.
Not sure if you want an error variable here but it's a possibility
| initrd := l.Params["initrd"] | ||
| func MeasureInitrd() error { | ||
| if bootEntry == nil { | ||
| return fmt.Errorf("boot entry not yet selected") |
There was a problem hiding this comment.
yeah you def want an error variable, this string appears twice.
var ErrBootNotSelected=errors.New("boot entry not yet selected")
it's a pain at first but once you start writing tests you will be happy.
rminnich
added
the
Awaiting author
|
Thanks for the review! I keep forgetting about %q, I'll go through and change to using that. In a previous version I introduced error variables, but wasn't sure if that was the right thing to do. I guess it was, so I'll work that patch back into the set. I didn't realize about the coverage drop until I did the PR. I thought I had covered most of what I could, but I can see from the report some extra tests I can write. Is there a way for me to check the coverage locally? I also see that tests are faililng. It seems what's happening is that its running tests in a generic VM, but they need to be run with specific disks attached. I based it off what the tests in |
|
I'm happy to do a talk at OSFC. Is it possible to do a pre-recording? I'm planning to be in Europe/UK end of September/start of October, but it depends when and where it actually happens this year as to whether I'd be able to attend in person or not. |
|
I'm going to update the branch, the coverage testing is better now. |
|
there's a fair bit of cleanup to be done here, but it will still be good to get this in, if you are up for it. |
|
Yes, I'd like to get this in. There's even more stuff after this too :-) I've been adding some more tests, not sure I'm doing it 100% the best way, but that can be a conversation later. I've been pulled in a different direction recently, but I should be able to make more progress on it this week. |
Add the ability for the Secure Launch u-root to verify the policy file. This is accomplished by signing the policy file and using the public key to verify the signature. The public key is verified by comparing its hash to the hash stored in the TPM.
Add a "config" section to the policy file to control which steps to execute.
Add a "boot entires" section to the policy file so that multiple possible target kernels can be specified. Include their hashes so the files loaded off disk can be verified.
Integration tests were added/updated where appropriate.