mozilla / authenticator-rs Goto Github PK

View Code? Open in Web Editor NEW

270.0 14.0 67.0 1.16 MB

Rust library to interact with Security Keys, used by Firefox

Home Page: https://crates.io/crates/authenticator

License: Mozilla Public License 2.0

Rust 99.85% Dockerfile 0.06% Shell 0.05% C 0.04%

u2f hid rust

authenticator-rs's People

Contributors

Stargazers

Watchers

authenticator-rs's Issues

Fix Windows platform manager

The recent API changes broke Windows. Needs to be fixed.

Final buffers have an extra 0x00 appended

The u2f_register and u2f_sign operations' buffers have a 0x00 value appended to them. This is visible in the "main" example as AA values on the end of each string:

Asking a security key to register now...
Register result: BQS8jDTzbDfefi9WJ8Gc73K1jtA9gFYKb/qAdjh+FEnE85/aLrYFm4v9uQhHet8CafAxTm03aGXyVbmKUTU8iQMcQG3Tru3jaByKCrbUd2+VHAU7AYr4exe+Ju8tP5nJABAv9licn9xj01KrbPJ3kcAShXWWuTOVZEyvnE78qONzpVowggGqMIIBUaADAgECAgkA/lb+euH/GA4wCgYIKoZIzj0EAwIwMjEwMC4GA1UEAwwnUGx1Zy11cCBGSURPIEludGVybmFsIEF0dGVzdGF0aW9uIENBICMxMB4XDTE0MTAwMzA4MDY0OFoXDTM0MTAwMzA4MDY0OFowQDE+MDwGA1UEAww1UGx1Zy11cCBGSURPIFByb2R1Y3Rpb24gQXR0ZXN0YXRpb24gI2ZlNTZmZTdhZTFmZjE4MGUwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAR16gZg4pBjdIQ3AACvqjIlPoJ72Eh0k6aGpWhMZcrOCYvov0uHJT3vlrlAIwEG/EYGH31lRsFvFLJavzAZ2PQno0IwQDAdBgNVHQ4EFgQUditEb/KU7TIq5CkJT6mE2IU+NYAwHwYDVR0jBBgwFoAUz6dE8qFiUPA56ZKF49pQ532wOqgwCgYIKoZIzj0EAwIDRwAwRAIga1/qf93OZYQ7Jdam/IpNtzuAseZELqsGd6k+Pbk1HyICIFlbgjJ5IcKPrSBiuSrqB8Q3pU1GpiyL5u77aVuKsUQWMEQCIDLFzD5ygx5Si2oi4RBWv/xEpXBNkLmlFxruVJ2yF0+vAiAYnlGuZ3xS87o3eafg2+2eHjV/WVUZ/PC5F7WntmZ/I5AA
Asking a security key to sign now, with the data from the register...
Sign result: AQAAABkwRgIhAK7Ib2SFxSDyY3vhcaKNxMJs6rUap6g/NnXrqip5GIemAiEAvJwdEAyD7DYUcvJDVOT+M7UCKs+tkYhKf8g0LBcexm2QAA==
Done.

These break the signature checks at Github and at https://demo.yubico.com/u2f

Fuzz the U2F protocol parsers

Some casual fuzzing with cargo-fuzz shouldn't take too long to implement.

Pass proper dict to IOHIDManagerSetDeviceMatching()

If we set a dictionary for the DeviceUsage and DeviceUsagePage we don't have to query these later and just rely on CF to do the matching for us.

Crash on OSX when removing token

Pulling out a token usually causes a crash, while adding one works OK.

The crash happens here:

https://github.com/jcjones/u2f-hid-rs/blob/master/src/macos/mod.rs#L83

u2f_register must accept a list of already-registered key handles

Like u2f_sign, u2f_register needs to accept an exclusion list of key handles that it can attempt to use before proceeding.

This could be done by having u2f_register accept an array of byte arrays, or defining a function that checks a single key handle at once (is_registered) which can be called serially by the U2FManager.

