Project

General

Profile

Actions

Bug #4374

closed

[ext/openssl] ASN1.decode wrong for infinite length values

Added by MartinBosslet (Martin Bosslet) about 13 years ago. Updated almost 13 years ago.

Status:
Closed
Target version:
ruby -v:
ruby 1.9.2p136 (2010-12-25 revision 30365) [i686-linux]
Backport:
[ruby-core:35123]

Description

=begin
Hi all,

ASN.1 decoding behaves incorrectly for DER encodings with infinite length values. Two examples:

require 'openssl'
require 'pp'

eoc = OpenSSL::ASN1::EndOfContent.new
int = OpenSSL::ASN1::Integer.new (1)

inner = OpenSSL::ASN1::Sequence.new([int, eoc])
inner.infinite_length = true

outer = OpenSSL::ASN1::Sequence.new([inner, eoc])
outer.infinite_length = true

asn1 = OpenSSL::ASN1.decode(outer.to_der)

pp asn1

=> #<OpenSSL::ASN1::Sequence:0x9b4bd70
@infinite_length=true,
@tag=16,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=
[#<OpenSSL::ASN1::Sequence:0x9b4bd84
@infinite_length=true,
@tag=16,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=
[#<OpenSSL::ASN1::Integer:0x9b4be24
@infinite_length=false,
@tag=2,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=1>,
#<OpenSSL::ASN1::EndOfContent:0x9b4bde8
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">,
#<OpenSSL::ASN1::EndOfContent:0x9b4bdac
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">]>]>

The end of content DER for the outer Sequence is incorrectly stored with the values
of the inner sequence. Although after encoding the resulting DER will be correct, the
structure should rather look like this:

#<OpenSSL::ASN1::Sequence:0x9f58ee0
@infinite_length=true,
@tag=16,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=
[#<OpenSSL::ASN1::Sequence:0x9f58f30
@infinite_length=true,
@tag=16,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=
[#<OpenSSL::ASN1::Integer:0x9f58f94
@infinite_length=false,
@tag=2,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=1>,
#<OpenSSL::ASN1::EndOfContent:0x9f58f6c
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">]>,
#<OpenSSL::ASN1::EndOfContent:0x9f58f08
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">]>

Another example:

require 'openssl'
require 'pp'

eoc = OpenSSL::ASN1::EndOfContent.new
oct = OpenSSL::ASN1::OctetString.new ("\x01")

inner = OpenSSL::ASN1::Constructive.new([oct, eoc], OpenSSL::ASN1::OCTET_STRING)
inner.infinite_length = true

outer = OpenSSL::ASN1::Constructive.new([inner, eoc], OpenSSL::ASN1::OCTET_STRING)
outer.infinite_length = true

asn1 = OpenSSL::ASN1.decode(outer.to_der)

pp asn1

=> <OpenSSL::ASN1::ASN1Data:0xa0fcdf0
@infinite_length=true,
@tag=4,
@tag_class=:CONTEXT_SPECIFIC,
@value=
[#<OpenSSL::ASN1::Constructive:0xa0fce04
@infinite_length=true,
@tag=4,
@tag_class=:UNIVERSAL,
@tagging=:EXPLICIT,
@value=
[#<OpenSSL::ASN1::ASN1Data:0xa0fce2c
@infinite_length=true,
@tag=4,
@tag_class=:CONTEXT_SPECIFIC,
@value=
[#<OpenSSL::ASN1::Constructive:0xa0fce40
@infinite_length=true,
@tag=4,
@tag_class=:UNIVERSAL,
@tagging=:EXPLICIT,
@value=
[#<OpenSSL::ASN1::OctetString:0xa0fcee0
@infinite_length=false,
@tag=4,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="\x01">,
#<OpenSSL::ASN1::EndOfContent:0xa0fceb8
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">,
#<OpenSSL::ASN1::EndOfContent:0xa0fce68
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">]>]>]>]>

Here it's worse, because when calling asn1.to_der it will even result in an error:

test.rb:17:in to_der': invalid constructed encoding (OpenSSL::ASN1::ASN1Error) from test.rb:17:in each'
from test.rb:17:in to_der' from test.rb:17:in '

The problem are the defaults for tagging and tag_class in ossl_asn1_initialize that are not
intuitive and are defaults for tagged DER values instead of "normal" values.

The correct structure for the above would look like this:

#<OpenSSL::ASN1::Constructive:0x93ed128
@infinite_length=true,
@tag=4,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=
[#<OpenSSL::ASN1::Constructive:0x93ed178
@infinite_length=true,
@tag=4,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value=
[#<OpenSSL::ASN1::OctetString:0x93ed1c8
@infinite_length=false,
@tag=4,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="\x01">,
#<OpenSSL::ASN1::EndOfContent:0x93ed1a0
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">]>,
#<OpenSSL::ASN1::EndOfContent:0x93ed150
@infinite_length=false,
@tag=0,
@tag_class=:UNIVERSAL,
@tagging=nil,
@value="">]>

The attached patch fixes the problems and has also "more natural" defaults for
ossl_asn1_initialize.

Regards,
Martin
=end


Files

fix_asn1.diff (3.62 KB) fix_asn1.diff MartinBosslet (Martin Bosslet), 02/07/2011 02:52 AM
Actions #1

Updated by MartinBosslet (Martin Bosslet) about 13 years ago

=begin
I verified that with the current code it's not possible to construct a Constructive
instance without tagging if the tag_class is passed as an explicit parameter.
Tagging will be set to :EXPLICIT if it was passed as nil. That's why

if (infinite && !(tag == V_ASN1_SEQUENCE || tag == V_ASN1_SET)){
asn1data = rb_funcall(cASN1Constructive,
rb_intern("new"),
4,
value,
INT2NUM(tag),
Qnil,
ID2SYM(tag_class));

in ossl_asn1_decode0 will always produce a Constructive with :EXPLICIT
tagging, and reencoding a parsed ASN.1 value will fail or produce incorrect
output.
At first I thought changing the defaults in ossl_asn1_initialize would be
more intuitive but not essential to the fix - but now I think it has become
mandatory.
=end

Updated by MartinBosslet (Martin Bosslet) almost 13 years ago

  • Assignee set to MartinBosslet (Martin Bosslet)
Actions #3

Updated by Anonymous almost 13 years ago

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

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


  • ext/openssl/ossl_asn1.c: Fix decoding of infinite length values.
    Simplified ossl_asn1_decode0 by splitting it into three separate
    functions. Add tests.
    [Ruby 1.9 - Bug #4374][ruby-core:35123]

Updated by MartinBosslet (Martin Bosslet) almost 13 years ago

Hi,

I fixed this bug and tried to simplify ossl_asn1_decode0 and
improve its performance.

Here is what I did:

  • removed the "once" parameter

  • added sanity checks that verify that all bytes are actually
    read when parsing is finished

  • tried to reduce complexity by splitting ossl_asn1_decode0
    into three separate methods to make it easier to spot the code that
    handles constructed values and the code that handles primitive
    values

  • ossl_asn1_decode now returns an Array just in the case of
    parsing constructed values, otherwise it returns the plain value
    to reduce overall allocated objects

  • changed the behavior of ossl_asn1_initialize slightly to allow the
    construction of infinite length primitive values in the form of a
    Constructive

  • replaced rb_intern with static symbols to improve performance

  • allocate and initialize ASN1Data (and sub-classes) instances
    in C to improve performance and save additional VM roundtrips

  • initialize those instances additionally with tag and tag_class
    to avoid hash look-ups each time in ossl_asn1_initialize

  • added some tests for edge cases (I'll add more soon)

The performance gain is noticeable - in my tests (certificates,
CMS signatures) the current implementation was roughly twice as
fast as that of 1.9.2p180.

I would be happy about comments and improvements.

Regards,
Martin

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0