CTAP1 requests with no command data payload (eg: U2F_VERSION) are incorrectly encoded
micolous opened this issue · 7 comments
U2FAPDUHeader::serialize() incorrectly encodes Nc = 0 (zero command data length), which makes authenticator-rs incorrectly serialize U2F_VERSION requests.
This issue affects CTAP1-compatible authenticators on all transports (not just NFC).
Per FIDO v1.1 U2F Raw Message Formats and FIDO v1.2 U2F Raw Message Formats, when Nc = 0, there are no Lc bytes. This encoding is adapted from ISO 7816-4:2005 extended APDUs (section 5.1, "Command-response pairs").
For example, a U2F_VERSION request (which has no command data, so Nc = 0) with Ne = 65536 should be:
(CLA)00 (INS)03 (P1)00 (P2)00 (Le)00 00 00
But at present, authenticator-rs sends Lc bytes:
(CLA)00 (INS)03 (P1)00 (P2)00 (Lc)00 00 00 (Le)00 00
Confusingly, FIDO v1.0 incorrectly describes ISO 7816-4:2005 extended APDUs (it doesn't special-case Nc = 0, omits Le, and suggests Lc is a uint24be), and then FIDO v2.0 and v2.1 cite FIDO v1.2 framing while describing FIDO v1.0 framing in-line.
Does this cause any actual errors with specific hardware? Apparently, right now it works for most USB tokens, so I'd be curious how this showed itself.
I haven't found a USB HID token that is broken by this issue, but I only have Yubico keys to test with. Given the specification authors include Google and Yubico employees, they are more likely to have inherited legacy code (bugs and all) from the original "Gnubby" U2F firmware.
FIDO v1.0's description of ISO 7816-4:2005 extended APDUs is very wrong, and there are other errors in the spec which have been mostly addressed in FIDO v1.1 and v1.2 (but then regressed in v2.x, per the commentary in the pull requests).
At least the FIDO v1.2 spec specifies the actual "raw APDU" bytes of the U2F GET_VERSION request – so it's clear there's a bug in authenticator-rs.
I noticed the issue while attempting to write interface code for NFC-enabled CTAP2 tokens (which uses a fork of this library for interfacing with USB HID CTAP2 tokens), and also inspecting #114. As someone who has implemented ISO 7816 before, these sorts of errors are pretty obvious: ISO 7816-3:2006 (sections 12.1.2, 12.1.3) and 7816-4:2005 (section 5.1) both clearly state that (Lc) 00 00 00 (Le) 00 00 is invalid.
As for other potentially impacted authenticators, I suspect that non-smartcard implementations of CTAP1/U2F are more likely to use bespoke parser code, as opposed to smartcard-first authenticators, which are more likely to strictly follow the ISO 7816 specification. It may also cause a problem for NFC hardware itself; though I was able to confirm with a Proxmark3 that my ACR122U (very cheap NFC hardware) sent a malformed GET_VERSION APDU as-is.
Makes sense, thanks for the explanation..
The only worry I have is, that older USB tokens who maybe only implement FIDO 1.0 and implemented APDU 'wrong' (according to FIDO-spec) will not work anymore with this.
But I'm not sure, if there are any of those around.
That's a fair point; however FIDO 1.0 also doesn't mention Le for extended form, so if they rely on Lc being always present, the correct (CLA)00 (INS)03 (P1)00 (P2)00 (Le)00 00 00 would be parsed as (CLA)00 (INS)03 (P1)00 (P2)00 (Lc)00 00 00 with implicit Ne = 65536.
IMHO a token which can't handle a command that's described verbatim in the FIDO v1.2 spec is defective. We should absolutely try to avoid triggering that issue wherever possible, but it would be unjust to break other tokens which implemented these specs correctly.
The current APDU which authenticator-rs sends is invalid. Really, a token should return an error.
I had a look at Chromium's U2F implementation just now; I wasn't able to find anywhere it sent a FIDO1/U2F GET_VERSION command. Their tests state GET_VERSION is never used, and so don't actually implement that command:
switch (parsed_command->ins()) {
// Version request is defined by the U2F spec, but is never used in
// production code.
case base::strict_cast<uint8_t>(U2fApduInstruction::kVersion):
break;Chromium appears to delegate APDU construction to a class which only implements ISO 7816 extended form, and will correctly omit Lc and/or Le as required:
std::vector<uint8_t> ApduCommand::GetEncodedCommand() const {
std::vector<uint8_t> encoded = {cla_, ins_, p1_, p2_};
// If data exists, request size (Lc) is encoded in 3 bytes, with the first
// byte always being null, and the other two bytes being a big-endian
// representation of the request size. If data length is 0, response size (Le)
// will be prepended with a null byte.
if (!data_.empty()) {
size_t data_length = data_.size();
encoded.push_back(0x0);
if (data_length > kApduMaxDataLength)
data_length = kApduMaxDataLength;
encoded.push_back((data_length >> 8) & 0xff);
encoded.push_back(data_length & 0xff);
encoded.insert(encoded.end(), data_.begin(), data_.begin() + data_length);
} else if (response_length_ > 0) {
encoded.push_back(0x0);
}
if (response_length_ > 0) {
size_t response_length = response_length_;
if (response_length > kApduMaxResponseLength)
response_length = kApduMaxResponseLength;
// A zero value represents a response length of 65,536 bytes.
encoded.push_back((response_length >> 8) & 0xff);
encoded.push_back(response_length & 0xff);
}
return encoded;
}There's also a command parser, ApduCommand::CreateFromMessage, which appears to incorrectly treat Lc = 00 00 00 as Nc = 0, rather than an error.
I've added commentary in-line:
uint16_t ParseMessageLength(base::span<const uint8_t> message, size_t offset) {
DCHECK_GE(message.size(), offset + 2);
return (message[offset] << 8) | message[offset + 1];
}
// EDIT: Snipped extra lines
absl::optional<ApduCommand> ApduCommand::CreateFromMessage(
base::span<const uint8_t> message) {
if (message.size() < kApduMinHeader || message.size() > kApduMaxLength)
return absl::nullopt;
// EDIT: Snipped extra lines
default:
// NOTE: message.size() > 7
// Fifth byte must be 0.
if (message[4] != 0)
return absl::nullopt;
auto data_length = ParseMessageLength(message, kApduCommandLengthOffset);
// BUG: doesn't check data_length > 0
if (message.size() == data_length + kApduCommandDataOffset) {
// NOTE: we can't hit this branch with data_length = 0 and Le missing; it's handled earlier
// in the switch block
// No response expected.
data.insert(data.end(), message.begin() + kApduCommandDataOffset,
message.end());
} else if (message.size() == data_length + kApduCommandDataOffset + 2) {
// NOTE: this branch can be hit if data_length = 0 and Le present: (Lc) 00 00 00 (Le) xx xx
// Maximum response size is stored in final 2 bytes.
data.insert(data.end(), message.begin() + kApduCommandDataOffset,
message.end() - 2);
auto response_length_offset = kApduCommandDataOffset + data_length;
response_length = ParseMessageLength(message, response_length_offset);
// Special case where response length of 0x0000 corresponds to 65536
// as defined in ISO7816-4.
if (response_length == 0)
response_length = kApduMaxResponseLength;
} else {
return absl::nullopt;
}
break;
// EDIT: Snipped extra linesHowever, in the interest of due diligence, I'll try to get my hands on some other tokens to see how they respond, and I'd encourage y'all to try it out with any tokens you have too.
For the record: My (tiny) token collection seems to work fine with the patch.
Trying to come up with a simple test with authenticator-rs was too hard for me, because there were too many levels of abstraction. I ended up writing a test program to check whether the devices respond to both malformed (as authenticator-rs sends today) and correct (as my PRs change it to) GET_VERSION requests.
I acquired some tokens from a friend, and also went through my personal collection.
I was unable to find any keys which:
- returned an error for a malformed
GET_VERSIONrequest (ie: are broken without my PR). - only accepted malformed
GET_VERSIONrequests (ie: would be broken by my PR).
These keys accepted both malformed and correct GET_VERSION requests (ie: are not broken by my PR):
- 4 different models of Google-internal security keys
- 0483:cdab: Gl.Sergei U2F-token (EFM32) (Tomu U2F firmware)
- 1050:0402: Yubico Yubikey 4 U2F (v4.3.5)
- 1050:0402: Yubico YubiKey FIDO (Yubikey 5 NFC v5.2.4)
- 1050:0402: Yubico YubiKey FIDO (Yubikey 5 Nano C v5.2.4)
- 1050:0402: Yubico YubiKey FIDO (Yubikey 5C FIPS v5.4.3)
- 1050:0406: Yubico YubiKey FIDO+CCID (Yubikey 5 Nano v5.1.2)
- 20a0:42b1: Nitrokey Nitrokey FIDO2 2.0.0 (v0.0.0)
- 2581:f1d0: Plug-up Plug-up (Nitrokey U2F v1.6.7, the folded plastic/sim-card style, looks similar to Happlink/Plug-up)