Add rustfmt to the test framework

Refactor U2FManager out of main.rs

U2FManager belongs somewhere other than main.rs.

Perhaps pull that into a state_machine.rs and, similarly, refactor lib.rs's u2f hid support stuff into u2fprotocol.rs

Macro-ize ctap2/server.rs map-matching algorithm

this construct that you do over and over should be a macro

Originally posted by @jcjones in #78

Use pretty-hex for AAGuid

replace with https://docs.rs/pretty-hex/0.1.0/pretty_hex/fn.simple_hex.html ?

Originally posted by @jcjones in https://github.com/jcjones/authenticator-rs/diffs/8

Transmit garbage challenge and application to u2f_register() when signing

When we're in .sign() and u2f_is_keyhandle_valid() returns false for a given device we call u2f_register() to keep it blinking. We should submit garbage values instead of the actual challenge and application values.

TSAN shows a data race in the unit tests

RUSTFLAGS="-Z sanitizer=thread" rustup run nightly cargo test

running 4 tests
test u2fprotocol::tests::test_init_device ... ok
test u2fprotocol::tests::test_ping_device ... ok
test u2fprotocol::tests::test_sendapdu ... ok
==================
WARNING: ThreadSanitizer: data race (pid=35046)
  Write of size 8 at 0x7e8000284070 by thread T4:
    #0 memmove <null>:65020504 (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0xc206)
    #1 pthread_setname_np <null>:63970968 (libsystem_pthread.dylib:x86_64+0x421a)

  Previous write of size 8 at 0x7e8000284070 by thread T1:
    #0 memmove <null>:65020504 (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0xc206)
    #1 pthread_setname_np <null>:63970968 (libsystem_pthread.dylib:x86_64+0x421a)

  Thread T4 (tid=5431922, running) created by main thread at:
    #0 pthread_create <null>:65020504 (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0x7121)
    #1 std::sys::imp::thread::Thread::new::hb0ee40c469bc0ac1 thread.rs:72 (u2fhid-67c8d56994e664bc:x86_64+0x10006d575)
    #2 __rust_maybe_catch_panic lib.rs:98 (u2fhid-67c8d56994e664bc:x86_64+0x10007176c)
    #3 start <null>:63965880 (libdyld.dylib:x86_64+0x5234)

  Thread T1 (tid=5431919, finished) created by main thread at:
    #0 pthread_create <null>:65020504 (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0x7121)
    #1 std::sys::imp::thread::Thread::new::hb0ee40c469bc0ac1 thread.rs:72 (u2fhid-67c8d56994e664bc:x86_64+0x10006d575)
    #2 __rust_maybe_catch_panic lib.rs:98 (u2fhid-67c8d56994e664bc:x86_64+0x10007176c)
    #3 start <null>:63965880 (libdyld.dylib:x86_64+0x5234)

SUMMARY: ThreadSanitizer: data race (libsystem_pthread.dylib:x86_64+0x421a) in pthread_setname_np
==================
test u2fprotocol::tests::test_sendrecv_multiple ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Not-selected tokens should stop blinking when one completes

Right now, tokens that you don't touch continue blinking until the timeout completes, while the one you do touch stops blinking immediately. They should all stop.

Consider using catch_unwind at the C API

catch_unwind might be good in the C API in release builds to avoid crashing the library caller.

Remove all instances of unwrap from the library code

Unwrap is dangerous, since a panic in this code will stop the caller. We shouldn't have any unwrap calls. Replace them with pattern matching or unwrap_or or similar.

Upload to crates.io

It seems like this library could be useful to others and having it on crates.io will make it more discoverable.

Add support for USB devices on Windows 7+

Fix Clippy linting issues

These are the Clippy linting warnings:

warning: this argument is passed by value, but not consumed in the function body
  --> src/util.rs:48:33
   |
