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 the 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. But, the end-of-contents are not
really part of the content octets, and this is annoying.

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 Jan 20, 2017
1 parent 56894bd commit a5317e8
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 26 deletions.
16 changes: 12 additions & 4 deletions ext/openssl/ossl_asn1.c
View file
Expand Up @@ -725,6 +725,12 @@ ossl_asn1cons_encode_value(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 Expand Up @@ -758,7 +764,7 @@ ossl_asn1data_encode_value(VALUE self)
static VALUE
to_der_internal(VALUE self, int tag_class, int tag_number)
{
int length, constructed;
int length, constructed, indef_len;
VALUE encoded, value, str;
unsigned char *p;

Expand All @@ -768,7 +774,8 @@ to_der_internal(VALUE self, int tag_class, int tag_number)
value = rb_ary_entry(encoded, 1);
StringValue(value);

if (RTEST(ossl_asn1_get_indefinite_length(self))) {
indef_len = RTEST(ossl_asn1_get_indefinite_length(self));
if (indef_len) {
if (constructed == 0)
ossl_raise(eASN1Error, "indefinite form used for primitive encoding");
constructed = 2;
Expand All @@ -780,8 +787,9 @@ to_der_internal(VALUE self, int tag_class, int tag_number)
ASN1_put_object(&p, constructed, RSTRING_LENINT(value), tag_number, tag_class);
memcpy(p, RSTRING_PTR(value), RSTRING_LEN(value));
p += RSTRING_LEN(value);
if (indef_len)
ASN1_put_eoc(&p);
ossl_str_adjust(str, p);
/* TODO: put EOC automatically even if it does not appear in value */

return str;
}
Expand Down Expand Up @@ -914,13 +922,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
44 changes: 22 additions & 22 deletions test/test_asn1.rb
View file
Expand Up @@ -319,10 +319,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 @@ -334,6 +332,14 @@ def test_sequence
])
obj.indefinite_length = true
assert_unencodable obj

# 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 @@ -343,10 +349,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 @@ -406,12 +409,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_unencodable obj
Expand All @@ -435,12 +441,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 @@ -535,32 +541,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 a5317e8

Please sign in to comment.