suit-wg / suit-firmware-encryption

Repo for draft-ietf-suit-firmware-encryption
1 stars 5 forks source link

Is limiting algorithms with CDDL needed? #48

Closed kentakayama closed 8 months ago

kentakayama commented 10 months ago

In short, the CDDL in the current draft is TOO MUCH STRICT (judges valid example as invalid = false negative), and one proposed in the PR #47 is TOO LOOSE (judges many invalid examples as valid = false positive). Let me explain some for the better CDDL representations.

While I was creating PR #47, I've wondered the role of CDDL. In most cases it acts like "necessary condition," but sometimes we leverages it to reject some anti-patterns. Writing one like "necessary and sufficient condition" is too tough and sometimes it could be so complicated like regular expressions.

Only for discussion, here are some example binaries and CDDL representations.

Example binaries

Only outer header is shown in these examples. Same issue occurs also on inner header.

Correct-1: AES-GCM in protected header

96([
  / protected: / << {
    / alg / 1: 1 / A128GCM /
  } >>,
  / unprotected: / {
    / IV / 5: h'...'
  },
  ...
]

Correct-2: AES-CTR in unprotected header

96([
  / protected: / h'',
  / unprotected: / {
    / alg / 1: -65534 / A128CTR /,
    / IV / 5: h'...'
  },
  ...
]

Correct-3: AES-CCM in protected header

96([
  / protected: / << {
    / alg / 1: 10 / AES-CCM-16-64-128 /
  } >>,
  / unprotected: / {
    / IV / 5: h'...'
  },
  ...
]

Incorrect-1: AES-GCM in unprotected header

96([
  / protected: / h'',
  / unprotected: / {
    / alg / 1: 1 / A128GCM /,
    / IV / 5: h'...'
  },
  ...
]

Incorrect-2: AES-CTR in protected header

96([
  / protected: / << {
    / alg / 1: -65534 / A128CTR /
  } >>,
  / unprotected: / {
    / IV / 5: h'...'
  },
  ...
]

Incorrect-3: no algorithm id in both protected nor unprotected header

96([
  / protected: / h'',
  / unprotected: / {
    / IV / 5: h'...'
  },
  ...
]

CDDLs

Current: One in current draft

; common definitions
outer_header_map_protected =
{
    1 => int,         ; algorithm identifier
  * label => values   ; extension point
}

outer_header_map_unprotected =
{
    5 => bstr,        ; IV
  * label => values   ; extension point
}

PR47: Allow algorithm id in unprotected header

; common definitions
outer_header_map_protected =
{
  ? 1 => int,         ; algorithm identifier for AEAD
  * label => values   ; extension point
}

outer_header_map_unprotected =
{
  ? 1 => int,         ; algorithm identifier for non AEAD
    5 => bstr,        ; IV
  * label => values   ; extension point
}

Verbose: Limit AEAD only in protected and non AEAD only in unprotected

a128gcm = 1
a128ctr = -65534
a128cbc = -65531
$aead-alg /= a128gcm
$non-aead-alg /= a128ctr
$non-aead-alg /= a128cbc

; common definitions
outer_header_map_protected =
{
  ? 1 => $aead-alg,   ; algorithm identifier for AEAD
  * label => values   ; extension point
}

outer_header_map_unprotected =
{
  ? 1 => $non-aead-alg, ; algorithm identifier for non AEAD
    5 => bstr,          ; IV
  * label => values     ; extension point
}

Quality of CDDL

βœ…: Judged "valid" in CDDL check 🚫: Judged "invalid" in CDDL check πŸ†—: Judged correctly πŸ†–: Judged incorrectly

Example Ideal Current Draft PR #47 Verbose
Correct-1: AES-GCM in protected βœ… βœ…πŸ†— βœ…πŸ†— βœ…πŸ†—
Correct-2: AES-CTR in unprotected βœ… πŸš«πŸ†– βœ…πŸ†— βœ…πŸ†—
Correct-3: AES-CCM in protected βœ… βœ…πŸ†— βœ…πŸ†— πŸš«πŸ†–*
Incorrect-1: AES-GCM in unprotected 🚫 πŸš«πŸ†— βœ…πŸ†— πŸš«πŸ†—
Incorrect-2: AES-CTR in protected 🚫 βœ…πŸ†– βœ…πŸ†– πŸš«πŸ†—
Incorrect-3: no alg in header 🚫 πŸš«πŸ†— βœ…πŸ†– βœ…πŸ†–
False Negative (🚫Invalid πŸ†–Incorrectly => NEED TO BE FIXED) 0 1 0 1*
False Positive (βœ…Valid πŸ†–Incorrectly => LOOSE BUT ACCEPTABLE?) 0 1 2 1

* Even with Verbose CDDL, we can enumerate possible algorithm ids to make them valid.

kentakayama commented 10 months ago

Covered algorithms for each CDDL. image

kentakayama commented 10 months ago

47 just allowed non AEAD cipher algorithm identifiers to be encoded into unprotected header as RFC 9459 requests.

On the other hand, it also allows them to be encoded into protected header (Incorrect-2 above).

It seems that writing the CDDL down to allow all correct SUIT_Encryption_Info binaries and to prohibit all incorrect ones is not realistic and reasonable.

Then, how about changing the CDDL like this ( GREEN CIRCLE above)?

SUIT_Encryption_Info = #6.96(COSE_Encrypt)

It only mentions that the tagged COSE_Encrypt binary is possible, and does not mentions about the values and structures related to the algorithm identifier. It is also good at accepting other key exchange and encryption algorithms in the future.

kentakayama commented 9 months ago

Prohibiting all of mal-formed CBOR binaries with CDDL are not realistic. Further more, listing SUIT_Encryption_Info_AESKW and SUIT_Encryption_Info_ESDH could be misleading because it implies HPKE in COSE is not allowed.

Instead, SUIT_Encryption_Info = #6.96(COSE_Encrypt) is proposed in #51 .

kentakayama commented 8 months ago

After #51 is merged, we can close this Issue as completed.

Our conclusions are,

hannestschofenig commented 8 months ago

Closed with PR