Skip to content

Commit 6b67c6d

Browse files
rheniumrhenium
committed
asn1: do not treat EOC octets as part of content octets
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.
1 parent 1d202b0 commit 6b67c6d

File tree

2 files changed

+35
-25
lines changed

2 files changed

+35
-25
lines changed

‎ext/openssl/ossl_asn1.c‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -682,8 +682,10 @@ to_der_internal(VALUE self, int constructed, int indef_len, VALUE body)
682682
ASN1_put_object(&p, encoding, body_length, default_tag_number, V_ASN1_UNIVERSAL);
683683
memcpy(p, RSTRING_PTR(body), body_length);
684684
p += body_length;
685-
if (indef_len)
685+
if (indef_len) {
686+
ASN1_put_eoc(&p); /* For inner object */
686687
ASN1_put_eoc(&p); /* For wrapper object */
688+
}
687689
}
688690
else {
689691
total_length = ASN1_object_size(encoding, body_length, tag_number);
@@ -692,8 +694,10 @@ to_der_internal(VALUE self, int constructed, int indef_len, VALUE body)
692694
ASN1_put_object(&p, encoding, body_length, tag_number, tag_class);
693695
memcpy(p, RSTRING_PTR(body), body_length);
694696
p += body_length;
697+
if (indef_len)
698+
ASN1_put_eoc(&p);
695699
}
696-
ossl_str_adjust(str, p);
700+
assert(p - (unsigned char *)RSTRING_PTR(str) == total_length);
697701
return str;
698702
}
699703

@@ -815,13 +819,13 @@ int_ossl_asn1_decode0_cons(unsigned char **pp, long max_len, long length,
815819
value = ossl_asn1_decode0(pp, available_len, &off, depth + 1, yield, &inner_read);
816820
*num_read += inner_read;
817821
available_len -= inner_read;
818-
rb_ary_push(ary, value);
819822

820823
if (indefinite &&
821824
ossl_asn1_tag(value) == V_ASN1_EOC &&
822825
ossl_asn1_get_tag_class(value) == sym_UNIVERSAL) {
823826
break;
824827
}
828+
rb_ary_push(ary, value);
825829
}
826830

827831
if (tc == sym_UNIVERSAL) {
@@ -1180,6 +1184,12 @@ ossl_asn1cons_to_der(VALUE self)
11801184
if (indef_len && rb_obj_is_kind_of(item, cASN1EndOfContent)) {
11811185
if (i != RARRAY_LEN(ary) - 1)
11821186
ossl_raise(eASN1Error, "illegal EOC octets in value");
1187+
1188+
/*
1189+
* EOC is not really part of the content, but we required to add one
1190+
* at the end in the past.
1191+
*/
1192+
break;
11831193
}
11841194

11851195
item = ossl_to_der_if_possible(item);

‎test/test_asn1.rb‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -339,10 +339,8 @@ def test_sequence
339339
OpenSSL::ASN1::Sequence.new([]),
340340
OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))
341341
])
342-
expected = OpenSSL::ASN1::Sequence.new([
343-
OpenSSL::ASN1::OctetString.new(B(%w{ 00 })),
344-
OpenSSL::ASN1::EndOfContent.new
345-
])
342+
343+
expected = OpenSSL::ASN1::Sequence.new([OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))])
346344
expected.indefinite_length = true
347345
encode_decode_test B(%w{ 30 80 04 01 00 00 00 }), expected
348346

@@ -354,6 +352,14 @@ def test_sequence
354352
])
355353
obj.indefinite_length = true
356354
assert_raise(OpenSSL::ASN1::ASN1Error) { obj.to_der }
355+
356+
# The last EOC in value is ignored if indefinite length form is used
357+
expected = OpenSSL::ASN1::Sequence.new([
358+
OpenSSL::ASN1::OctetString.new(B(%w{ 00 })),
359+
OpenSSL::ASN1::EndOfContent.new
360+
])
361+
expected.indefinite_length = true
362+
encode_test B(%w{ 30 80 04 01 00 00 00 }), expected
357363
end
358364

