awwad / uptane

Uptane, security framework for automotive updates
https://uptane.github.io/
MIT License
10 stars 42 forks source link

Initial PR for BER encoding for Director targets metadata #29

Closed awwad closed 7 years ago

awwad commented 7 years ago

The first of the three commits here so far is a few files directly from @trishankkarthik's uptane/asn1 repo. (Thanks, Trishank.) It includes asn.1 definitions for some of the metadata (There's more in that repo.). Unfortunately, there have been some bugs and I'm still sorting through them.

The second commit adds some functionality to pull it in, such that ber.encode_signed_json_metadata_as_ber(<filename of targets.json>) will correctly encode and re-sign targets role metadata... as long as there aren't any images listed. If there are image files listed, the encoder currently borks. I'll work on this ASAP.

There's a lot to still do here with targets.json, including:

The third commit also pulls in timestamp.json BER encoding and signing, mostly for testing purposes. (It works, via the same call from the second commit, ber.encode_signed_json_metadata_as_ber(<filename of timestamp.json>), but I won't need it for the cross-CAN partial verification Secondary workflow. (PV Secondaries don't take in timestamp metadata - just timeserver attestations and Director targets metadata.) Most of the configuration in this third commit is from @trishankkarthik's uptane/asn1 repo again. (More will probably get pulled in in the coming days, too.)

After that, I'll move on to the timeserver attestations and then finish plugging this code into the live demo server code.

trishankkarthik commented 7 years ago

Let me know how I can help!

awwad commented 7 years ago

It would be helpful to figure out why a targets.json metadata file with targets fails to be processed. I think it's just a bug in the format specification, but you're more familiar with it.

It's probably easiest to just run import uptane.ber_encoder.encode_signed_json_metadata_as_ber() and give it the filename of a targets.json file I've just emailed you as a sample to replicate the issue above.

If you would like, you can try running your asn1/README.py function and approaching things that way instead. You'd just need to fix your json.load calls for the json metadata to use 'r' instead of 'rb' reads. I don't remember if there were other fixes necessary.

awwad commented 7 years ago

The demo Director now additionally generates targets.ber on top of targets.json, and hosts it in the same way.

The Primary does not yet retrieve this BER file from the Director, as there are questions about how to do this cleanly and securely. It could be retrieved as a target file, inhering the full TUF protections, but this would require that the map file (pinning.json) add a delegation to the Director alone for targets.ber, which would be an unfortunate addition of complexity.... We could otherwise download the file using some additional, TUF-independent logic (and then, ultimately, validate it on the Primary in a similar manner to a target file).

Hm. This bears some thought. Ideally, TUF would support the additional encoding so that it could be treated identically to JSON metadata, but that is not currently practical.

Right now, I'm leaning toward targets.ber being a target file, which will require at least two special exceptions in the logic for the director targets.json file:

trishankkarthik commented 7 years ago

Fixed the bug you sent in https://github.com/uptane/asn1/commit/07a44b959749d665d437cacc6cb804126c72f51f

Just send me the JSON files that fail to convert to BER anymore, and I'll fix the bugs

awwad commented 7 years ago

This is the error I get, which remains with those edits.


>>> ber.encode_signed_json_metadata_as_ber('targets.json')
Traceback (most recent call last):
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/univ.py", line 92, in prettyIn
    return int(value)
ValueError: invalid literal for int() with base 10: b'sha512'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/s/w/uptane/uptane/ber_encoder.py", line 71, in encode_signed_json_metadata_as_ber
    asn_signed = relevant_asn_module.get_asn_signed(json_signed)
  File "/Users/s/w/uptane/uptane/encoding/targetsmetadata.py", line 20, in get_asn_signed
    set_asn_targets(json_signed, targetsMetadata)
  File "/Users/s/w/uptane/uptane/encoding/targetsmetadata.py", line 172, in set_asn_targets
    hash['function'] = int(HashFunction(hash_function.encode('ascii')))
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/univ.py", line 22, in __init__
    self, value, tagSet, subtypeSpec
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/base.py", line 74, in __init__
    value = self.prettyIn(value)
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/univ.py", line 95, in prettyIn
    'Can\'t coerce %r into integer: %s' % (value, sys.exc_info()[1])
pyasn1.error.PyAsn1Error: Can't coerce b'sha512' into integer: invalid literal for int() with base 10: b'sha512'
>>>
…
awwad commented 7 years ago

Bah, I think that last one may have been a python3 incompatibility thing. It's odd: the timestamp metadata and no-image targets metadata validated in python3, but when you add target info, that's when it fails in python3? The encoding is completing without error in python2. I'll make sure it decodes correctly.

trishankkarthik commented 7 years ago

Yeah, let's test on Python 2 first, so that we are not tripped by pyasn1's incompatibility with Python 3. Once we fix our bugs in Python 2, then we can figure out how to make pyasn1 work with Python 3...

awwad commented 7 years ago

Yes, I'm working in Python2 now. I was lulled into a false sense of security by some seemingly-successful encodings in Python3 previously.

trishankkarthik commented 7 years ago

