Skip to content

Commit

Permalink
asn1: do not treat EOC octets as part of content octets
Browse files Browse the repository at this point in the history 
We currently treat end-of-contents octets as a BER encoding of a value
whose tag is universal class and the number is zero, and require users
to put one in the end of 'value' array when encoding using indefinite
length form. However, the end-of-contents are just a marker indicating
the end of the contents and not really part of the contents.

Do not require users to put an EOC object in the content when encoding,
and don't produce an EOC object when decoding an encoding that uses
indefinite length form.
  • Loading branch information
@rhenium
rhenium committed Jul 2, 2017
1 parent 107367b commit 23887b7
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 25 deletions.
16 changes: 13 additions & 3 deletions ext/openssl/ossl_asn1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,8 +683,10 @@ to_der_internal(VALUE self, int constructed, int indef_len, VALUE body)
ASN1_put_object(&p, encoding, body_length, default_tag_number, V_ASN1_UNIVERSAL);
memcpy(p, RSTRING_PTR(body), body_length);
p += body_length;
if (indef_len)
if (indef_len) {
ASN1_put_eoc(&p); /* For inner object */
ASN1_put_eoc(&p); /* For wrapper object */
}
}
else {
total_length = ASN1_object_size(encoding, body_length, tag_number);
Expand All @@ -693,8 +695,10 @@ to_der_internal(VALUE self, int constructed, int indef_len, VALUE body)
ASN1_put_object(&p, encoding, body_length, tag_number, tag_class);
memcpy(p, RSTRING_PTR(body), body_length);
p += body_length;
if (indef_len)
ASN1_put_eoc(&p);
}
ossl_str_adjust(str, p);
assert(p - (unsigned char *)RSTRING_PTR(str) == total_length);
return str;
}

Expand Down Expand Up @@ -816,13 +820,13 @@ int_ossl_asn1_decode0_cons(unsigned char **pp, long max_len, long length,
value = ossl_asn1_decode0(pp, available_len, &off, depth + 1, yield, &inner_read);
*num_read += inner_read;
available_len -= inner_read;
rb_ary_push(ary, value);

if (indefinite &&
ossl_asn1_tag(value) == V_ASN1_EOC &&
ossl_asn1_get_tag_class(value) == sym_UNIVERSAL) {
break;
}
rb_ary_push(ary, value);
}

if (tc == sym_UNIVERSAL) {
Expand Down Expand Up @@ -1181,6 +1185,12 @@ ossl_asn1cons_to_der(VALUE self)
if (indef_len && rb_obj_is_kind_of(item, cASN1EndOfContent)) {
if (i != RARRAY_LEN(ary) - 1)
ossl_raise(eASN1Error, "illegal EOC octets in value");

/*
* EOC is not really part of the content, but we required to add one
* at the end in the past.
*/
break;
}

item = ossl_to_der_if_possible(item);
Expand Down
44 changes: 22 additions & 22 deletions test/test_asn1.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,10 +339,8 @@ def test_sequence
OpenSSL::ASN1::Sequence.new([]),
OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))
])
expected = OpenSSL::ASN1::Sequence.new([
OpenSSL::ASN1::OctetString.new(B(%w{ 00 })),
OpenSSL::ASN1::EndOfContent.new
])

expected = OpenSSL::ASN1::Sequence.new([OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))])
expected.indefinite_length = true
encode_decode_test B(%w{ 30 80 04 01 00 00 00 }), expected

Expand All @@ -354,6 +352,14 @@ def test_sequence
])
obj.indefinite_length = true
assert_raise(OpenSSL::ASN1::ASN1Error) { obj.to_der }

# The last EOC in value is ignored if indefinite length form is used
expected = OpenSSL::ASN1::Sequence.new([
OpenSSL::ASN1::OctetString.new(B(%w{ 00 })),
OpenSSL::ASN1::EndOfContent.new
])
expected.indefinite_length = true
encode_test B(%w{ 30 80 04 01 00 00 00 }), expected
end

def test_set
Expand All @@ -363,10 +369,7 @@ def test_set
OpenSSL::ASN1::Sequence.new([]),
OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))
])
expected = OpenSSL::ASN1::Set.new([
OpenSSL::ASN1::OctetString.new(B(%w{ 00 })),
OpenSSL::ASN1::EndOfContent.new
])
expected = OpenSSL::ASN1::Set.new([OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))])
expected.indefinite_length = true
encode_decode_test B(%w{ 31 80 04 01 00 00 00 }), expected
end
Expand Down Expand Up @@ -431,12 +434,15 @@ def test_basic_asn1data
encode_decode_test B(%w{ 41 81 80 } + %w{ AB CD } * 64), OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD } * 64), 1, :APPLICATION)
encode_decode_test B(%w{ 41 82 01 00 } + %w{ AB CD } * 128), OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD } * 128), 1, :APPLICATION)
encode_decode_test B(%w{ 61 00 }), OpenSSL::ASN1::ASN1Data.new([], 1, :APPLICATION)
obj = OpenSSL::ASN1::ASN1Data.new([OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD }), 2, :PRIVATE)], 1, :APPLICATION)
obj.indefinite_length = true
encode_decode_test B(%w{ 61 80 C2 02 AB CD 00 00 }), obj
obj = OpenSSL::ASN1::ASN1Data.new([
OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD }), 2, :PRIVATE),
OpenSSL::ASN1::EndOfContent.new
], 1, :APPLICATION)
obj.indefinite_length = true
encode_decode_test B(%w{ 61 80 C2 02 AB CD 00 00 }), obj
encode_test B(%w{ 61 80 C2 02 AB CD 00 00 }), obj
obj = OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD }), 1, :UNIVERSAL)
obj.indefinite_length = true
assert_raise(OpenSSL::ASN1::ASN1Error) { obj.to_der }
Expand All @@ -460,12 +466,12 @@ def test_basic_constructed
encode_test B(%w{ 21 00 }), OpenSSL::ASN1::Constructive.new([], 1, nil, :UNIVERSAL)
encode_test B(%w{ A1 00 }), OpenSSL::ASN1::Constructive.new([], 1, nil, :CONTEXT_SPECIFIC)
encode_test B(%w{ 21 04 04 02 AB CD }), OpenSSL::ASN1::Constructive.new([octet_string], 1)
obj = OpenSSL::ASN1::Constructive.new([
octet_string,
OpenSSL::ASN1::EndOfContent.new
], 1)
obj = OpenSSL::ASN1::Constructive.new([octet_string], 1)
obj.indefinite_length = true
encode_decode_test B(%w{ 21 80 04 02 AB CD 00 00 }), obj
obj = OpenSSL::ASN1::Constructive.new([octet_string, OpenSSL::ASN1::EndOfContent.new], 1)
obj.indefinite_length = true
encode_test B(%w{ 21 80 04 02 AB CD 00 00 }), obj
end

def test_prim_explicit_tagging
Expand Down Expand Up @@ -570,32 +576,26 @@ def test_recursive_octet_string_parse
assert_equal(OpenSSL::ASN1::Constructive, asn1.class)
assert_universal(OpenSSL::ASN1::OCTET_STRING, asn1)
assert_equal(true, asn1.indefinite_length)
assert_equal(4, asn1.value.size)
assert_equal(3, asn1.value.size)
nested1 = asn1.value[0]
assert_equal(OpenSSL::ASN1::Constructive, nested1.class)
assert_universal(OpenSSL::ASN1::OCTET_STRING, nested1)
assert_equal(true, nested1.indefinite_length)
assert_equal(2, nested1.value.size)
assert_equal(1, nested1.value.size)
oct1 = nested1.value[0]
assert_universal(OpenSSL::ASN1::OCTET_STRING, oct1)
assert_equal(false, oct1.indefinite_length)
assert_universal(OpenSSL::ASN1::EOC, nested1.value[1])
assert_equal(false, nested1.value[1].indefinite_length)
nested2 = asn1.value[1]
assert_equal(OpenSSL::ASN1::Constructive, nested2.class)
assert_universal(OpenSSL::ASN1::OCTET_STRING, nested2)
assert_equal(true, nested2.indefinite_length)
assert_equal(2, nested2.value.size)
assert_equal(1, nested2.value.size)
oct2 = nested2.value[0]
assert_universal(OpenSSL::ASN1::OCTET_STRING, oct2)
assert_equal(false, oct2.indefinite_length)
assert_universal(OpenSSL::ASN1::EOC, nested2.value[1])
assert_equal(false, nested2.value[1].indefinite_length)
oct3 = asn1.value[2]
assert_universal(OpenSSL::ASN1::OCTET_STRING, oct3)
assert_equal(false, oct3.indefinite_length)
assert_universal(OpenSSL::ASN1::EOC, asn1.value[3])
assert_equal(false, asn1.value[3].indefinite_length)
end

def test_decode_constructed_overread
Expand Down

0 comments on commit 23887b7

Please sign in to comment.