359365
def test_set
@@ -363,10 +369,7 @@ def test_set
363369
OpenSSL::ASN1::Sequence.new([]),
364370
OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))
365371
])
366-
expected = OpenSSL::ASN1::Set.new([
367-
OpenSSL::ASN1::OctetString.new(B(%w{ 00 })),
368-
OpenSSL::ASN1::EndOfContent.new
369-
])
372+
expected = OpenSSL::ASN1::Set.new([OpenSSL::ASN1::OctetString.new(B(%w{ 00 }))])
370373
expected.indefinite_length = true
371374
encode_decode_test B(%w{ 31 80 04 01 00 00 00 }), expected
372375
end
@@ -431,12 +434,15 @@ def test_basic_asn1data
431434
encode_decode_test B(%w{ 41 81 80 } + %w{ AB CD } * 64), OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD } * 64), 1, :APPLICATION)
432435
encode_decode_test B(%w{ 41 82 01 00 } + %w{ AB CD } * 128), OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD } * 128), 1, :APPLICATION)
433436
encode_decode_test B(%w{ 61 00 }), OpenSSL::ASN1::ASN1Data.new([], 1, :APPLICATION)
437+
obj = OpenSSL::ASN1::ASN1Data.new([OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD }), 2, :PRIVATE)], 1, :APPLICATION)
438+
obj.indefinite_length = true
439+
encode_decode_test B(%w{ 61 80 C2 02 AB CD 00 00 }), obj
434440
obj = OpenSSL::ASN1::ASN1Data.new([
435441
OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD }), 2, :PRIVATE),
436442
OpenSSL::ASN1::EndOfContent.new
437443
], 1, :APPLICATION)
438444
obj.indefinite_length = true
439-
encode_decode_test B(%w{ 61 80 C2 02 AB CD 00 00 }), obj
445+
encode_test B(%w{ 61 80 C2 02 AB CD 00 00 }), obj
440446
obj = OpenSSL::ASN1::ASN1Data.new(B(%w{ AB CD }), 1, :UNIVERSAL)
441447
obj.indefinite_length = true
442448
assert_raise(OpenSSL::ASN1::ASN1Error) { obj.to_der }
@@ -460,12 +466,12 @@ def test_basic_constructed
460466
encode_test B(%w{ 21 00 }), OpenSSL::ASN1::Constructive.new([], 1, nil, :UNIVERSAL)
461467
encode_test B(%w{ A1 00 }), OpenSSL::ASN1::Constructive.new([], 1, nil, :CONTEXT_SPECIFIC)
462468
encode_test B(%w{ 21 04 04 02 AB CD }), OpenSSL::ASN1::Constructive.new([octet_string], 1)
463-
obj = OpenSSL::ASN1::Constructive.new([
464-
octet_string,
465-
OpenSSL::ASN1::EndOfContent.new
466-
], 1)
469+
obj = OpenSSL::ASN1::Constructive.new([octet_string], 1)
467470
obj.indefinite_length = true
468471
encode_decode_test B(%w{ 21 80 04 02 AB CD 00 00 }), obj
472+
obj = OpenSSL::ASN1::Constructive.new([octet_string, OpenSSL::ASN1::EndOfContent.new], 1)
473+
obj.indefinite_length = true
474+
encode_test B(%w{ 21 80 04 02 AB CD 00 00 }), obj
469475
end
470476

471477
def test_prim_explicit_tagging
@@ -570,32 +576,26 @@ def test_recursive_octet_string_parse
570576
assert_equal(OpenSSL::ASN1::Constructive, asn1.class)
571577
assert_universal(OpenSSL::ASN1::OCTET_STRING, asn1)
572578
assert_equal(true, asn1.indefinite_length)
573-
assert_equal(4, asn1.value.size)
579+
assert_equal(3, asn1.value.size)
574580
nested1 = asn1.value[0]
575581
assert_equal(OpenSSL::ASN1::Constructive, nested1.class)
576582
assert_universal(OpenSSL::ASN1::OCTET_STRING, nested1)
577583
assert_equal(true, nested1.indefinite_length)
578-
assert_equal(2, nested1.value.size)
584+
assert_equal(1, nested1.value.size)
579585
oct1 = nested1.value[0]
580586
assert_universal(OpenSSL::ASN1::OCTET_STRING, oct1)
581587
assert_equal(false, oct1.indefinite_length)
582-
assert_universal(OpenSSL::ASN1::EOC, nested1.value[1])
583-
assert_equal(false, nested1.value[1].indefinite_length)
584588
nested2 = asn1.value[1]
585589
assert_equal(OpenSSL::ASN1::Constructive, nested2.class)
586590
assert_universal(OpenSSL::ASN1::OCTET_STRING, nested2)
587591
assert_equal(true, nested2.indefinite_length)
588-
assert_equal(2, nested2.value.size)
592+
assert_equal(1, nested2.value.size)
589593
oct2 = nested2.value[0]
590594
assert_universal(OpenSSL::ASN1::OCTET_STRING, oct2)
591595
assert_equal(false, oct2.indefinite_length)
592-
assert_universal(OpenSSL::ASN1::EOC, nested2.value[1])
593-
assert_equal(false, nested2.value[1].indefinite_length)
594596
oct3 = asn1.value[2]
595597
assert_universal(OpenSSL::ASN1::OCTET_STRING, oct3)
596598
assert_equal(false, oct3.indefinite_length)
597-
assert_universal(OpenSSL::ASN1::EOC, asn1.value[3])
598-
assert_equal(false, asn1.value[3].indefinite_length)
599599
end
600600

601601
def test_decode_constructed_overread

0 commit comments

Comments
 (0)