search

Yubico / python-fido2

Provides library functionality for FIDO 2.0, including communication with a device over USB.
BSD 2-Clause "Simplified" License
432 stars 109 forks source link

adds tpm support #72

Closed baloo closed 5 years ago

baloo commented 5 years ago

This commits adds support of windows hello devices I found, to achieve support we needed to :

I believe this fixes #52

fdennis commented 5 years ago

I'm no expert but I have been doing some reading on this topic and thought I might add my two cents.

Great job implementing most of what is needed but I couldn't find the following parts:

  1. "Verify that the public key specified by the parameters and unique fields of pubArea is identical to the credentialPublicKey in the attestedCredentialData in authenticatorData"
  2. "Verify that attested contains a TPMS_CERTIFY_INFO structure as specified in [TPMv2-Part2] section 10.12.3, whose name field contains a valid Name for pubArea, as computed using the algorithm in the nameAlg field of pubArea using the procedure specified in [TPMv2-Part1] section 16."
  3. Parts from "Verify that aikCert meets the requirements in § 8.3.1 TPM Attestation Statement Certificate Requirements." are missing such as: 3.1. "Subject field MUST be set to empty.", the subject field is never checked, although you added a boolean. 3.2. "The Subject Alternative Name extension MUST be set as defined in [TPMv2-EK-Profile] section 3.2.9." 3.3. "The Extended Key Usage extension MUST contain the "joint-iso-itu-t(2) internationalorganizations(23) 133 tcg-kp(8) tcg-kp-AIKCertificate(3)" OID."

(from https://w3c.github.io/webauthn/#sctn-tpm-attestation)

Please feel free to correct me if some of these parts are not missing and I just missed them.

baloo commented 5 years ago

I should probably feel ashamed, but I haven't looked at the "verification procedure" section of the spec. I only looked at the signature section :/ to try to have it parsed. Thanks for the feedback, I'll try to push fixes for that.

baloo commented 5 years ago

@fdennis I think I fixed your points 1 and 3, but I'm not too sure I understand how to implement the second one:

Verify that attested contains a TPMS_CERTIFY_INFO structure as specified in [TPMv2-Part2] section 10.12.3, whose name field contains a valid Name for pubArea, as computed using the algorithm in the nameAlg field of pubArea using the procedure specified in [TPMv2-Part1] section 16.

name field in the pubArea is expected to be NULL (and is in my samples). And I'm not too sure which field to lookup the TPMS_CERTIFY_INFO from either. Any advise is welcome.

I did not implement ECC keys yet, quite on purpose. I'd prefer to get feedback on the "form" of the RSA verification first, I'd expect ECC to be simple to add once we agree how to proceed.

dainnilsson commented 5 years ago

Whenever I try to use io.BytesIO I run into issues which usually cause me to go with regular array slicing instead (which has it's own, different, issues), and I think you're hitting some of those same problems.

  1. The fact that read(n) can return less than n bytes, requireing length checks.
  2. The fact that you often want to read some bytes, and then struct.unpack a single value from that, resulting in:

a_var = struct.unpack(">I", reader.read(4))[0]

On the other hand, array slicing has the problem of manually keeping track of the offset, and for the amount of values you're reading out I can certainly see why you would opt for io.BytesIO instead.

So, I though that maybe we should "solve" this problem by introducing a new fido2.utils.ByteBuffer class:

class ByteBuffer(BytesIO):
    """BytesIO-like object with the ability to unpack values."""

    def unpack(self, fmt):
        """Reads and unpacks a value from the buffer.

        :param fmt: A struct format string yielding a single value.
        :return: The unpacked value.
        """
        s = struct.Struct(fmt)
        return s.unpack(self.read(s.size))[0]

    def read(self, size=-1):
        """Like BytesIO.read(), but checks the number of bytes read and raises an error
        if fewer bytes were read than expected.
        """
        data = super(ByteBuffer, self).read(size)
        if size > 0 and len(data) != size:
            raise ValueError(
                "Not enough data to read (need: %d, had: %d)." % (size, len(data))
            )
        return data

which would transform this:

data_len = struct.unpack("!H", reader.read(2))[0]
data = reader.read(data_len)
if data_len != len(data):
    raise ValueError("data is too short")

clock = struct.unpack("!Q", reader.read(8))[0]
reset_count = struct.unpack("!L", reader.read(4))[0]
restart_count = struct.unpack("!L", reader.read(4))[0]

into this (assuming you also don't mind skipping the intermediate data_len variable):

data = reader.read(reader.unpack("!H"))

clock = reader.unpack("!Q")
reset_count = reader.unpack("!L")
restart_count = reader.unpack("!L")

What do you think, seem like a good idea? I think that class would be useful in other parts of this project as well.

baloo commented 5 years ago

@dainnilsson I like the idea of our own reader, introduced in the last commit.

mstingl commented 5 years ago

@baloo I just tested your latest commit a1b1ee3 on my surface book 2 with my project and registration and login works fine 👍

mkalioby commented 5 years ago

I'm still getting none on Huawei MateBook X Windows 10 1903 on Mozilla Firefox, but it shows tpm on Google Chrome. Both work for registration and auth and it used to work from fido2==0.7

mstingl commented 5 years ago

I also tested Windows Hello in combination with a AMD fTPM, also works for me with a1b1ee3

baloo commented 5 years ago

The ECC support is still missing. Ideally I'd like to test support for it. I can't find any platform with ECC keys. TPM 2.0 should be able to issue those, but I'm not sure they're used by windows yet. If anyone has platform with ecc keys, I'd be happy to have samples :)

dainnilsson commented 5 years ago

I don't really like the idea of adding the aenum dependency. It doesn't seem to be packaged in a lot of Linux distros which would mean adding it as a dependency will make it more work for others to package this library (compared to enum34 which is available pretty much everywhere).

I think we should stick to IntEnum for now, even if I agree that IntFlag would be better suited.

baloo commented 5 years ago

@dainnilsson how about vendoring aenum (like, try to load from system (>py3.6) otherwise, vendor version)? I might be missing something, but I don't see how I can make any use of IntEnum for that usecase :(

baloo commented 5 years ago

alright, yes it looks like that works.

baloo commented 5 years ago

@dainnilsson I believe I'm mostly done with this. I do not have test for ECC keys in TPM (which I would have like to have), but I'm unable to find a platform that use those :(

dainnilsson commented 5 years ago

Great, thanks for your work on this!

Can I get a quick statement from you agreeing that you're contributing this code to the project under the BSD 2 clause license (which you can find in the COPYING file in the root of the repository)?

baloo commented 5 years ago

My employer and I agree to contribute this code under the BSD 2 clause license.

baloo commented 5 years ago

If I may ask, I'd like if you could tag a release with this, that makes it easier for me to release/deploy :D

dainnilsson commented 5 years ago

I'll aim to get a new release of this out by the end of the week! I added some minor modifications here: https://github.com/Yubico/python-fido2/commits/tpm-support. Please take a look and let me know if you have any objections, otherwise I think we're good to merge!

baloo commented 5 years ago

@dainnilsson looks good to me, I added a minor fix to the ECC parsing in #73