Attestation Object when cbor serialised is invalid (CTAP2)
Firstyear opened this issue · 7 comments
When serialised, the AttestationObject returned is invalid. This is because the types are incorrectly used.
A valid attestation object cbor is:
AttestationObjectInner v=Map({
Text("fmt"): Text("packed"),
Text("attStmt"): Map({
Text("alg"): Integer(-7),
Text("sig"): Bytes([48, 69, ... ]),
Text("x5c"): Array([Bytes([48, 130, ...])])
}),
Text("authData"): Bytes([106, 185, ... ])
})
When serialised, the Attestation Object from authenticator-rs in the ctap 2 branch yields:
AttestationObjectInner v=Map({
Integer(1): Text("packed"),
Integer(2): Array([Integer(73), Integer(150), ... ]),
Integer(3): Map({
Text("alg"): Integer(-7),
Text("sig"): Bytes([48, 69, ...]),
Text("x5c"): Array([Bytes([48, 130, ... ])])
})
})
While the integer keys are correct for ctap, string keys are used for webauthn. Also note that the content of authData are incorrectly serialised as an array of integers and NOT a byte array as required.
cc @msirringhaus as I can't assign the CTAP2 label.
The following very rough diff resolves
diff --git a/src/ctap2/attestation.rs b/src/ctap2/attestation.rs
index 29abf00..a68184e 100644
--- a/src/ctap2/attestation.rs
+++ b/src/ctap2/attestation.rs
@@ -531,36 +531,39 @@ impl Serialize for AttestationObject {
S: Serializer,
{
// serializer.serialize_bytes(&v)
- let mut map_len = 2;
- if self.att_statement != AttestationStatement::None {
- map_len += 1;
- }
-
+ let map_len = 3;
let mut map = serializer.serialize_map(Some(map_len))?;
- map.serialize_entry(
- &2,
- &self
+ let auth_data =
+ self
.auth_data
.to_vec()
- .map_err(|_| SerError::custom("Failed to serialize auth_data"))?,
+ .map(|v| serde_cbor::Value::Bytes(v))
+ .map_err(|_| SerError::custom("Failed to serialize auth_data"))?;
+
+ map.serialize_entry(
+ &"authData",
+ &auth_data
)?;
match self.att_statement {
AttestationStatement::None => {
- map.serialize_entry(&1, &"none")?;
+ // Even with Att None, an empty map is returned in the cbor!
+ map.serialize_entry(&"fmt", &"none")?;
+ let v = serde_cbor::Value::Map(std::collections::BTreeMap::new());
+ map.serialize_entry(&"attStmt", &v)?;
}
AttestationStatement::Packed(ref v) => {
- map.serialize_entry(&1, &"packed")?;
- map.serialize_entry(&3, v)?;
+ map.serialize_entry(&"fmt", &"packed")?;
+ map.serialize_entry(&"attStmt", v)?;
}
AttestationStatement::FidoU2F(ref v) => {
- map.serialize_entry(&1, &"fido-u2f")?;
- map.serialize_entry(&3, v)?;
+ map.serialize_entry(&"fmt", &"fido-u2f")?;
+ map.serialize_entry(&"attStmt", v)?;
}
AttestationStatement::Unparsed(ref v) => {
// Unparsed is currently only used in the fido1.0/u2f-case
- map.serialize_entry(&1, &"fido-u2f")?;
- map.serialize_entry(&3, v)?;
+ map.serialize_entry(&"fmt", &"fido-u2f")?;
+ map.serialize_entry(&"attStmt", v)?;
}
}
impl Serialize for AttestationObject {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
// serializer.serialize_bytes(&v)
let map_len = 3;
let mut map = serializer.serialize_map(Some(map_len))?;
let auth_data =
self
.auth_data
.to_vec()
.map(|v| serde_cbor::Value::Bytes(v))
.map_err(|_| SerError::custom("Failed to serialize auth_data"))?;
map.serialize_entry(
&"authData",
&auth_data
)?;
match self.att_statement {
AttestationStatement::None => {
// Even with Att None, an empty map is returned in the cbor!
map.serialize_entry(&"fmt", &"none")?;
let v = serde_cbor::Value::Map(std::collections::BTreeMap::new());
map.serialize_entry(&"attStmt", &v)?;
}
AttestationStatement::Packed(ref v) => {
map.serialize_entry(&"fmt", &"packed")?;
map.serialize_entry(&"attStmt", v)?;
}
AttestationStatement::FidoU2F(ref v) => {
map.serialize_entry(&"fmt", &"fido-u2f")?;
map.serialize_entry(&"attStmt", v)?;
}
AttestationStatement::Unparsed(ref v) => {
// Unparsed is currently only used in the fido1.0/u2f-case
map.serialize_entry(&"fmt", &"fido-u2f")?;
map.serialize_entry(&"attStmt", v)?;
}
}
map.end()
}
}
This is currently by design, or rather was not (yet) the goal.
The CTAP1-branch also did not package the return values 'webauthn-ready', but returned the answer of the token "as is". Leaving it up to the users to repackage as they need it.
This would need to be a more general discussion about the 'stated' goals of the crate and possible downsides (if there are any).
@msirringhaus Thinking about it more overnight, I think the main question is if authenticator-rs want's to be "firefox internal only" or if it wants to expose a "usable interface'>
Additionally, where it might get a bit more weird is with tpm and other attestation types. Anyway for now I can work around it in webauthn-authenticator-rs :)
@msirringhaus The alternative is you'll need to make ctap2::attestation public because currentl AttestationStatement is blocked as a private element so you can't deconstruct AttestationObject as a public library without fiddling. you could hack it with some weird cbor magic I guess ... but not nice.
Ok, I have a working prototype of this going, incl. changes in Firefox. Seems like there are no real downsides here, except for Firefoxes "Force None-Attestation"-option, which now needs to be passed into the C-API (or one would need to parse and rewrite the whole CBOR-answer again on the C++ side, which I would like to avoid).
But, this is only half the story, of course. CTAP1-answers also need to be deconstructed and wrapped in a webauthn-ready answer, to be consistent. This was, until now, not really possible, as I didn't have a DER-parser to split the certification from the signature. PR #185 tries to solve this without pulling in even more dependencies, and can be seen as prep-work for this.
I'm currently just a bit worried, that this path will lead to us pulling in more and more webauthn-specific stuff. Not even sure if that is a bad thing, either.
From a "Firefox-usage" perspective, it's not really that bad, since we can move more C++ code to Rust, which usually is preferable.
I still would like to hear some opinions from actual Mozillians about this :-)
Not even sure if that is a bad thing, either.
Well, this "library" is pretty webauthn specific anyway so it's not that bad? If there are non-webauthn use cases then that would require extra layers to be added here :)