Skip to content
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
9f71461
add remote attestation test inside UEFI app
Apr 11, 2022
fb4f905
Stub missing symbols
Apr 11, 2022
c4b2bea
improve comment spelling somewhat
Apr 11, 2022
63c22a9
Use ___chkstk_ms from go
Apr 11, 2022
8c49af0
simplify test to aid debugging: just test ring source of randomness
Apr 11, 2022
d73e848
log random data in test
Apr 12, 2022
d8111c8
patch rdrand availability check for debuggingnd enable KVM
Apr 13, 2022
b20fe50
Merge branch 'main' into uefi-test-remote-attestation
Apr 14, 2022
837660c
Add test to check that the byte array has in fact been filled
Apr 14, 2022
3227a63
add remote attestation handshake test
Apr 14, 2022
d2009a3
Update config to match #2725
Apr 14, 2022
d8de7d6
Remoe test of ring, as remote attestation is now tested
Apr 14, 2022
7a94c05
remove debugging logs from remote attestation test
Apr 14, 2022
2779e2a
wip
Apr 14, 2022
e69748c
Merge branch 'main' into uefi-test-remote-attestation
Apr 14, 2022
17960fd
Revert "wip"
Apr 14, 2022
b458198
Move the UEFI remote attestation test into it's own module
Apr 14, 2022
9d43182
remove now unneeded crate imports in the main UEFI app
Apr 14, 2022
2e0bc9a
finalize moving remote attestation test
Apr 14, 2022
7f0889b
redundant surplus qemu cpu flags
Apr 14, 2022
7bb2381
Add anyhow as dev dep for test
Apr 14, 2022
56e0e25
Add extra context to ring stubs
Apr 14, 2022
6f66c52
Only run remote attestation tests if the hosts supports kvm, and the …
Apr 14, 2022
ca2cea7
improve comment for clarity
Apr 14, 2022
0c424d5
improve comment copy
Apr 14, 2022
8f1e760
Merge branch 'main' into uefi-test-remote-attestation
Apr 14, 2022
8d258a5
disable ring default features to fix duplicate lang items
Apr 14, 2022
26082d0
bust CI cache
Apr 14, 2022
eebf47c
toggle tests that require kvm by disabling a default flag, as xtask e…
Apr 14, 2022
6c27bf0
Clarify comment
Apr 14, 2022
23a3508
Improve comments for accuracy and clarity
Apr 19, 2022
9e03123
Don't lint target subdirectories created by cargo fuzz
Apr 19, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion experimental/uefi/app/.cargo/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
target = "x86_64-unknown-uefi"

[target.x86_64-unknown-uefi]
runner = "qemu-system-x86_64 -nodefaults -nographic -bios /usr/share/OVMF/OVMF_CODE.fd -serial file:target/console.log -serial stdio -machine q35 -device isa-debug-exit,iobase=0xf4,iosize=0x04 -kernel"
runner = "./runner"

[unstable]
build-std = ["core", "alloc"]
Expand Down
17 changes: 9 additions & 8 deletions experimental/uefi/app/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions experimental/uefi/app/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@ authors = ["Andri Saar <[email protected]>"]
edition = "2021"
license = "Apache-2.0"

[features]
default = []
kvm = []

[dependencies]
uefi = { version = "*", features = ["exts"] }
uefi-services = "*"
log = { version = "*" }
oak_remote_attestation = { path = "../../../remote_attestation/rust" }

[dev-dependencies]
anyhow = { version = "*", default-features = false }
uefi-services = { version = "*", features = ["qemu"] }
36 changes: 36 additions & 0 deletions experimental/uefi/app/runner
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#!/usr/bin/env bash

# Thin shell script invoked as a cargo runner to run the compiled efi firmware
# in QEMU. Detects if kvm is supported, and sets qemu flags based on that.
# Instead of this single runner script it would be preferable to use a different
# runner based on whether the kvm feature is set. However, cargo does not
# currently allow this. Ref: https://github.com/rust-lang/cargo/issues/8170
Comment on lines +3 to +7
Copy link
Author

Choose a reason for hiding this comment

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

Not a fan of this solution, but there does not currently seem to be a better way to do this. As long as we want this crate to run in GitHub CI we'll need to support hosts that do not support kvm.

