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

If tboot falls back to unmeasured boot, Anti Evil Maid appears to work but is insecure #2569

Closed
mattmccutchen opened this Issue Jan 12, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@mattmccutchen

Qubes OS version (e.g., R3.2): R3.2

Affected TemplateVMs (e.g., fedora-23, if applicable): N/A

If a user sets up Anti Evil Maid but tboot is unable to perform a measured boot for some reason (e.g., TXT is disabled or the wrong SINIT module is installed), tboot automatically falls back to an unmeasured boot and leaves PCR 17-19 set to all FF. Anti Evil Maid will happily seal the secret to these values and appear to be working, but the secret will be successfully unsealed regardless of changes to the boot components (as long as the condition that prevents a measured boot remains), so there is no security. This may give the user a false sense of security. In this situation, Anti Evil Maid should show a message that measured boot is not working and refuse to seal the secret.

Previous thread.

@rustybird

This comment has been minimized.

Show comment
Hide comment
@rustybird

rustybird Jan 12, 2017

We could make anti-evil-maid-seal abort if grep -E '^PCR-1[3789]:( (00|FF)){20}' on the pcrs file in /sys succeeds, i.e. if any of the standard registers are all zero/one bits. Now, where to find this file - does /sys/devices/pnp*/*/tpm/tpm*/pcrs work for everybody?

rustybird commented Jan 12, 2017

We could make anti-evil-maid-seal abort if grep -E '^PCR-1[3789]:( (00|FF)){20}' on the pcrs file in /sys succeeds, i.e. if any of the standard registers are all zero/one bits. Now, where to find this file - does /sys/devices/pnp*/*/tpm/tpm*/pcrs work for everybody?

@rustybird

This comment has been minimized.

Show comment
Hide comment
@rustybird

rustybird Jan 12, 2017

Actually, let's put that check (whatever it will be, exactly) into anti-evil-maid-lib and call it from both -seal and -unseal, so existing users can also see the warning if necessary.

Actually, let's put that check (whatever it will be, exactly) into anti-evil-maid-lib and call it from both -seal and -unseal, so existing users can also see the warning if necessary.

@mattmccutchen

This comment has been minimized.

Show comment
Hide comment
@mattmccutchen

mattmccutchen Jan 12, 2017

Now, where to find this file - does /sys/devices/pnp*/*/tpm/tpm*/pcrs work for everybody?

It doesn't work for me: I have /sys/devices/pnp0/00:06/pcrs, and /sys/devices/pnp0/00:06/tpm/tpm0 is a directory that does not contain pcrs but contains a symlink device -> ../../../00:06, so /sys/devices/pnp*/*/tpm/tpm*/device/pcrs would work for me. Other ideas: find /sys/devices -name pcrs like in the instructions, or add a tpm_pcr_read tool to the tpm-extra package that uses the Trousers API, which would be mostly a copy and paste of the existing tpm_pcr_extend tool.

Now, where to find this file - does /sys/devices/pnp*/*/tpm/tpm*/pcrs work for everybody?

It doesn't work for me: I have /sys/devices/pnp0/00:06/pcrs, and /sys/devices/pnp0/00:06/tpm/tpm0 is a directory that does not contain pcrs but contains a symlink device -> ../../../00:06, so /sys/devices/pnp*/*/tpm/tpm*/device/pcrs would work for me. Other ideas: find /sys/devices -name pcrs like in the instructions, or add a tpm_pcr_read tool to the tpm-extra package that uses the Trousers API, which would be mostly a copy and paste of the existing tpm_pcr_extend tool.

@rustybird

This comment has been minimized.

Show comment
Hide comment
@rustybird

rustybird Jan 12, 2017

@mattmccutchen: Do you also have the /sys/devices/pnp*/*/pcrs symlink? Sorry, I've reread your comment and see that you do.

rustybird commented Jan 12, 2017

@mattmccutchen: Do you also have the /sys/devices/pnp*/*/pcrs symlink? Sorry, I've reread your comment and see that you do.

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 12, 2017

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 12, 2017

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 12, 2017

@andrewdavidwong andrewdavidwong added this to the Release 3.2 updates milestone Jan 13, 2017

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 13, 2017

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 13, 2017

@rustybird

This comment has been minimized.

Show comment
Hide comment
@mattmccutchen

This comment has been minimized.

