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

Bugfix/emv #5

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mmattice
Copy link
Contributor

mmattice commented Mar 17, 2016

This is a small pile of changes required to make EMV tags be capable of being uniquely identified and parsed more correctly. As far as I can tell, these don't interfere with existing tag encoding/decoding and probably should be applied to DER and CER if applicable.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 17, 2016

Current coverage is 84.42%

Merging #5 into master will increase coverage by +0.01% as of c03ab2e

@@            master      #5   diff @@
======================================
  Files           21      21       
  Stmts         2605    2607     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           2199    2201     +2
  Partial          0       0       
  Missed         406     406       

Review entire Coverage Diff as of c03ab2e

Powered by Codecov. Updated on successful CI builds.

@viraptor

This comment has been minimized.

Copy link
Contributor

viraptor commented Mar 23, 2016

Could use a regression test for encoding univ.Integer(2, tag.TagSet((), tag.Tag(tag.tagClassContext, 0, 128))), so it doesn't come back in the future.

@viraptor

This comment has been minimized.

Copy link
Contributor

viraptor commented Mar 23, 2016

Actually, doesn't this patch break my example? What's the original failing case? The integer from example encodes as:

  • with patch: 9f000102
  • without: 9f81000102

According to X690-0207 8.1.2.4.2 a) "bit 8 of each octet shall be set to one unless it is the last octet of the identifier octets". The initial identifier octet doesn't seem to encode any bits on its own - the whole tag number needs to be base-128 encoded, so for 128, you're encoding 7-bit bytes 0x01 0x00

@viraptor

This comment has been minimized.

Copy link
Contributor

viraptor commented Mar 23, 2016

The caching bug is pretty nasty though - definitely should be fixed:

In [1]: from pyasn1.codec.ber import decoder
In [2]: decoder.decode(b'\x1f\x02\x01\x02')
Out[2]: (Integer(2, tagSet=TagSet((), Tag(tagClass=0, tagFormat=0, tagId=2))), b'')
In [3]: decoder.decode(b'\x1f\x02\x01\x02')
Out[3]: (Integer(258, tagSet=TagSet((), Tag(tagClass=0, tagFormat=0, tagId=2))), b'')

Or just rejected with error message... X690-0207 8.1.2.2: "For tags with a number ranging from zero to 30 (inclusive), the identifier octets shall comprise a single octet encoded as follows: "

@mmattice

This comment has been minimized.

Copy link
Contributor Author

mmattice commented Mar 23, 2016

The caching bug should probably be a different pull request. I don't have a problem making that happen if necessary.

As far as the extra bit is concerned I sent an email to pyasn1-users that contained this:

I've run into some problems decoding multiple extended tags starting with 9F (9F02, 9F03, 9F06, etc). If I make the following changes, they decode perfectly. The reason it's a problem is because the tagId's end up being < 31. This makes me wonder if the tag number should be 0x80 after the SHL 7 instead of 0. Otherwise it seems as though (for instance) 82 and 9F02 would decode/encode incorrectly.

I'm not an X690 guru by any means. I've just been tasked with making EMV work. To do so, I have to make the tags in emv-asn1 work with BER encoding, which the above change permits. I've not yet run into any problems (I'm still working through a test suite against our processor) by using this set of patches. It leaves the on-wire BER the way it seems it should be and just changes the internal (to pyasn1) representation to have the extra bits. Of course, it also affects the way the ASN.1 grammar is written to be compatible, which I've noted in the above repo as well.

If this can be fixed some other way, please, let me know.

@viraptor

This comment has been minimized.

Copy link
Contributor

viraptor commented Mar 23, 2016

The tags 82 and 9F02 should be equivalent if accepted. But without some specific option like "yeah_I_really_want_to_break_the_standards=True", I don't think pyasn1 should ever encode 9F02 on its own. The caching fix should sort out the decoding side without issues.

But once decoding is fixed, what other issue did you see? The other patches break encoding currently - for example univ.Integer(2, tag.TagSet((), tag.Tag(tag.tagClassContext, 0, 128))) should be

(0x80 + 0x1F), (0x80 + 0x01), (0x00 + 0x00), 0x01, 0x02
   |      |      |       |       |      +- tag+=0*1
   |      |      |       |       +- no more tag bytes follow
   |      |      |       +- tag += 1*128
   |      |      +- more tag bytes follow
   |      +- tag is >31, spread across extra bytes
   +- context-specific, primitive

But it's not correct anymore with the other patches you posted. I feel it would be easier to talk about if you separate the issues. (then again, it's up to @etingof really :) )

@mmattice

This comment has been minimized.

Copy link
Contributor Author

mmattice commented Mar 24, 2016

82 and 9F02 aren't equivalent. Having them decode from BER into something different is important.

I ran the tests in the package and it didn't fail any of them, so no existing functionality is disturbed, or the coverage is lacking.

@mmattice

This comment has been minimized.

Copy link
Contributor Author

mmattice commented Mar 24, 2016