Perhaps once we add a CI runner that supports virtualization we can remove this, only leaving the kvm reliant runner.


readonly TARGET=$1

qemu_flags=(
'-nodefaults'
'-nographic'
'-bios' '/usr/share/OVMF/OVMF_CODE.fd'
'-serial' 'file:target/console.log'
'-serial' 'stdio'
'-machine' 'q35'
'-device' 'isa-debug-exit,iobase=0xf4,iosize=0x04'
)

# Use kvm if supported, as it is required for certain features (that are gated
# behind the cargo `kvm` feature flag. Note that systems that support kvm still
# need to enable the cargo `kvm` feature for this crate in order to include
# code that requires kvm in the compiled firmware. Ideally this check would
# check the cargo `kvm` flag itself. However, cargo does not expose this
# information to the runner.
# Ref: https://doc.rust-lang.org/cargo/reference/environment-variables.html
if [[ -e "/dev/kvm" ]]; then
qemu_flags+=(
'-enable-kvm'
'-cpu' 'Broadwell-IBRS'
)
fi

qemu-system-x86_64 "${qemu_flags[@]}" -kernel "${TARGET}"

Copy link
Author

Choose a reason for hiding this comment

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

Not a fan of this solution, but there does not currently seem to be a better way to do this. As long as we want this crate to run in GitHub CI we'll need to support hosts that do not support kvm.

Perhaps once we add a CI runner that supports virtualization we can remove this, only leaving the kvm reliant runner.

3 changes: 3 additions & 0 deletions experimental/uefi/app/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,6 @@ fn test_simple() {
let x = 1;
assert_eq!(x, 1);
}

#[cfg(test)]
mod tests;
20 changes: 20 additions & 0 deletions experimental/uefi/app/src/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//
// Copyright 2022 The Project Oak Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

// Requires advanced CPU features that are unavailable when running using
// emulation.
#[cfg(feature = "kvm")]
mod remote_attestation;
109 changes: 109 additions & 0 deletions experimental/uefi/app/src/tests/remote_attestation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
//
// Copyright 2022 The Project Oak Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

//! Integration Test of Remote Attestation in UEFI
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This line of the comment should end with a . and seeing that it looks like a heading it should probably have a blank line after it.

//! This tests that remote attestion works inside the UEFI app. While the test
//! code is identical to (a subset of) the tests in the remote attestation crate
//! they here utilize the qemu runner configured in the UEFI app. This means
//! that test code actually compiled to a UEFI target, which changes the
//! underlying implementation of the remote attestation crate.
//! TODO(#2654): It would be preferable to remove the test here, and instead
//! run the tests in the oak_remote_attestation crate itself for both standard
//! and UEFI targets. Due to concerns related to the workspace this is presently
//! not possible. Ref: https://github.com/project-oak/oak/issues/2654
Copy link
Author

@jul-sh jul-sh Apr 14, 2022

Choose a reason for hiding this comment

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

don't love repeating the test here, but imo the pragmatic choice for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that it is a good pragmatic choice for now. Once we have an end-to-end example with a connection from a client that exercises this functionality the test could be removed.


extern crate alloc;

use alloc::{boxed::Box, sync::Arc};
use oak_remote_attestation::handshaker::{AttestationBehavior, ClientHandshaker, ServerHandshaker};

const TEE_MEASUREMENT: &str = "Test TEE measurement";
const DATA: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];

fn create_handshakers() -> (ClientHandshaker, ServerHandshaker) {
let bidirectional_attestation =
AttestationBehavior::create_bidirectional_attestation(&[], TEE_MEASUREMENT.as_bytes())
.unwrap();
let client_handshaker = ClientHandshaker::new(
bidirectional_attestation,
Box::new(|server_identity| {
if !server_identity.additional_info.is_empty() {
Ok(())
} else {
anyhow::bail!("No additional info provided.")
}
}),
);

let bidirectional_attestation =
AttestationBehavior::create_bidirectional_attestation(&[], TEE_MEASUREMENT.as_bytes())
.unwrap();

let additional_info = br"Additional Info".to_vec();
let server_handshaker =
ServerHandshaker::new(bidirectional_attestation, Arc::new(additional_info));

(client_handshaker, server_handshaker)
}