Show comment
Hide comment
@mattmccutchen

mattmccutchen Jan 13, 2017

I realized after I drafted the new error message that if the PCR check fails unexpectedly during unsealing, the user won't want to boot the system in order to view /usr/share/doc/anti-evil-maid/README. This looks silly, but do we care? The use case is that before going through the full procedure to recover from a potential compromise, the user might want to see if there was a bad configuration change that can just be reversed; if unsealing then succeeds, the user knows the reversal was successful as far as the AEM security model goes. Currently, the user would have to read the readme using another device. We could try to be more helpful by enhancing /etc/grub.d/19_linux_xen_tboot to generate another boot menu option with tboot vga logging enabled and have the error message direct the user to try that option. But I'm happy to proceed with the existing change to fix the security problem and leave additional help as a possible future enhancement.

I realized after I drafted the new error message that if the PCR check fails unexpectedly during unsealing, the user won't want to boot the system in order to view /usr/share/doc/anti-evil-maid/README. This looks silly, but do we care? The use case is that before going through the full procedure to recover from a potential compromise, the user might want to see if there was a bad configuration change that can just be reversed; if unsealing then succeeds, the user knows the reversal was successful as far as the AEM security model goes. Currently, the user would have to read the readme using another device. We could try to be more helpful by enhancing /etc/grub.d/19_linux_xen_tboot to generate another boot menu option with tboot vga logging enabled and have the error message direct the user to try that option. But I'm happy to proceed with the existing change to fix the security problem and leave additional help as a possible future enhancement.

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 13, 2017

@rustybird

This comment has been minimized.

Show comment
Hide comment
@rustybird

rustybird Jan 13, 2017

Good point. The user definitely shouldn't be encouraged (even subconsciously) to boot an unverified system. I've force pushed an updated commit series referring to the Anti Evil Maid README instead of /usr/share/doc/anti-evil-maid/README, because e.g. viewing it on GitHub with another device is also an option. But it's true, a recovery menu would be nice...

Good point. The user definitely shouldn't be encouraged (even subconsciously) to boot an unverified system. I've force pushed an updated commit series referring to the Anti Evil Maid README instead of /usr/share/doc/anti-evil-maid/README, because e.g. viewing it on GitHub with another device is also an option. But it's true, a recovery menu would be nice...

@rustybird rustybird referenced this issue in QubesOS/qubes-antievilmaid Jan 13, 2017

Merged

PCR sanity check #19

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jan 15, 2017

Instead of hardcoding it to PCRs 13,17,18,19 it would be better to check the PCRs that have been specified when sealing (i.e. /etc/anti-evil-maid.conf). The AEM README does give the impression the user can pick and choose them instead of using the defaults.

tasket commented Jan 15, 2017

Instead of hardcoding it to PCRs 13,17,18,19 it would be better to check the PCRs that have been specified when sealing (i.e. /etc/anti-evil-maid.conf). The AEM README does give the impression the user can pick and choose them instead of using the defaults.

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jan 15, 2017

To clarify, if a user has selected PCRs so as not to rely on the TXT dependant registers, then AEM should continue to work instead of failing... correct?

tasket commented Jan 15, 2017

To clarify, if a user has selected PCRs so as not to rely on the TXT dependant registers, then AEM should continue to work instead of failing... correct?

rustybird added a commit to rustybird/qubes-antievilmaid that referenced this issue Jan 15, 2017

Invalidate existing users' sealed secrets
Ensure that users already affected by QubesOS/qubes-issues#2569 have to
reseal their secret, so they can see the new error message.

(Of the existing users _not_ affected by the bug, almost everyone will
have to reseal anyway, because an anti-evil-maid package update causes
the initrd to be regenerated.)
@rustybird

This comment has been minimized.

Show comment
Hide comment
@rustybird

rustybird Jan 15, 2017

@tasket:

I suppose there is a (marginal) use case for not sealing to some of the default PCRs: If tboot is incompatible, the user might want to seal only to PCR 13, to at least verify the LUKS header(s). Unconditionally requiring nonempty PCRs 17-19 would indeed break this.

So for -seal, let's verify the intersection between standard and configured PCRs.

For -unseal, even if /etc/anti-evil-maid.conf were to be made available in the initrd, it might be out of date in case the user later changed the PCRs an reran -seal (manually or due to a Xen update). And tpm-tools doesn't seem to expose a way to find out which PCRs a blob was actually sealed to. But given that the -unseal check really only needs to be a one-time thing to warn existing users with broken AEM installations, there was an easier way:

I've updated the PR to change the sealed blob filename suffix from .sealed to .sealed2 to ensure that everyone's sealed secrets really are invalidated by the upgrade.

@tasket:

I suppose there is a (marginal) use case for not sealing to some of the default PCRs: If tboot is incompatible, the user might want to seal only to PCR 13, to at least verify the LUKS header(s). Unconditionally requiring nonempty PCRs 17-19 would indeed break this.

So for -seal, let's verify the intersection between standard and configured PCRs.

For -unseal, even if /etc/anti-evil-maid.conf were to be made available in the initrd, it might be out of date in case the user later changed the PCRs an reran -seal (manually or due to a Xen update). And tpm-tools doesn't seem to expose a way to find out which PCRs a blob was actually sealed to. But given that the -unseal check really only needs to be a one-time thing to warn existing users with broken AEM installations, there was an easier way:

I've updated the PR to change the sealed blob filename suffix from .sealed to .sealed2 to ensure that everyone's sealed secrets really are invalidated by the upgrade.

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jan 15, 2017

Sealing with PCRs 0-5 is possible. This includes the BIOS, option ROM, configuration, and MBR. Are you saying this does not help the security of the boot process?

With the above changes, someone sealing to those Non-TXT PCRs would have their boot proccess halted if TXT didn't work.

tasket commented Jan 15, 2017

Sealing with PCRs 0-5 is possible. This includes the BIOS, option ROM, configuration, and MBR. Are you saying this does not help the security of the boot process?

With the above changes, someone sealing to those Non-TXT PCRs would have their boot proccess halted if TXT didn't work.

@rustybird

This comment has been minimized.

Show comment
Hide comment
@rustybird

rustybird Jan 15, 2017

Sealing with PCRs 0-5 is possible. This includes the BIOS, option ROM, configuration, and MBR. Are you saying this does not help the security of the boot process?

Sounds good to me, at least with an external AEM device. Not sure why I didn't include the lower PCRs in the example use case, TBH I haven't really thought a lot about non-SINIT boot.

With the above changes, someone sealing to those Non-TXT PCRs would have their boot proccess halted if TXT didn't work.

Do you mean the previous version of the PR? The newer one will only verify the standard TXT/LUKS registers if they've also been configured in /etc/anti-evil-maid.conf. And it does not affect unsealing (only sealing), aside from the one time this update is applied.

Sealing with PCRs 0-5 is possible. This includes the BIOS, option ROM, configuration, and MBR. Are you saying this does not help the security of the boot process?

Sounds good to me, at least with an external AEM device. Not sure why I didn't include the lower PCRs in the example use case, TBH I haven't really thought a lot about non-SINIT boot.

With the above changes, someone sealing to those Non-TXT PCRs would have their boot proccess halted if TXT didn't work.

Do you mean the previous version of the PR? The newer one will only verify the standard TXT/LUKS registers if they've also been configured in /etc/anti-evil-maid.conf. And it does not affect unsealing (only sealing), aside from the one time this update is applied.

@mattmccutchen

This comment has been minimized.

Show comment
Hide comment
@mattmccutchen

mattmccutchen Jan 19, 2017

Now what about the existing HCL entries that say "AEM works"? Are we confident AEM actually worked and didn't just appear to work due to this bug?

Now what about the existing HCL entries that say "AEM works"? Are we confident AEM actually worked and didn't just appear to work due to this bug?

@qubesos-bot

This comment has been minimized.

Show comment
Hide comment
@qubesos-bot

qubesos-bot Jan 19, 2017

Automated announcement from builder-github

The package anti-evil-maid-3.0.5-1.fc23 has been pushed to the r3.2 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

Automated announcement from builder-github

The package anti-evil-maid-3.0.5-1.fc23 has been pushed to the r3.2 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

@qubesos-bot

This comment has been minimized.

Show comment
Hide comment
@qubesos-bot

qubesos-bot Jun 29, 2017

Automated announcement from builder-github

The package anti-evil-maid-3.0.5-1.fc23 has been pushed to the r3.2 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update

Automated announcement from builder-github

The package anti-evil-maid-3.0.5-1.fc23 has been pushed to the r3.2 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment