Bug #4030: ext/openssl OpenSSL::ASN1::decode / to_der

• Status: Closed
• Assignee: tenderlovemaking (Aaron Patterson)
• Target version: -
• ruby -v: 1.9.2-p0

[MartinBosslet (Martin Bosslet)]

=begin
Hi all,

I have a PKCS#7 SignedData whose EncapContentInfo's content is BER-encoded using indefinite length. If I decode it and then encode it again, e.g. by

der_string = OpenSSL::ASN1::decode(File.read("signature_file")).to_der
File.open("out_Again", "w") do |out|
out.print(der_string)
end

then, the resulting file will no longer use the previous encoding, but actually DER-encode the content using definite length.

I think I spotted the reason for this in ext/openssl/ossl_asn1.c:

if(j & V_ASN1_CONSTRUCTED){
/* TODO: if j == 0x21 it is indefinite length object. */
if((j == 0x21) && (len == 0)){
long lastoff = off;
value = ossl_asn1_decode0(&p, length, &off, depth+1, 0, yield);
len = off - lastoff;
}
else value = ossl_asn1_decode0(&p, len, &off, depth+1, 0, yield);
}

Could the encoding be preserved? A simple way would be to cache the initial encoding and the information that infinite length tags were used?

I'd love to write a library for digital signatures in Ruby that supports CAdES (RFC5126) signatures. For these signatures, elements of the SignedData are hashed and on the basis of these hashes, timestamps are generated. For recalculating these hashes it's very important that the encoding is exactly the same as the initial one. Unfortunately, with the current implementation, this would only be possible if the initial signature is already DER-encoded, but it would fail for BER-encoded signatures.

Best regards,
Martin Boßlet
=end

[naruse (Yui NARUSE)]

• Status changed from Open to Assigned
• Assignee set to nahi (Hiroshi Nakamura)

=begin

=end

[MartinBosslet (Martin Bosslet)]

• File asn1_inf_length.diff asn1_inf_length.diff added

=begin
I'd like to propose a patch that would resolve the issue for me.

After analyzing the signatures in more detail I found out that the only BER encodings used were infinite length constructed encodings e.g. for sequence, set and octet string (or bit string) structures. This makes sense in particular for CMS SignedData structures when "attached signatures" are used - encoding the EncapContentInfo using infinite length constructs is the only way to keep streaming support possible.
But the rest of the primitive encodings used DER encoding. Essentially this also means that there is not really a need to cache the entire encoding, it suffices to store the information whether a constructed encoding was encoded with infinite length or not.
To support this, I added "@infinite length" to each ASN1Data (with getter/setter) that is used to determine the encoding rule to be applied when serializing the object. This is only effective for sub-classes of ASN1Constructive, which is why #infinite_length= is explicitly undefined for all ASN1Primitives.
Infinite length octet strings and bit strings are represented as ASN1Constructives whose value is an array of octet strings or bit strings respectively, where these again can be constructed, in ASN1Constructive form, or ,ultimately, primitive.
To construct an infinite length encoding from scratch, an instance of the new class OpenSSL::ASN1::EndOfContent must be added at the end of the respective value array.
I also added a couple of tests that assert the expected behavior and also demonstrate the usage of infinite_length.

Best regards,
Martin Boßlet
=end

[naruse (Yui NARUSE)]

=begin
I think, this proposal is reasonable, but the code should be reviewed.
So anyone can review this?
=end

[MartinBosslet (Martin Bosslet)]

=begin
Ok, thanks for that!
=end

[tenderlovemaking (Aaron Patterson)]

=begin
I've reviewed this patch. It seems fine except for one thing, OpenSSL::ASN1::EndOfContent#initialize is defined with -1, but then ignores the argv and argc. Can we just make the initialize 0 argument?

I haven't tried applying and running the tests yet. But if the change I suggested were made, and the tests all pass, I think this patch would be good.
=end

[tenderlovemaking (Aaron Patterson)]

• Assignee changed from nahi (Hiroshi Nakamura) to tenderlovemaking (Aaron Patterson)

=begin

=end

[MartinBosslet (Martin Bosslet)]

• File asn1_inf_length_2.diff asn1_inf_length_2.diff added

=begin
I changed the code to feature the 0 argument initialize!
=end

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

=begin
This issue was solved with changeset r30178.
Martin, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end