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

Confirm Credential removal with a touch #92

Closed
szszszsz opened this issue Jun 27, 2023 · 18 comments · Fixed by #93
Closed

Confirm Credential removal with a touch #92

szszszsz opened this issue Jun 27, 2023 · 18 comments · Fixed by #93
Labels
bug Something isn't working

Comments

@szszszsz
Copy link
Member

szszszsz commented Jun 27, 2023

By design all state changes (exception: HOTP counter) should be confirmed by touch. Currently removal of the credential does not have such. To add it.

Connected: Nitrokey/nitrokey-3-firmware#303

image

fn delete(&mut self, delete: command::Delete<'_>) -> Result {
debug_now!("{:?}", delete);
// It seems tooling first lists all credentials, so the case of
// delete being called on a non-existing label hardly occurs.
// APDU: 00 A4 04 00 07 A0 00 00 05 27 21 01
// SW: 79 03 01 00 00 71 08 26 9F 14 54 3A 0E C7 AC 90 00
// APDU: 00 A1 00 00 00
// SW: 72 13 21 74 6F 74 70 2E 64 61 6E 68 65 72 73 61 6D 2E 63 6F 6D 72 07 21 79 75 62 69 63 6F 90 00
// APDU: 00 02 00 00 08 71 06 79 75 62 69 63 6F
// SW: 90 00
let label = &delete.label;
if let Some(_credential) = self.load_credential(label) {
let _filename = self.filename_for_label(label);
let _deletion_result =
try_syscall!(self.trussed.remove_file(self.options.location, _filename));
debug_now!(
"Delete credential with filename {}, result: {:?}",
&self.filename_for_label(label),
_deletion_result
);
} else {
return Err(Status::NotFound);
}
Ok(())
}

@szszszsz szszszsz added the bug Something isn't working label Jun 27, 2023
@peterwilli
Copy link
Contributor

Wow, I love that diagram. Is it something I can try and fix? I want to contribute to the firmware and this seems to be a good start! I have done embedded security hardware before (Ledger, in C) but am working fulltime in Rust now.

@szszszsz
Copy link
Member Author

szszszsz commented Jun 27, 2023

Sure! This is a one-line change, but it needs to be tested / checked for potential implications. Feel free to take it :-)
Let me know if you need any directions. Quick crash course - you can test all changes live using USB/IP simulation:

$ env RUST_LOG=debug cargo run --example usbip --features "ctaphid devel"
$ make -f examples/Makefile attach
# run tests
$ cd ../pynitrokey && make secrets-test
# or with pynitrokey as usual
$ pynitrokey nk3 secrets list
# disconnect from the USB server once done
$ make -f examples/Makefile stop
# kill the usb/ip client run by the cargo
# <ctrl+c> or killall

Edit: added RUST_LOG=debug

@peterwilli
Copy link
Contributor

I'll get on it right away! So this means you don't need to flash your NitroKey with the firmware to test it out?

@szszszsz
Copy link
Member Author

szszszsz commented Jun 27, 2023

Great!
Yes, you can run it locally, and it could behave like a real connected device. For the real hardware test you would need a "Hacker" variant to flash your build, since only signed firmware is accepted in the sold Nitrokey 3 devices.
The USB/IP Simulation has one disadvantage currently however - it auto-accepts all touch requests, hence it does not allow to test touch-based test cases. It's logged though, so you can see whether it shows up or not.
Former is not a technical limitation though - this should be relatively simple to implement, e.g. using a TCP/IP supplementary app for that.

Edit: alternatively maybe the DevKits would do, but this target was removed recently

@szszszsz
Copy link
Member Author

I forgot to mention, that you need USB/IP kernel modules and tools available in your Linux system. I have not tested that with Windows/macOS, but perhaps it would work there too.

@robin-nitrokey
Copy link
Member

If you want to develop with real hardware, this would be the starting point: https://github.com/Nitrokey/nitrokey-3-firmware/blob/main/docs/lpc55-quickstart.md

@peterwilli
Copy link
Contributor

I forgot to mention, that you need USB/IP kernel modules and tools available in your Linux system. I have not tested that with Windows/macOS, but perhaps it would work there too.

I run Linux so it should be fine. If not, I'll be able to install them I'm sure!

@peterwilli
Copy link
Contributor

Yes, you can run it locally, and it could behave like a real connected device. For the real hardware test you would need a "Hacker" variant to flash your build, since only signed firmware is accepted in the sold Nitrokey 3 devices.