#[test_case]
fn test_handshake() {
let (mut client_handshaker, mut server_handshaker) = create_handshakers();

let client_hello = client_handshaker
.create_client_hello()
.expect("Couldn't create client hello message");

let server_identity = server_handshaker
.next_step(&client_hello)
.expect("Couldn't process client hello message")
.expect("Empty server identity message");

let client_identity = client_handshaker
.next_step(&server_identity)
.expect("Couldn't process server identity message")
.expect("Empty client identity message");
assert!(client_handshaker.is_completed());

let result = server_handshaker
.next_step(&client_identity)
.expect("Couldn't process client identity message");
assert_eq!(result, None);
assert!(server_handshaker.is_completed());

let mut client_encryptor = client_handshaker
.get_encryptor()
.expect("Couldn't get client encryptor");
let mut server_encryptor = server_handshaker
.get_encryptor()
.expect("Couldn't get server encryptor");

let encrypted_client_data = client_encryptor
.encrypt(&DATA)
.expect("Couldn't encrypt client data");
let decrypted_client_data = server_encryptor
.decrypt(&encrypted_client_data)
.expect("Couldn't decrypt client data");
assert_eq!(decrypted_client_data, DATA);

let encrypted_server_data = server_encryptor
.encrypt(&DATA)
.expect("Couldn't encrypt server data");
let decrypted_server_data = client_encryptor
.decrypt(&encrypted_server_data)
.expect("Couldn't decrypt server data");
assert_eq!(decrypted_server_data, DATA);
}
2 changes: 2 additions & 0 deletions third_party/ring/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ include = [
"benches/*.rs",
"build.rs",

"stubs.c",

"crypto/chacha/asm/chacha-armv4.pl",
"crypto/chacha/asm/chacha-armv8.pl",
"crypto/chacha/asm/chacha-x86.pl",
Expand Down
1 change: 1 addition & 0 deletions third_party/ring/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ const RING_SRCS: &[(&[&str], &str)] = &[
(&[X86], "crypto/chacha/asm/chacha-x86.pl"),
(&[X86], "crypto/fipsmodule/modes/asm/ghash-x86.pl"),

(&[X86_64], "stubs.c"),
(&[X86_64], "crypto/chacha/asm/chacha-x86_64.pl"),
(&[X86_64], "crypto/fipsmodule/aes/asm/aesni-x86_64.pl"),
(&[X86_64], "crypto/fipsmodule/aes/asm/vpaes-x86_64.pl"),
Expand Down
30 changes: 30 additions & 0 deletions third_party/ring/stubs.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#include <stddef.h>

/**
*
Stub function for win64 error handler API call inserted by nasm.
Stubbed as it is unavailable in UEFI. It appears unlikely this call will
ever be invoked in deployment.
Ref: https://github.com/openssl/openssl/issues/12712.
Inspired by: https://github.com/tianocore/edk2/blob/7c0ad2c33810ead45b7919f8f8d0e282dae52e71/CryptoPkg/Library/OpensslLib/X64/ApiHooks.c
**/
void *
__imp_RtlVirtualUnwind (
void *Args
)
{
return NULL;
}

/**
Stub function for win64 routine used for exceedingly large variables.
Inserted Mby inGW, stubbed as it is unavailable in UEFI. Given that this
Copy link
Collaborator

Choose a reason for hiding this comment

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

super nit: I think this is inserted by nasm rather than MinGW.

routine is used for very large variable it appears unlikely to ever be
invoked in deployment.
Copy link
Author

Choose a reason for hiding this comment

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

Famous last words; would appreciate extra eyes on this in review. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I could see this is only used by the poly1305 code because it puts a lot of data on the stack. My understanding is that it is only inserted as a peformance optimisation on Windows, and should not break anything if a blank stub version is invoked. At worst it might cause slightly reduced performance, so I thinkg this is good for now.

Ref: https://github.com/golang/go/issues/6305
Inspired by: https://android.googlesource.com/platform/external/compiler-rt/+/ccaafe6%5E%21/#F1
**/
void ___chkstk_ms(void)
{
}