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

header: fix authentication when protected header is zero-length map #98

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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 src/encrypt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ pub fn enc_structure_data(
) -> Vec<u8> {
let arr = vec![
Value::Text(context.text().to_owned()),
protected.cbor_bstr().expect("failed to serialize header"), // safe: always serializable
protected.to_be_authenticated().expect("failed to serialize header"), // safe: always serializable
Value::Bytes(external_aad.to_vec()),
];

Expand Down
13 changes: 13 additions & 0 deletions src/header/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,19 @@ impl ProtectedHeader {
))
}

pub fn to_be_authenticated(self) -> Result<Value> {
let result = self.cbor_bstr()?;
// protected header might have been encoded as a zero length map, only containing
// the byte 0xA0 (see RFC 9052, Section 3).
// However, this byte should not be included during authentication (RFC 9052, Section 4.4,
// 5.3 and 6.3), so we need to return an empty byte string here instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would ProtectedHeader::cbor_bstr() ever return an (encoded) empty map?

I think the is_empty() check would mean that this could only happen if the original input (in ProtectedHeader::original_data) had the non-minimal encoding, and that object then got re-encoded using the original data. Or is there another way it can happen?

Copy link
Author

Choose a reason for hiding this comment

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

That should be the only way that this can happen, yes.

For actually re-encoding an existing COSE object this should not be an issue (hence why cbor_bstr() is unchanged).

However, as far as I understand the COSE specification for generating the structures that are provided to the signature/encryption/MAC-function (e.g. Sig_structure), this non-minimal encoding needs to be explicitly replaced with a zero-length byte string if no attributes are set, regardless of whether the actual encoding of the protected header is minimal or not.

The aforementioned examples/test cases explicitly check for this behavior (e.g. sign1-tests/sign-pass-01 and sign-tests/sign-pass-01), and in my own tests verification of these (supposedly valid) COSE structures only works with the changes made in this PR.

if result.eq(&Value::Bytes(vec![0xA0])) {
Ok(Value::Bytes(vec![]))
} else {
Ok(result)
}
}

/// Indicate whether the `ProtectedHeader` is empty.
pub fn is_empty(&self) -> bool {
self.header.is_empty()
Expand Down
2 changes: 1 addition & 1 deletion src/mac/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ pub fn mac_structure_data(
) -> Vec<u8> {
let arr = vec![
Value::Text(context.text().to_owned()),
protected.cbor_bstr().expect("failed to serialize header"), // safe: always serializable
protected.to_be_authenticated().expect("failed to serialize header"), // safe: always serializable
Value::Bytes(external_aad.to_vec()),
Value::Bytes(payload.to_vec()),
];
Expand Down
9 changes: 6 additions & 3 deletions src/sign/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,13 +518,16 @@ pub fn sig_structure_data(
aad: &[u8],
payload: &[u8],
) -> Vec<u8> {

let mut arr = vec![
Value::Text(context.text().to_owned()),
body.cbor_bstr().expect("failed to serialize header"), // safe: always serializable
body.to_be_authenticated().expect("failed to serialize header"), // safe: always serializable
];
if let Some(sign) = sign {
arr.push(sign.cbor_bstr().expect("failed to serialize header")); // safe: always
// serializable
arr.push(
sign.to_be_authenticated()
.expect("failed to serialize header") // safe: always serializable
);
}
arr.push(Value::Bytes(aad.to_vec()));
arr.push(Value::Bytes(payload.to_vec()));
Expand Down
Loading