Ah that's kinda sad, I was hoping to add more features to the firmware when I would get more experienced, I assumed that since the firmware was open source it would be possible to flash a custom build in some way or another

@robin-nitrokey
Copy link
Member

Ah that's kinda sad, I was hoping to add more features to the firmware when I would get more experienced, I assumed that since the firmware was open source it would be possible to flash a custom build in some way or another

This is not possible for normal Nitrokey 3 devices because they are shipped with secure boot. But if you order a Nitrokey 3 Hacker device from [email protected], you will get the same hardware without secure boot and can run any firmware you want on it.

@peterwilli
Copy link
Contributor

Ah that's kinda sad, I was hoping to add more features to the firmware when I would get more experienced, I assumed that since the firmware was open source it would be possible to flash a custom build in some way or another

This is not possible for normal Nitrokey 3 devices because they are shipped with secure boot. But if you order a Nitrokey 3 Hacker device from [email protected], you will get the same hardware without secure boot and can run any firmware you want on it.

I might get back about that later then 👍🏼

robin-nitrokey added a commit to Nitrokey/nitrokey-app2 that referenced this issue Jun 27, 2023
This patch adds a touch prompt when deleting a credential.  While a
touch confirmation is not required at the moment, this is unintentional
and will be changed in an upcoming firmware release.

See: Nitrokey/trussed-secrets-app#92
Fixes: #110
@peterwilli
Copy link
Contributor

I did get the error about USB/IP. I managed to install the driver and now tests seem to run well, so I can begin make the change

@peterwilli
Copy link
Contributor

peterwilli commented Jun 27, 2023

I seem to have done it! It was really easy in hindsight, I will make a PR now and we'll see from there. The logs were very clear on where a button was supposed to be pressed.

Also, this USB/IP thing is very new to me but very pleasant to work like this, it this possible on any Rust based embedded hardware/software project? Like using it on a firmware written in Rust for STM32 or something.

@peterwilli
Copy link
Contributor

peterwilli commented Jun 27, 2023

Here we go: #93

Ironically, this is my first commit signed with a Nitrokey

@robin-nitrokey
Copy link
Member

Wow, that was quick! :)

Also, this USB/IP thing is very new to me but very pleasant to work like this, it this possible on any Rust based embedded hardware/software project? Like using it on a firmware written in Rust for STM32 or something.

This is not related to Rust at all. You just need a driver that wraps the USB packets in IP packets and sends them to the correct socket. Rust’s abstractions make it easy to switch out the real USB driver with the usbip driver, but the same principle works for all languages.

@szszszsz
Copy link
Member Author

@peterwilli
I am glad you like it! We are using this one: https://github.com/sawchord/usbip-device
You may find the examples there interesting. As @robin-nitrokey said you need a proper abstraction, but once this is set up its a breeze.

@peterwilli
Copy link
Contributor

Wow, that was quick! :)

Also, this USB/IP thing is very new to me but very pleasant to work like this, it this possible on any Rust based embedded hardware/software project? Like using it on a firmware written in Rust for STM32 or something.

This is not related to Rust at all. You just need a driver that wraps the USB packets in IP packets and sends them to the correct socket. Rust’s abstractions make it easy to switch out the real USB driver with the usbip driver, but the same principle works for all languages.

Oh, so you use an abstraction layer in Rust to switch to USB/IP. From what I understand, there's no "emulator" in place that ran the firmware I just tested?

@robin-nitrokey
Copy link
Member

From what I understand, there's no "emulator" in place that ran the firmware I just tested?

Exactly. The bigger picture is: We use the Trussed framework for our firmware. This framework provides abstractions that can be used by applications like the secrets app. Then there is a runner that provides implementations for these abstractions and defines the available applications.

To run the firmware on hardware, you would use a runner that uses hardware-specific code to implement these abstractions, e. g. accessing the flash chip or the touch button. But for the usbip simulation, we just use the standard library and the file system to implement them and compile to a native binary. The advantage is that it is very easy to test, the downside is that we don’t test the low-level components of the firmware. But for application development, this is typically sufficient.

@peterwilli
Copy link
Contributor

Oh, that makes sense, so it's a good Swiss Army knife for development, but for testing out actual hardware elements you'd still need a different way to test on an actual device.

robin-nitrokey added a commit to Nitrokey/nitrokey-app2 that referenced this issue Jun 28, 2023
This patch adds a touch prompt when deleting a credential.  While a
touch confirmation is not required at the moment, this is unintentional
and will be changed in an upcoming firmware release.

See: Nitrokey/trussed-secrets-app#92
Fixes: #110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants