Thiago Jung Bauermann [off-list ref] writes:
Am Mittwoch, 07 September 2016, 09:19:40 schrieb Eric W. Biederman:
quoted
ebiederm@xmission.com (Eric W. Biederman) writes:
quoted
Thiago Jung Bauermann [off-list ref] writes:
quoted
Hello,
The purpose of this new version of the series is to fix a small issue
that I found, which is that the kernel doesn't remove the memory
reservation for the hand-over buffer it received from the previous
kernel in the device tree it sets up for the next kernel. The result
is that for each successive kexec, a stale hand-over buffer is left
behind, wasting memory.
This is fixed by changes to kexec_free_handover_buffer and
setup_handover_buffer in patch 2. The other change is to fix checkpatch
warnings in the last patch.
This is fundamentally broken. You do not increase the integrity of a
system by dropping integrity checks.
No. No. No. No.
Nacked-by: "Eric W. Biederman" [off-list ref]
The IMA measurement list can be verified without the need of a checksum over
its contents by replaying the PCR extend operations and checking that the
result matches the registers in the TPM device. So the fact that it is not
part of the kexec segments checksum verification doesn't actually reduce the
integrity of the system.
Bit flips and errant DMA transfers are the concern here. That happens
routinely and can easily result in a corrupt data structure which may be
non-trivial to verify.
Currently, IMA doesn't perform that verification when it restores the
measurement list from the kexec handover buffer, so if you believe it's
necessary to do that check at boot time, we could do one of the following:
1. Have IMA replay the PCR extend operations when it restores the
measurement list from the handover buffer and validate it against the TPM
PCRs, or
2. Have a buffer hash in the ima_kexec_hdr that IMA includes in the handover
buffer, and verify the buffer checksum before restoring the measurement
list.
What do you think?
I think you are playing very much with fire and I am extremely
uncomfortable with the entire concept. I think you are making things
more complicated in a way that will allow system to try and start
booting when their memory is correct. Which may wind up corrupting
someones non-volatile storage.
It makes me doubly nervous that this adds a general purpose facility
that is generally not at all reusable.
I have seen malicious actors cause entirely too much damage to be at all
comfortable using a data structure before we validate that it was valid
before we started booting. This isn't the same case but it is close
enough I don't trust someone just splatting data structures.
We can't guarantee integrity but we should not bypass best practices
either.
quoted
To be constructive the way we have handled similiar situations in the
past (hotplu memory) is to call kexec_load again.
Thanks for your suggestion. Unfortunately it's always possible for new
measurements to be added to the measurement list between the kexec_file_load
and the reboot. We see that happen in practice with system scripts and
configuration files that are only read or executed during the reboot
process. They are only measured by IMA as a result of the kexec execute.
If I understand what you are saying correctly I expect things could be
setup so that those measurements are forced to happen before kexec load.
Especially in a boot loader context which you described earlier I
believe you should have quite a lot of control of the system. Having a
facility that fundamentally undermines the design of kexec for a case
where someone might do something you have not predicted does not make me
comfortable.
Which is to say I don't see why you can't measure things before the
kexec_load system call in a tightly controlled setup like a boot loader.
Which should make it that in practice no measurements change. I believe
that should increase the reliability of the system overall.
Having code in the kernel that updates a buffer that kexec will use
after that buffer is loaded in memory honestely gives me the heebie
jeebies.
Eric
On Thu, 2016-09-08 at 23:07 -0500, Eric W. Biederman wrote:
Thiago Jung Bauermann [off-list ref] writes:
quoted
Am Mittwoch, 07 September 2016, 09:19:40 schrieb Eric W. Biederman:
quoted
ebiederm@xmission.com (Eric W. Biederman) writes:
quoted
Thiago Jung Bauermann [off-list ref] writes:
quoted
Hello,
The purpose of this new version of the series is to fix a small issue
that I found, which is that the kernel doesn't remove the memory
reservation for the hand-over buffer it received from the previous
kernel in the device tree it sets up for the next kernel. The result
is that for each successive kexec, a stale hand-over buffer is left
behind, wasting memory.
This is fixed by changes to kexec_free_handover_buffer and
setup_handover_buffer in patch 2. The other change is to fix checkpatch
warnings in the last patch.
This is fundamentally broken. You do not increase the integrity of a
system by dropping integrity checks.
No. No. No. No.
Nacked-by: "Eric W. Biederman" [off-list ref]
The IMA measurement list can be verified without the need of a checksum over
its contents by replaying the PCR extend operations and checking that the
result matches the registers in the TPM device. So the fact that it is not
part of the kexec segments checksum verification doesn't actually reduce the
integrity of the system.
Bit flips and errant DMA transfers are the concern here. That happens
routinely and can easily result in a corrupt data structure which may be
non-trivial to verify.
Point taken. Before using the measurement list, the hash needs to be
verified. So then the question is when and where that hash should be
taken and included.
New measurements can take place between the kexec load and execute.
Those measurements would be included in an additional execute of kexec
load, as you suggested. There are additional new measurements that are
the result of the kexec execute itself, as Thiago pointed out. Having
to know a priori what those measurements are, or should be, makes
carrying the measurement list across kexec brittle.
One solution, as suggested by Thiago, would be to calculate the
measurement list hash, include the hash in the ima_kexec_hdr and verify
the hash, before restoring the measurement list. Another option would
be to calculate the image hash on kexec load either with a zero filled
measurement list segment or without the segment, verify the image hash
on kexec execute, before re-calculating the image hash.
quoted
Currently, IMA doesn't perform that verification when it restores the
measurement list from the kexec handover buffer, so if you believe it's
necessary to do that check at boot time, we could do one of the following:
1. Have IMA replay the PCR extend operations when it restores the
measurement list from the handover buffer and validate it against the TPM
PCRs, or
2. Have a buffer hash in the ima_kexec_hdr that IMA includes in the handover
buffer, and verify the buffer checksum before restoring the measurement
list.
What do you think?
I think you are playing very much with fire and I am extremely
uncomfortable with the entire concept. I think you are making things
more complicated in a way that will allow system to try and start
booting when their memory is correct. Which may wind up corrupting
someones non-volatile storage.
Everything that was previously included in the image hash, before this
proposed patch set, is still included in the image hash.
Lets assume for a moment that the measurement list hash is stored in
the ima_kexec_hdr. In addition to preventing the measurement list from
being restored, we could either allow the reboot without the measurement
list to occur or halt the system.
Rebooting without the measurement list would result in the attestation
of the system failing.
It makes me doubly nervous that this adds a general purpose facility
that is generally not at all reusable.
Thiago has already agreed to reverting the skip hash bit.
I have seen malicious actors cause entirely too much damage to be at all
comfortable using a data structure before we validate that it was valid
before we started booting. This isn't the same case but it is close
enough I don't trust someone just splatting data structures.
We can't guarantee integrity but we should not bypass best practices
either.
quoted
quoted
To be constructive the way we have handled similiar situations in the
past (hotplu memory) is to call kexec_load again.
Thanks for your suggestion. Unfortunately it's always possible for new
measurements to be added to the measurement list between the kexec_file_load
and the reboot. We see that happen in practice with system scripts and
configuration files that are only read or executed during the reboot
process. They are only measured by IMA as a result of the kexec execute.
If I understand what you are saying correctly I expect things could be
setup so that those measurements are forced to happen before kexec load.
Especially in a boot loader context which you described earlier I
believe you should have quite a lot of control of the system. Having a
facility that fundamentally undermines the design of kexec for a case
where someone might do something you have not predicted does not make me
comfortable.
Which is to say I don't see why you can't measure things before the
kexec_load system call in a tightly controlled setup like a boot loader.
Which should make it that in practice no measurements change. I believe
that should increase the reliability of the system overall.
Either we measure everything possible, making the measurement list
unnecessarily large, or have a brittle system that could break the
attestation of the target system, if any packages add new files/scripts
that are accessed on a soft reboot.
Having code in the kernel that updates a buffer that kexec will use
after that buffer is loaded in memory honestely gives me the heebie
jeebies.
To address your understandable concerns, we suggested two possible
solutions for including and verifying the measurement list hash.
- calculate the image hash on load, verify it on kexec reload, before
recalculating the image hash with the measurement list segment.
- defer hashing and verifying the measurement list hash to IMA.
Mimi