Issue #4374 has been updated by Martin Bosslet.


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

----------------------------------------
Bug #4374: [ext/openssl] ASN1.decode wrong for infinite length values
http://redmine.ruby-lang.org/issues/4374

Author: Martin Bosslet
Status: Closed
Priority: Normal
Assignee: Martin Bosslet
Category: ext
Target version: 1.9.3
ruby -v: ruby 1.9.2p136 (2010-12-25 revision 30365) [i686-linux]


=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 `<main>'
 
 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



-- 
http://redmine.ruby-lang.org