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

Ambiguous instructions in the Android Key Attestation Statement Format verification procedure #1980

Open
vanbukin opened this issue Oct 1, 2023 · 7 comments
Assignees
Labels
@Risk Items that are at risk for L3 type:technical
Milestone

Comments

@vanbukin
Copy link

vanbukin commented Oct 1, 2023

According to: https://w3c.github.io/webauthn/#android-key-attestation

  • Verify the following using the appropriate authorization list from the attestation certificate extension data:
    • The AuthorizationList.allApplications field is not present on either authorization list (softwareEnforced nor teeEnforced), since PublicKeyCredential MUST be scoped to the RP ID.
    • For the following, use only the teeEnforced authorization list if the RP wants to accept only keys from a trusted execution environment, otherwise use the union of teeEnforced and softwareEnforced.
      • The value in the AuthorizationList.origin field is equal to KM_ORIGIN_GENERATED.
      • The value in the AuthorizationList.purpose field is equal to KM_PURPOSE_SIGN.
  • If successful, return implementation-specific values representing attestation type Basic and attestation trust path x5c.

According to the schema, KeyDescription can contain 2 AuthorizationList elements: softwareEnforced and teeEnforced.

And the problem lies in how the specification describes otherwise use the union of teeEnforced and softwareEnforced.

KeyDescription can (and probably should) be interpreted as a singular (indivisible) object. From this, it follows that the formulation should be somewhat different: otherwise, the following conditions must be met for either teeEnforced or softwareEnforced.

The following combinations of parameters should be considered valid:

  • softwareEnforced:
    • purpose - ✅valid
    • origin - ✅valid
  • teeEnforced:
    • purpose - 🚫invalid
    • origin - 🚫invalid

  • softwareEnforced:
    • purpose - 🚫invalid
    • origin - 🚫invalid
  • teeEnforced:
    • purpose - ✅valid
    • origin - ✅valid

  • softwareEnforced:
    • purpose - ✅valid
    • origin - ✅valid
  • teeEnforced:
    • purpose - ✅valid
    • origin - ✅valid

Because currently, the wording implies that you can combine origin from softwareEnforced and teeEnforced, and check that at least one of them contains a valid value, and then do the same for purpose.

And moreover, this is how it is done in the real world.
https://github.com/webauthn4j/webauthn4j/blob/master/webauthn4j-core/src/main/java/com/webauthn4j/validator/attestation/statement/androidkey/KeyDescriptionValidator.java#L122-L134

This leads to a scenario where such a combination of values

  • softwareEnforced:
    • purpose - ✅valid
    • origin - 🚫invalid
  • teeEnforced:
    • purpose - 🚫invalid
    • origin - ✅valid

would pass the validation.

If KeyDescription can be interpreted as not a singular (indivisible) object, then the wording can also be changed to something more comprehensible: otherwise, verify that each of the unions of teeEnforced and softwareEnforced parameter values meet the requirements.
In such case, the scheme outlined above would be considered legitimate.

@Kieun
Copy link
Member

Kieun commented Oct 3, 2023

After reading Android key attestation related docs, I'm thinking that the key description may have both teeEnforced and softwareEnforced at the same time.

See the following links:

So, the current implementation you've provided is valid per the spec. Our implementation has similar validation logics.

@vanbukin
Copy link
Author

vanbukin commented Oct 4, 2023

@Kieun I apologize if my description of the problem was not precise enough. As you correctly pointed out - KeyDescription can simultaneously have both teeEnforced and softwareEnforced. The question lies in how the origin and purpose, which are nested within teeEnforced and softwareEnforced, should be validated. Because currently, the specification states otherwise use the union of teeEnforced and softwareEnforced, and then lists the validation rules for origin and purpose:

- The value in the AuthorizationList.origin field is equal to KM_ORIGIN_GENERATED.
- The value in the AuthorizationList.purpose field is equal to KM_PURPOSE_SIGN.

As an implementer, it's unclear to me how validation should look.

bool isValid = IsOneOfOriginsValid(new [] {keyDescription.teeEnforced.origin, keyDescription.softwareEnforced.origin})
&&
IsOneOfPurposesValid(new [] {keyDescription.teeEnforced.purpose, keyDescription.softwareEnforced.purpose})

or

bool isValid = IsPurposeAndOriginValid(keyDescription.teeEnforced) || IsPurposeAndOriginValid(keyDescription.softwareEnforced)

@Kieun
Copy link
Member

Kieun commented Oct 4, 2023

First of all, the KeyDescription may have teeEnforced and softwareEnforced at the same time. But the thing is that, if the both are presented in the KeyDescription, the same filed must not be present.
For example, teeEnforced.origin field and softwareEnforced.origin field should not be presented together. This is the same for purpose field as well.
If the same fields are present both in teeEnforced and softwareEnforced, that would be the invalid structure.

So, the union means that, you just get authorization list from teeEnforced and softwareEnforced and let it as single authorization list saying temp.
Then, you need to verify (not validate) temp.origin and temp.purpose.

I'm not a native english speaker, but my interpretation was something like this.
It's better to get response from any Googler, @agl .

@vanbukin
Copy link
Author

vanbukin commented Oct 5, 2023

@Kieun You have provided good clarifications. It has become clear to me. It would be helpful to add a small Note to the specification, for the benefit of others as well.

@nadalin nadalin added this to the L3-WD-02 milestone Oct 25, 2023
@nadalin nadalin added the @Risk Items that are at risk for L3 label Oct 25, 2023
@arnar
Copy link
Contributor

arnar commented Jan 24, 2024

Let me verify this with Android experts, but my interpretation is that each security property can be either enforced in software or in TEE, and they can combine independently. E.g. KM_ORIGIN_GENERATED in the teeEnforced would mean the key was generated in the TEE but enforcing that the key is only used for sign ops (KM_PURPOSE_SIGN) could still be enforced by software, and therefore that property would appear in the softwareEnforced field.

I don't actually know if that's a valid combination, it at least seems odd. But assuming it is valid then I think the spec is correct. In essence:

  1. If an RP cares about TEE enforcement, check only the teeEnforced list.
  2. If an RP is ok with software enforcement, check each property appears on /either/ softwareEnforced or teeEnforced, individually.

I.e. there's no reason for an RP that accepts software enforcement to reject a credential where one of these is actually TEE enforced.

I'll double check if this is actually a valid scenario.

@agl
Copy link
Contributor

agl commented Jan 31, 2024

We should poke Arnar in two weeks time about this.

@nadalin
Copy link
Contributor

nadalin commented Mar 20, 2024

@arnar please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@Risk Items that are at risk for L3 type:technical
Projects
None yet
Development

No branches or pull requests

5 participants