48 | pub fn to_io_err<T: Error>(err: T) -> io::Error {
   |                                 ^ help: consider taking a reference instead: `&T`
   |
   = note: #[warn(needless_pass_by_value)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value

warning: very complex type used. Consider factoring parts into `type` definitions
  --> src/util.rs:53:15
   |
53 |     callback: Arc<Mutex<Option<SendBoxFnOnce<(io::Result<T>,)>>>>,
   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(type_complexity)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#type_complexity

warning: use `assert_eq` for better reporting
   --> src/macos/device.rs:107:9
    |
107 |         assert!(bytes.len() == HID_RPT_SIZE + 1);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(should_assert_eq)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#should_assert_eq
    = note: this error originates in a macro outside of the current crate

warning: explicit lifetimes given in parameter types where they could be elided
   --> src/macos/device.rs:139:5
    |
139 | /     fn get_cid<'a>(&'a self) -> &'a [u8; 4] {
140 | |         &self.cid
141 | |     }
    | |_____^
    |
    = note: #[warn(needless_lifetimes)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes

warning: explicit lifetimes given in parameter types where they could be elided
  --> src/macos/monitor.rs:81:5
   |
81 | /     pub fn events<'a>(&'a self) -> TryIter<'a, Event> {
82 | |         self.rx.try_iter()
83 | |     }
   | |_____^
   |
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes

warning: you should consider deriving a `Default` implementation for `platform::PlatformManager`
  --> src/macos/mod.rs:27:5
   |
27 | /     pub fn new() -> Self {
28 | |         Self { thread: None }
29 | |     }
   | |_____^
   |
   = note: #[warn(new_without_default_derive)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive
help: try this
   |
21 | #[derive(Default)]
   |

warning: redundant pattern matching, consider using `is_ok()`
   --> src/macos/mod.rs:135:40
    |
135 |                                 if let Ok(_) = u2f_register(device, &blank, &blank) {
    |                                 -------^^^^^--------------------------------------- help: try this: `if u2f_register(device, &blank, &blank).is_ok()`
    |
    = note: #[warn(if_let_redundant_pattern_matching)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching

warning: explicit lifetimes given in parameter types where they could be elided
  --> src/u2ftypes.rs:18:5
   |
18 |     fn get_cid<'a>(&'a self) -> &'a [u8; 4];
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes

warning: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
   --> src/u2ftypes.rs:167:21
    |
