Asn1Value._new_instance doesn't copy the value of member method

[smlu]

When copying Asn1Value object the value of Asn1Value.method is not copied over to the new instance in Asn1Value._new_instance method.

asn1crypto/asn1crypto/core.py

Lines 484 to 497 in 5a24aed

	def _new_instance(self):
	"""
	Constructs a new copy of the current object, preserving any tagging
	:return:
	An Asn1Value object
	"""
	new_obj = self.__class__()
	new_obj.class_ = self.class_
	new_obj.tag = self.tag
	new_obj.implicit = self.implicit
	new_obj.explicit = self.explicit
	return new_obj

[joernheissler]

Hi, got a small example to demonstrate why this is actually bad? This could then serve as a test case to prevent regressions.

[MatthiasValvekens]

While I'm not the OP, I'd wager that this is never a problem when dealing with DER, where all (universal) types have a fixed encoding method. However, in BER, most string-like types can be either primitive or constructed. More to the point, CER even requires strings to be encoded as constructed (with primitive subsequences) in some cases:

Bitstring, octetstring, and restricted character string values shall be encoded with a primitive encoding if they would
require no more than 1000 contents octets, and as a constructed encoding otherwise. The string fragments contained in
the constructed encoding shall be encoded with a primitive encoding. The encoding of each fragment, except possibly
the last, shall have 1000 contents octets. (Contrast with 8.23.6.) The last fragment shall have at least one, and no more
than 1000, contents octets.

(Source ITU-T Rec. X.690 | ISO/IEC 8825-1:2015 clause 9.2)

Long story short: I'm curious how asn1crypto would behave when cloning a constructed string (see e.g. the example at the bottom of § 8.23 of X.690). It looks like not preserving the method properly could potentially mangle such strings.

Unfortunately I don't have time to test the behaviour myself right now, but I can take a look later if no-one else gets to it by then.

[smlu]

Hi, got a small example to demonstrate why this is actually bad? This could then serve as a test case to prevent regressions.

While @MatthiasValvekens has great point where this could potential be a real problem, I unfortunately only have python semantic case i.e. changing the value of method for an object is not copied over to the new object. I encountered this while doing some custom testing.

In theory where this could lead to an issue, for example is when digital signature was made over different encoding than parsed object (e.g. signedAttrs of SignerInfo in CMS, though not the case here) and the object was copied after the method value for the object was changed. Though this is bad coding and in most cases copying should be done prior modification, not copying the method value to new instance can still lead to some unexpected bugs.

class TestValue(Asn1Value):
        class_ = 1
        method = 1
        tag = 23

    t = TestValue() 
    t.method = 0
    t.tag = 22
    t_cpy = t.copy()

    assert t_cpy.tag == 22
    assert t_cpy.method == 0 # <-- should fail

[MatthiasValvekens]

I'm not disagreeing with you, but I have one small nitpick:

In theory where this could lead to an issue, for example is when digital signature was made over different encoding than parsed object (e.g. signedAttrs of SignerInfo in CMS, though not the case here)

While that could happen, a strict reading of the CMS spec will tell you that by definition, the signature has to be computed over the DER encoding of the signed attributes, thus ruling out this particular class of bugs since it'd render all signatures that would be affected by an issue like this invalid out of the gate :).

That said, in practice many validators tolerate non-DER encodings and compute their digests over whatever happens to be within the signedAttrs payload, regardless of what the spec says.

---

In the meantime, I tried running this:

>>> import binascii
>>> x=binascii.unhexlify('3a0904034a6f6e04026573')
>>> from asn1crypto import core
>>> core.VisibleString.load(x)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.10/site-packages/asn1crypto/core.py", line 230, in load
    value, _ = _parse_build(encoded_data, spec=spec, spec_params=kwargs, strict=strict)
  File "/usr/lib/python3.10/site-packages/asn1crypto/core.py", line 5676, in _parse_build
    return (_build(*info, spec=spec, spec_params=spec_params), new_pointer)
  File "/usr/lib/python3.10/site-packages/asn1crypto/core.py", line 5555, in _build
    raise ValueError(unwrap(
ValueError: Error parsing asn1crypto.core.VisibleString - method should have been primitive, but constructed was found

So I guess my example doesn't apply, since apparently asn1crypto doesn't accept constructed strings regardless. I don't know off the top of my head if the internal logic assumes that method is a class-level constant, but if so, there's probably not much point in trying to fix the copying behaviour before addressing that issue first, since clearly the rabbit hole goes a lot deeper... :)

[smlu]

While that could happen, a strict reading of the CMS spec will tell you that by definition, the signature has to be computed over the DER encoding of the signed attributes, thus ruling out this particular class of bugs since it'd render all signatures that would be affected by an issue like this invalid out of the gate :).

In the past I had cases with some biometric passports where EF.SOD file (SignedData wrapped in ContentInfo) had signed attributes encoded implicitly. The asn1crypto doesn't auto change the encoding and I had to manually change the tag & class to asn1.SET in order to get valid DER encoding for signature hash.

[wbond]

At this point between the asn1crypto and other modularcrypto packages, there is a decent corpus of tests. While I likely never ran into a use case for customizing the method per instance, I am fine with adding some tests for this use case and making sure nothing existing breaks.