Actually, it looks like pyasn1 is Python3-compatible, but my code isn't. In any case, we'll fix it later!

awwad commented 7 years ago

It'll be fixed as I gradually rewrite things, then.

On Mon, Jan 30, 2017 at 12:12 PM Trishank Karthik Kuppusamy < notifications@github.com> wrote:

Actually, it looks like pyasn1 is Python3-compatible, but my code isn't. In any case, we'll fix it later!

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/awwad/uptane/pull/29#issuecomment-276123744, or mute the thread https://github.com/notifications/unsubscribe-auth/AMpkOBaP96zffjyelz7_4E8TLZGnOn_Xks5rXhoRgaJpZM4LvoOO .

awwad commented 7 years ago

I have a sizable bug in this. The signature isn't included in what I have the Director hosting. Working on it now.

trishankkarthik commented 7 years ago

What's missing?

awwad commented 7 years ago

Bugs I've found fixed.

Additionally, code exists to convert back to a simple dict (compatible with the format we're using for JSON encoding), and I've tried it out with some sample Director files and confirmed that they're equal. (uptane.ber_encoder.convert_signed_ber_to_unsigned_json())

Remaining tasks:

  1. Test the signatures to make sure they're as expected.
  2. Plug in timeserver attestations next (should be much quicker, assuming the ASN.1 definition for them is correct - still quicker, either way, though).
  3. Primary must retrieve the BER targets file from the Director, decode it, validate its signature, and compare the data to the JSON data it retrieves to ensure that they are equal (otherwise additional attacks are possible).
  4. Fun Times With CAN Hardware
  5. Distributing the BER file through Sam's library (which is imported as a C lib in the Python).
  6. Rewrite of targetsmetadata.py and timeservermodule.py (for clarity, security, and python3 compatibility)

I'm already not using metadata.py and README.py from the uptane/asn1 repo (replaced by ber_encoder.py). There are a few lines in ber_encoder that are still just kinda magic (which isn't acceptable). Security review follows....

Much less urgent, and general (captured by other issues):

  1. Add support for hardware identifiers (general - not specific to encoding; easy, but touches a number of places).
  2. Add support for release counters (general - not specific to encoding)
  3. Good unit testing.

Once I have some ECU Manifests from Sam's PV Secondary, I can also ensure that the Primary handles them correctly and that the Director knows how to verify their signatures and convert their contents.

awwad commented 7 years ago

Regarding Item 3: Shifting tacks slightly: have decided that the Primary will retrieve BER data via TUF, treating the BER file like any other metadata file (rather than attempting to duplicate protections through a separate mechanism). Consequently, I'm moving ASN.1/BER support into TUF. The first commit for that is here.

awwad commented 7 years ago

I've added all the code to TUF that should be required to make it fully ASN.1/BER-compliant (all metadata types). I'm still debugging some stuff. The commits now pushed here cover most of it, though that code is still using some external stuff modified from uptane/asn1 for the metadata definitions in ASN.1. I'm pulling in the necessary code next.

awwad commented 7 years ago

Good news: I've worked through the myriad bugs and gotten TUF working with ASN.1/BER as best I can tell! (It's been somewhat ugly, but when there's time, I think I have an image of how this should all look when it's refactored.)

These changes to TUF are visible here in awwad/tuf@pinning_w_ber. (I've tried to start splitting my commits into one for each individual issue, so hopefully that helps.)

As for hooking back up to Uptane, the quick test I ran with the demo Image Repository seemed to work, but more testing is merited tomorrow (Thursday), along with reconciling Uptane with it (which should be quick). I'll hold onto the Uptane-side commits until more testing.

(This also includes the switch to octal from hexstrings, which hopefully saves some space.)

awwad commented 7 years ago

I'm using DER instead of BER now and that seems to have had no impact. TUF itself supports DER now. I've kept the newest stuff to awwad/uptane instead of uptane/uptane because it doesn't connect all the way through the Secondaries yet, and I don't want to break people's stuff if they're experimenting with it.

The Director and Image Repo generate metadata live and sign and host it as DER. The Primary downloads it and fully validates the DER metadata. Distribution to Secondaries (FV and C PV) comes next. The FV Secondary validation should be trivial as long as there are no more weird pyasn1 conversion bugs. Roping in the C & CAN code will be a bit trickier, but that's the next focus.

There were still more bugs in the ASN1 conversion, needless to say. I've worked through every bug I've yet found (commits here - pushed those from before and after I was sick) with the exception of three things that aren't of urgent import (though I'll certainly get to them after we have a working integrated demo that uses Sam's PV Secondary):

awwad commented 7 years ago

The Full Verification Secondary now handles DER metadata properly, meaning that the Uptane reference implementation and demo now support DER in all components. The console demo including one attack (whose instructions are now in the main README.md) now functions by default in DER (and is still working).

This has been merged into the develop branch on awwad/uptane and has now been pushed to uptane/uptane.

The caveats above about delegations, arbitrary custom info, and RSA keys still hold.

This PR is resolved and I'll open a new one for those three issues when they're ready. The next order of business, though, is working on integrating Sam's PV Secondary demo again!