167 |     pub fn to_bytes(ins: u8, p1: u8, data: &[u8]) -> io::Result<Vec<u8>> {
    |                     ^^^
    |
    = note: #[warn(wrong_self_convention)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#wrong_self_convention

warning: redundant pattern matching, consider using `is_err()`
  --> src/u2fprotocol.rs:23:12
   |
23 |     if let Err(_) = init_device(dev, &nonce) {
   |     -------^^^^^^--------------------------- help: try this: `if init_device(dev, &nonce).is_err()`
   |
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching

warning: redundant pattern matching, consider using `is_err()`
  --> src/u2fprotocol.rs:29:12
   |
29 |     if let Err(_) = ping_device(dev, &random) {
   |     -------^^^^^^---------------------------- help: try this: `if ping_device(dev, &random).is_err()`
   |
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching

warning: useless use of `vec!`
   --> src/u2fprotocol.rs:171:53
    |
171 |     let mut res = send_apdu(dev, U2F_VERSION, 0x00, &vec![])?;
    |                                                     ^^^^^^^ help: you can use a slice directly: `&[]`
    |
    = note: #[warn(useless_vec)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#useless_vec

warning: long literal lacking separators
  --> examples/main.rs:52:19
   |
52 |         .register(15000, chall_bytes.clone(), app_bytes.clone(), move |rv| {
   |                   ^^^^^
   |
   = note: #[warn(unreadable_literal)] on by default
   = help: consider: 15_000
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#unreadable_literal

warning: long literal lacking separators
  --> examples/main.rs:64:15
   |
64 |         .sign(15000, chall_bytes, app_bytes, vec![key_handle], move |rv| {
   |               ^^^^^
   |
   = help: consider: 15_000
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#unreadable_literal

warning: unused `#[macro_use]` import
  --> examples/main.rs:10:1
   |
10 | #[macro_use]
   | ^^^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default

warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices. Consider changing the type to `&[...]`
  --> examples/main.rs:14:65
   |
14 | fn u2f_get_key_handle_from_register_response(register_response: &Vec<u8>) -> io::Result<Vec<u8>> {
   |                                                                 ^^^^^^^^
   |
   = note: #[warn(ptr_arg)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#ptr_arg

Add negative tests for AuthenticatorData

Particularly running off buffers

Originally posted by @jcjones in https://github.com/jcjones/authenticator-rs/diffs/8

Support ISO/NFC

see FIDO NFC Protocol Specification v1.0.

Use core-foundation crate

Currently u2fhid just uses core-foundation-sys directly. It seems like some of the unsafe wrappers around core-foundation-sys could be replaced with safe ones from core-foundation.

Add Clippy to the Travis CI builds

Rust-Clippy is the preferred linter for Rust projects, so we should run it on every build.

https://github.com/rust-lang-nursery/rust-clippy

Support TravisCI

Implement an actual WorkQueue for the U2FManager

The first version of the U2FManager was added to provide the right API. Because PlatformManager::cancel() can block however, we need implement a work queue that spawns a thread and has register, sign, and cancel as work items. These corresponds to API calls from PBackground.

Fix test_init_device

The fix for #4 has caused the test for ping to fail:

---- tests::test_init_device stdout ----

	USB send: 00ffffffff860008be87e369afa2756200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

thread 'tests::test_init_device' panicked at 'assertion failed: `(left == right)` (left: `[0, 255, 255, 255, 255, 134, 0, 8, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]`, right: `[0, 255, 255, 255, 255, 134, 0, 8, 190, 135, 227, 105, 175, 162, 117, 98, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]`)', src/lib.rs:468

The current assertion that the in and out are identical needs to change; the test can just assert that everything is identical except the 8 nonce bytes by slicing around them.

Copy bytes instead of using unsafe casting for HID types

We basically should get rid of from_u8_array() and to_u8_array(). There shouldn't be any performance reason why we have to do unsafe things here.

Compile error on OSX with rand 0.6

Works fine with rand v0.5.5

   Compiling rand v0.6.4
error[E0425]: cannot find function `pthread_atfork` in module `libc`
   --> /Users/jcjones/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.6.4/src/rngs/adapter/reseeding.rs:320:28
    |
320 |             unsafe { libc::pthread_atfork(None, None, Some(fork_handler)) };
    |                            ^^^^^^^^^^^^^^ not found in `libc`

lib.rs::init_device must use a real nonce

Current code:

    // TODO This is not a nonce. This is the opposite of a nonce.
    let nonce = vec![0x8, 0x7, 0x6, 0x5, 0x4, 0x3, 0x2, 0x1];

linux/hidraw.rs needs tests

Writing tests is really straightforward, we just need to do it. Could probably also add a fuzzing target.

update to winapi 0.3

I'm working on an aarch64 windows Firefox port, and the necessary modifications to winapi are more easily done on the 0.3.x development line rather than the 0.2.x versions. So it'd be great if this code was using 0.3, so when the aarch64 changes land, this code would automatically get upgraded.

Remove dependency on rust-crypto

We're only using SHA256, and even there we're only using it for test code. Move the test code maybe?

Custom errorkinds shouldn't use a magic number

Not all errors should be 42.

Originally posted by @jcjones in https://github.com/jcjones/authenticator-rs/diffs/8

Add coverage to the CI tests

Support Bluetooth

Combine platform managers, use consistent Transaction interface

With #57 fixed, all platform managers look very, very similar. The Transaction implementations contain the platform-specific stuff. We can now have a single state machine that creates platform-specific transactions.

Rename project authenticator-hid-rs?

This will likely grow to include a CTAP implementation, (#33) so perhaps this should be named 2fa-hid-rs?

Use byteorder::ReadBytesExt instead of a local implementation

read_uint_le is very similar to byteorder::ReadBytesExt, as @mozkeeler pointed out:

https://github.com/jcjones/u2f-hid-rs/blob/26a203290fbb5c912fc109b97f7ba2634725f2cd/src/hidproto.rs#L137-L143

We should try switching to byteorder::ReadBytesExt (and adding it as a dependency).

KeyHandleMatcher might theoretically not handle re-used dev_refs correctly

From KeyHandleMatcher.update():

// TODO In theory, it might be possible to replace a token between
// `update()` calls while reusing the device_ref/path. We should refactor
// this part of the code and probably merge the KeyHandleMatcher into the
// DeviceMap and Monitor somehow so that we query key handle/token
// assignments right when we start tracking a new device.

I'd certainly favor coming up with a nicer structure for the KHM, DevMap, and Monitor that fits the runloop we have better, yet still maintains some separation of concern.

Make the logging compatible with Gecko

I don't know how exactly we handle logging when loaded by Gecko, but whatever it is, it's probably not println!().

Don't use the `mach` crate on OSX

The mach crate is only being used for some constants, and it has an incompatible license. Pull it.

Support CTAP 2.0

Review Draft: https://fidoalliance.org/specs/fido-v2.0-rd-20161004/fido-client-to-authenticator-protocol-v2.0-rd-20161004.html

Add tests for macOS/transaction.rs

Need moar tests.

Refactor the unit test platform driver out of lib.rs

mod tests in lib.rs probably should move into its own file.

Respect REQUIRE_USER_VERIFICATION

The flag REQUIRE_USER_VERIFICATION can be passed in by callers, which cannot be satisfied by U2F devices. As such, REQUIRE_USER_VERIFICATION should not permit any U2F devices to complete the operation.

Additionally, REQUIRE_USER_VERIFICATION is mis-defined (but unused) as being both 1 and 2 in src/lib.rs, while it is only 2 in u2fhid-capi.h which will need fixing.

Return consistent custom error codes

We currently usually return io::Errors. We should have a nice list of error codes, maybe defined by the standard, that allow callers of the API to determine the cause of the error and provide better feedback in the JS console.

Support Android

There are Android stubs, and Android now has native U2F support available, so this should make those stubs call out to the native support. That should be pretty clean based on how the interface looks, as the state machines are handled by the OS.

Don't use mem::transmute in OSX platform driver

There's still at least one mem::transmute in use in mod.rs:get_int_property(); let's not do that.

Yubikey ping packet corruption when device is inserted during polling

I've been using my yubikey in firefox for U2F (on windows) and have noticed it almost never works if I insert the USB device after the registration/sign sequence has begun. I have tried it directly with this library and it also gives problems there with the default example (main). If the key was inserted before starting it works fine, if I insert it after starting while it's polling, it sometimes works but most of the time it will fail.

I tracked the problem down to the ping function returning different bytes than the random bytes that were sent. In fact the sequence returned is pretty much always the same. I think it may be a hardware issue with the yubikey, not with your source code and was wondering if you've come across this issue? My yubikey is a few years old (U2F firmware is 1.1.0) so it might work with newer versions.

Using a USB capture I found out that when the ping request is sent, between the request packet and response packet there is a bunch of CCID/ISO7816 commands (note that my yubikey is in U2F+CCID mode), this is probably windows checking the smartcard type. The bytes that are returned in the ping response actually seem to come from parts of earlier CCID responses, so I suspect the yubikey's memory where the ping request is stored is corrupted by these intervening packets. When the registration is done some time after initialization it works because there is no other simultaneous communication.

Properly handle absence of libudev on Linux

Currently, we just time out when dlopen(libudev.so.1) fails. We should fail immediately.

mozilla / authenticator-rs Goto Github PK

authenticator-rs's People

Contributors

Stargazers

Watchers

Forkers

authenticator-rs's Issues

Recommend Projects

Recommend Topics

Recommend Org