Skip to content

Commit 35a1574

Browse files
committed
x509attr: avoid using OpenSSL::ASN1 internals in #value=
OpenSSL::ASN1 is being rewritten in Ruby. To make it easier, let's remove dependency to the instance variables and the internal-use function ossl_asn1_get_asn1type() outside OpenSSL::ASN1. This also fixes the insufficient validation of the passed value with its tagging.
1 parent bb45527 commit 35a1574

File tree

2 files changed

+27
-24
lines changed

2 files changed

+27
-24
lines changed

ext/openssl/ossl_x509attr.c

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -201,37 +201,36 @@ static VALUE
201201
ossl_x509attr_set_value(VALUE self, VALUE value)
202202
{
203203
X509_ATTRIBUTE *attr;
204-
VALUE asn1_value;
205-
int i, asn1_tag;
204+
GetX509Attr(self, attr);
206205

207206
OSSL_Check_Kind(value, cASN1Data);
208-
asn1_tag = NUM2INT(rb_attr_get(value, rb_intern("@tag")));
209-
asn1_value = rb_attr_get(value, rb_intern("@value"));
210-
if (asn1_tag != V_ASN1_SET)
211-
ossl_raise(eASN1Error, "argument must be ASN1::Set");
212-
if (!RB_TYPE_P(asn1_value, T_ARRAY))
213-
ossl_raise(eASN1Error, "ASN1::Set has non-array value");
207+
VALUE der = ossl_to_der(value);
208+
const unsigned char *p = (const unsigned char *)RSTRING_PTR(der);
209+
STACK_OF(ASN1_TYPE) *sk = d2i_ASN1_SET_ANY(NULL, &p, RSTRING_LEN(der));
210+
if (!sk)
211+
ossl_raise(eX509AttrError, "attribute value must be ASN1::Set");
214212

215-
GetX509Attr(self, attr);
216213
if (X509_ATTRIBUTE_count(attr)) { /* populated, reset first */
217-
ASN1_OBJECT *obj = X509_ATTRIBUTE_get0_object(attr);
218-
X509_ATTRIBUTE *new_attr = X509_ATTRIBUTE_create_by_OBJ(NULL, obj, 0, NULL, -1);
219-
if (!new_attr)
220-
ossl_raise(eX509AttrError, NULL);
221-
SetX509Attr(self, new_attr);
222-
X509_ATTRIBUTE_free(attr);
223-
attr = new_attr;
214+
ASN1_OBJECT *obj = X509_ATTRIBUTE_get0_object(attr);
215+
X509_ATTRIBUTE *new_attr = X509_ATTRIBUTE_create_by_OBJ(NULL, obj, 0, NULL, -1);
216+
if (!new_attr) {
217+
sk_ASN1_TYPE_pop_free(sk, ASN1_TYPE_free);
218+
ossl_raise(eX509AttrError, "X509_ATTRIBUTE_create_by_OBJ");
219+
}
220+
SetX509Attr(self, new_attr);
221+
X509_ATTRIBUTE_free(attr);
222+
attr = new_attr;
224223
}
225224

226-
for (i = 0; i < RARRAY_LEN(asn1_value); i++) {
227-
ASN1_TYPE *a1type = ossl_asn1_get_asn1type(RARRAY_AREF(asn1_value, i));
228-
if (!X509_ATTRIBUTE_set1_data(attr, ASN1_TYPE_get(a1type),
229-
a1type->value.ptr, -1)) {
230-
ASN1_TYPE_free(a1type);
231-
ossl_raise(eX509AttrError, NULL);
232-
}
233-
ASN1_TYPE_free(a1type);
225+
for (int i = 0; i < sk_ASN1_TYPE_num(sk); i++) {
226+
ASN1_TYPE *a1type = sk_ASN1_TYPE_value(sk, i);
227+
if (!X509_ATTRIBUTE_set1_data(attr, ASN1_TYPE_get(a1type),
228+
a1type->value.ptr, -1)) {
229+
sk_ASN1_TYPE_pop_free(sk, ASN1_TYPE_free);
230+
ossl_raise(eX509AttrError, "X509_ATTRIBUTE_set1_data");
231+
}
234232
}
233+
sk_ASN1_TYPE_pop_free(sk, ASN1_TYPE_free);
235234

236235
return value;
237236
}

test/openssl/test_x509attr.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ def test_invalid_value
4848
assert_raise(TypeError) {
4949
attr.value = "1234"
5050
}
51+
assert_raise(OpenSSL::X509::AttributeError) {
52+
v = OpenSSL::ASN1::Set([OpenSSL::ASN1::UTF8String("1234")], 17, :EXPLICIT)
53+
attr.value = v
54+
}
5155
assert_equal(test_der, attr.to_der)
5256
assert_raise(OpenSSL::X509::AttributeError) {
5357
attr.oid = "abc123"

0 commit comments

Comments
 (0)