The following is a base64 encoded BER block that is decoded properly by our vendor. It has both 82 and 9F02 inside it. While I understand it may not be "to spec", unfortunately it's what the EMV council has spec'd. If you use emv-asn1, and asn1ate that grammar, you can decode this. Without it, you cannot. I will look at adding a test for your above example (since it doesn't seem to be covered by the existing tests) and attempt to adjust my second two patches.

'TwegAAAAAxAQUAtWSVNBIENSRURJVIICXACEB6AAAAADEBCVBQIAAIAAmgMWAyGbAugAnAEAXyoCCEBfNAEBnwIGAAAAAAhYnwMGAAAAAAAAnwYHoAAAAAMQEJ8JAgCMnxAHBgEKA6AAAJ8RAQGfEg9DUkVESVRPIERFIFZJU0GfGgIIQJ8eCDgwMzMwMTI3nyEDFzMAnyYIEa1OFebMt5KfJwGAnzMD4PjInzQDHgMAnzUBIZ82AgADnzcE5kxstJ85AQWfQAXwAPCgAZ9BBAAAATSfUwFS'

@etingof

This comment has been minimized.

Copy link
Owner

etingof commented Mar 27, 2016

@mmattice Could you please elaborate on "82 and 9F02 aren't equivalent"? From BER perspective they seem equivalent, no? Although 9F02 looks strange as it employs large tag infrastructure for encoding small tags (<=30).

Would it be possible to ask someone from EMV to explain how 82 could be made different from 9F02 when decoding by a strict BER decoder?

BTW, I've added some more tests to the test suite.

@mmattice

This comment has been minimized.

Copy link
Contributor Author

mmattice commented Mar 27, 2016

One is the amount of the transaction, the other is an indicator of capabilities.

I would love to get in touch with EMVCo. It's $2500 for a guaranteed response.

EMVCo will review your comment but is not obligated to provide you with a response. Only Subscriber Queries will receive direct EMVCo feedback. If you wish to be a subscriber, SUBSCRIBE NOW!

@mmattice

This comment has been minimized.

Copy link
Contributor Author

mmattice commented Mar 28, 2016

After finding https://github.com/Shopify/grizzly_ber and this lovely tidbit:

This works differently from OpenSSL::ASN1 in that it does not decode tags (EMV-style). This is important when processing a long tag like 9F02 (Amount Authorized in EMV). OpenSSL will write it as the TLV-DER tag 82. GrizzlyBer maintains the original TLV-BER tag 9F02.

I'm closing this PR. Thanks for entertaining my learning/discovery process.

@mmattice mmattice closed this Mar 28, 2016

@etingof

This comment has been minimized.

Copy link
Owner

etingof commented Mar 28, 2016

@mmattice If you want pyasn1 encoding/decoding tags in a way EMV does, you could customize pyasn1 encoder/decoder code in your application and pass you code to pyasn1 codecs.

That should be fairly easy to do for encoder (overload IntegerEncoder.encodeTag()), to decode colliding tags differently (82 vs 9F02) we could move tag decoding code from Decoder.call() to a separate method (e.g. Decoder.decodeTag?) to make it easily overloadable/customizable.

Would that be a useful change, what do you think?

@mmattice

This comment has been minimized.

Copy link
Contributor Author

mmattice commented Mar 28, 2016

They're not actually following X.680 afaict. Tags 9F05 and 9F06 for instance aren't Null and OID respectively. Also, they're using reserved types like on 9F0F that have no meaning yet. I'm waiting for a response from EMVCo presently on the issue. Although, I'm also not holding my breath.

Even if they miraculously decide they're going to fix their use of BER, I think I'm going to be stuck with this problem for a while yet anyway. I've got a whole (internal only because it's generated code) asn1ate'd asn1spec I'm using to get the EMV tags turned into identifiable objects in my code. Having pyasn1 decode things in a way that's not representable in a proper asn1 grammar (even a kludged one), would make that difficult without hacking the crap out of the pyasn1 internals.

Because what they're doing isn't BER, I'm probably going to dupe the BER encoder/decoder pair into an EMV namespace for my kludge so that it doesn't affect other pyasn1 using things on the system.

I've already got a private repo for a package to encode/decode the Ingenico EMV tag format (which is text based -- 'T82:02:h5800\x1cT9F02:06:h000000008000\x1c' for example). When I asked them directly if they were going to switch to a more standard format like BER, they said they weren't because they had a major customer that wanted the format they now use. I wonder if that major customer didn't run into this same problem and side-stepped the problem this way.

nekolyanich added a commit to nekolyanich/pyasn1 that referenced this pull request Mar 28, 2017

Fix BitString initialization via namedValues
  class KeyUsage(BitString):
      namedValues = NamedValues(
          ('digitalSignature', 0),
          ('nonRepudiation', 1),
          ('keyEncipherment', 2),
          ('dataEncipherment', 3),
          ('keyAgreement', 4),
          ('keyCertSign', 5),
          ('cRLSign', 6),
          ('encipherOnly', 7),
          ('decipherOnly', 8),
      )

  encode(KeyUsage('keyCertSign,cRLSign')) != encode(KeyUsage("'0000011'B"))
                                                                    ^^
                                                                    ||
                                                bit etingof#5 -------------+|
                                                bit etingof#6 --------------+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.