wbond / asn1crypto

Python ASN.1 library with a focus on performance and a pythonic API
MIT License
335 stars 140 forks source link

x509 AnotherName not encoded correctly #231

Open dirkjanm opened 2 years ago

dirkjanm commented 2 years ago

Hey, I'm having some issues with creating a cert request with a subject alt name for Microsoft certificates. It comes down to the asn1 encoding of the AnotherName structure, which either is broken or that I simply don't understand how to construct.

I have the following sample code:

import base64
import asn1crypto
from asn1crypto import core
from asn1crypto import pem
from asn1crypto import x509
PRINCIPAL_NAME = x509.ObjectIdentifier("1.3.6.1.4.1.311.20.2.3")
othername = x509.AnotherName()
othername['type_id']=PRINCIPAL_NAME
othername['value'] = core.UTF8String("testUPN")
name = x509.GeneralName(name="other_name", value=othername)
gnames = x509.GeneralNames()
gnames.append(name)
print(base64.b64encode(gnames.dump()))
# result MBegFQYKKwYBBAGCNxQCAwwHdGVzdFVQTg==

This generates the following ASN.1: image

However, any Microsoft code can't parse this, because it expects a structure like this:

image

I'm not an expert on asn1 but it seems something goes wrong with the explicit tagging of the element. In fact, when I try to debug this, the library throws an error:

  asn1crypto.x509.GeneralNames Object #140695617513360
      constructed universal tag 16
    Data: 0xa015060a2b0601040182371402030c077465737455504e
    asn1crypto.x509.GeneralName Object #140695617512112
      Data: 0x060a2b0601040182371402030c077465737455504e
        asn1crypto.x509.AnotherName Object #140695617514992
          Header: 0xa015
            constructed context tag 0 (implicitly tagged)
          Data: 0x060a2b0601040182371402030c077465737455504e
            Field "type_id"
              asn1crypto.core.ObjectIdentifier Object #140695617712576
                Header: 0x060a
                  primitive universal tag 6
                Data: 0x2b060104018237140203
                  Native: 1.3.6.1.4.1.311.20.2.3
Traceback (most recent call last):
  File "debugasn1.py", line 13, in <module>
    gnames.debug()
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 4570, in debug
    child.debug(nest_level + 1)
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 610, in debug
    self.chosen.debug(nest_level + 2)
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 4095, in debug
    child = self._lazy_child(self._field_map[field_name])
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 3478, in _lazy_child
    child = self.children[index] = _build(*child)
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 5478, in _build
    raise ValueError(unwrap(
ValueError: Error parsing asn1crypto.core.Any - explicitly-tagged class should have been context, but universal was found

Curiously enough, when I change the structure of the object in x509.py here: https://github.com/wbond/asn1crypto/blob/8a4c621e34f8cbdc83b047aa617e66dcbfde0f75/asn1crypto/x509.py#L1167

To the following:

class AnotherName(Sequence):
    _fields = [
        ('type_id', ObjectIdentifier),
        ('value', UTF8String, {'explicit': 0}),
    ]

(so instead of Any specify a UTF8String explicitly) Then it encodes without issue, and the bytes are as expected:

  asn1crypto.x509.GeneralNames Object #140419936165792
      constructed universal tag 16
    Data: 0xa017060a2b060104018237140203a0090c077465737455504e
    asn1crypto.x509.GeneralName Object #140419936162480
      Data: 0x060a2b060104018237140203a0090c077465737455504e
        asn1crypto.x509.AnotherName Object #140419936164928
          Header: 0xa017
            constructed context tag 0 (implicitly tagged)
          Data: 0x060a2b060104018237140203a0090c077465737455504e
            Field "type_id"
              asn1crypto.core.ObjectIdentifier Object #140419936362896
                Header: 0x060a
                  primitive universal tag 6
                Data: 0x2b060104018237140203
                  Native: 1.3.6.1.4.1.311.20.2.3
            Field "value"
              asn1crypto.core.UTF8String Object #140419936363184
                Header: 0xa0090c07
                  context tag 0 (explicitly tagged)
                    primitive universal 12
                Data: 0x7465737455504e
                  Native: testUPN
b'MBmgFwYKKwYBBAGCNxQCA6AJDAd0ZXN0VVBO'

With both the original and changed version of the library, the reverse parsing breaks:

ind = 'MBmgFwYKKwYBBAGCNxQCA6AJDAd0ZXN0VVBO'
bindata = base64.b64decode(ind)
asn1_obj = asn1crypto.core.load(bindata)
asn1_obj.native
Traceback (most recent call last):
  File "debugasn1.py", line 18, in <module>
    asn1_obj.native
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 4044, in native
    self._parse_children(recurse=True)
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 3988, in _parse_children
    raise e
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 3960, in _parse_children
    child = _build(*child)
  File "/home/user/.local/share/virtualenvs/test/lib/python3.8/site-packages/asn1crypto/core.py", line 5607, in _build
    raise ValueError(unwrap(
ValueError: Unknown element - context class, constructed method, tag 0
    while parsing asn1crypto.core.Sequence

Thanks for your work on this, hope this helps track down the issue!

MatthiasValvekens commented 2 years ago

Interesting... That does feel like a bug (or just a limitation in how asn1crypto deals with Any), but there might be a relatively straightforward workaround/fix here. I'm pretty sure that this will work as intended once you explicitise the DEFINED BY relationship in much the same way as for ContentInfo: https://github.com/wbond/asn1crypto/blob/8a4c621e34f8cbdc83b047aa617e66dcbfde0f75/asn1crypto/cms.py#L527-L528.

In other words

class AnotherName(Sequence):
    _fields = [
        ('type_id', ObjectIdentifier),
        ('value', Any, {'explicit': 0}),
    ]

    _oid_pair = ('type_id', 'value')
    _oid_specs = {}

And you can then even add some mappings to _oid_specs to automagically handle the case you want, for example:

AnotherName._oid_specs['1.3.6.1.4.1.311.20.2.3'] = UTF8String

I unfortunately don't have time to test this right now, but it's worth a try. If it works, it's definitely worth doing a PR IMO. In the meantime, you can (reasonably safely) monkeypatch this kind of thing into asn1crypto at runtime... ;)

dirkjanm commented 2 years ago

hey Matthias, thanks, this does work pretty well for my problem, your proposed additions work and it can construct the asn1 properly. However it does not fix the reverse parsing from asn1 back to python. Not sure if that's a separate bug or has the same root cause.

wbond commented 2 years ago

@dirkjanm Looking at the ASN.1 at https://datatracker.ietf.org/doc/html/rfc5280#page-128, it does say that the ANY tag is defined by the type-id. It appears the definition of AnotherName is currently missing:

    _oid_pair = ('type_id', 'value')
    _oid_specs = {}

as suggested by @MatthiasValvekens.

My guess is that I hadn't run into such a tag yet, so it hasn't been implemented.

That said, you did seem to identify a bug in the parsing.