PKCS7 unsigned/signed attributes support

[rxbynerd]

Hi there!

I'm attempting to implement a SCEP server in Ruby to produce certificates for Apple devices as part of a device management solution. As part of this, Apple confirms that the signed PKCS7 response contains both a senderNonce and recipientNonce in the signedAttributes field of the PKCS7 message. micromdm/scep has an implementation of this process written in Go.

I've spent my weekend attempting to add support for PKCS7 attribute assignment to openssl-ruby, but I've hit a wall, hence the draft PR. Specifically, I have some compile errors (submitted below the fold) I've run into, and before continuing I thought it best to start a discussion about my approach to confirm I'm on the right track, and how to proceed.

I should note now that I'm very new to both C extension development & the inner workings of OpenSSL/X509/ASN.1, and I understand I may have taken an incorrect approach. Please don't hesitate to let me know how I can improve, and I would be more than happy to donate this pull request to someone more qualified so they can nurture this to completion. That said, I would really like to see the capability added to Ruby (especially as I'm not the only one asking for it), so I'm more than happy to continue work to get this PR into a mergeable state.

At this juncture, I would like a confirmation as to my approach, and the answers to the following questions:

• Is it okay to use obj_to_asn1obj & ossl_asn1_get_value from outside of ossl_asn1.c? This would solve my compile error, but given my unfamiliarity with C I'm not sure how to make this happen. I'm pretty confident I only need a header declaration, but I'm sensitive to unintentionally clashing with another function somewhere.
• I need to unwrap a given "value" from my attribute from a Ruby-style argument into a void *value suitable for ASN1_TYPE_set (via X509_ATTRIBUTE_create via add_attribute via PKCS7_add_signed_attribute). We already do this with a long case statement in ossl_asn1_get_asn1type — would it be okay to break this case statement out of get_asn1type? I'm currently calling get_asn1type then pulling the "value" from the returned ASN1_TYPE, which does not seem efficient or wise.
• Am I correct in that a "tag" is the same as an "attrtype" for the purposes of PKCS7_add_signed_attribute? It seems that the last two arguments to PKCS7_add_signed_attribute are provided by the single typed value in #add_signed_attribute(oid, value)
• How can I confirm that an ObjectId is an ObjectId in C?
• I've added the OpenSSL::PKCS7::PARTIAL constant but neglected PKCS7_STREAM, as I do not have a use case nor understand it. Is it acceptable to only continue with PARTIAL support?
• Because of OpenSSL's reliance on "numerical IDs" & the lack of connection in openssl-ruby between the numerical ID, we would need to raise an error when an un-registered OID is passed into any add_attribute-style method. Is it okay to explain in this error what's necessary to fix the issue? The PKCS7 class is notably devoid of documentation, which I'm not sure is intentional or not.

Additionally I'm struggling to understand ASN.1 as a concept, especially coming from JSON/Protobuffers land where I have the ability to parse a structure into something with labels. If anyone has any further reading that could assist with this, I would be more than grateful.

Thank you for reading & maintaining this library, and I hope we can work together to get this pull request over the finish line.

---

Current compile errors

-> % rake compile                                                                                                                                                                                                                          
cd tmp/x86_64-darwin20/openssl/3.0.0                                                                                                                                                                                                       
/usr/bin/make                                                                                                                                                                                                                              
compiling ../../../../ext/openssl/ossl_pkcs7.c                                                                                                                                                                                             
../../../../ext/openssl/ossl_pkcs7.c:1011:13: error: implicit declaration of function 'obj_to_asn1obj' is invalid in C99 [-Werror,-Wimplicit-function-declaration]                                                                         
    a1obj = obj_to_asn1obj(ossl_asn1_get_value(oid)); // TODO: error check                                                                                                                                                                 
            ^                                                                                                                                                                                                                              
../../../../ext/openssl/ossl_pkcs7.c:1011:28: error: implicit declaration of function 'ossl_asn1_get_value' is invalid in C99 [-Werror,-Wimplicit-function-declaration]                                                                    
    a1obj = obj_to_asn1obj(ossl_asn1_get_value(oid)); // TODO: error check                                                                                                                                                                 
                           ^                                                                                                                                                                                                               
../../../../ext/openssl/ossl_pkcs7.c:1011:11: warning: incompatible integer to pointer conversion assigning to 'ASN1_OBJECT *' (aka 'struct asn1_object_st *') from 'int' [-Wint-conversion]                                               
    a1obj = obj_to_asn1obj(ossl_asn1_get_value(oid)); // TODO: error check                                                                                                                                                                 
          ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                                                                                                       
../../../../ext/openssl/ossl_pkcs7.c:1025:11: error: implicit declaration of function 'ossl_asn1_tag' is invalid in C99 [-Werror,-Wimplicit-function-declaration]                                                                          
    tag = ossl_asn1_tag(value); // TODO: error check                                                                                                                                                                                       
          ^                                                                                                                                                                                                                                
../../../../ext/openssl/ossl_pkcs7.c:1066:61: error: member reference type 'ASN1_TYPE *' (aka 'struct asn1_type_st *') is a pointer; did you mean to use '->'?                                                                             
    PKCS7_add_signed_attribute(p7si, nid, tag, value_as_type.value);                                                                                                                                                                       
                                               ~~~~~~~~~~~~~^                                                                                                                                                                              
                                                            ->                                                                                                                                                                             
../../../../ext/openssl/ossl_pkcs7.c:1066:48: error: passing 'union (anonymous union at /Users/rubynerd/.rbenv/versions/3.0.0/openssl/include/openssl/asn1.h:446:5)' to parameter of incompatible type 'void *'                            
    PKCS7_add_signed_attribute(p7si, nid, tag, value_as_type.value);                                                                                                                                                                       
                                               ^~~~~~~~~~~~~~~~~~~                                                                                                                                                                         
/Users/rubynerd/.rbenv/versions/3.0.0/openssl/include/openssl/pkcs7.h:274:38: note: passing argument to parameter 'data' here                                                                                                              
                               void *data);                                                                                                                                                                                                
                                     ^                                                                                                                                                                                                     
1 warning and 5 errors generated.                                                                                                                                                                                                          
make: *** [ossl_pkcs7.o] Error 1                                                                                                                                                                                                           
rake aborted!                                                                                                                                                                                                                              
Command failed with status (2): [/usr/bin/make...]                                                                                                                                                                                         

testharness.rb

CERTIFICATE = OpenSSL::X509::Certificate.new(File.read("./scep.pem"))
PRIVATE_KEY = OpenSSL::PKey::RSA.new(File.read("./scep-key.pem"))

# fake response, would be from OpenSSL::PKCS7.encrypt
response = OpenSSL::ASN1::Sequence.new([
  OpenSSL::ASN1::ObjectId.new('1.2.840.113549.1.7.2'),
])

puts "OpenSSL::PKCS7::PARTIAL: #{OpenSSL::PKCS7::PARTIAL}"

res = OpenSSL::PKCS7.sign(CERTIFICATE, PRIVATE_KEY, nil, [], OpenSSL::PKCS7::BINARY | OpenSSL::PKCS7::PARTIAL)

puts "res.signers: #{res.signers}"

# effectively:
# res.signers.first.add_signed_attribute(OpenSSL::ASN1::ObjectId.new('1.x.x'), OpenSSL::ASN1::OctetString.new(""))

res.finalize(response.to_der, OpenSSL::PKCS7::BINARY | OpenSSL::PKCS7::PARTIAL)

puts res.to_pem

[rhenium]

Thank you for your time looking into this!

Unfortunately, the PKCS#7 module hasn't got much attention since it was initially implemented in ~2005 and it's still lacking core features and documentation. :(

signedAttrs/unsignedAttrs is specified in RFC 5652:

   SignerInfo ::= SEQUENCE {
     [...],
     signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL,
     [...],
     unsignedAttrs [1] IMPLICIT UnsignedAttributes OPTIONAL }

   SignedAttributes ::= SET SIZE (1..MAX) OF Attribute
   UnsignedAttributes ::= SET SIZE (1..MAX) OF Attribute

   Attribute ::= SEQUENCE {
     attrType OBJECT IDENTIFIER,
     attrValues SET OF AttributeValue }

   AttributeValue ::= ANY

[...]
      attrValues is a set of values that comprise the attribute.  The
      type of each value in the set can be determined uniquely by
      attrType.  The attrType can impose restrictions on the number of
      items in the set.

It seems that PKCS7_add_signed_attribute(si, nid, attrtype_of_value, value) is a specialized function to create an Attribute containing a single-valued SET as attrValues (and replace the current Attribute in signedAttrs with the same attrType=nid with the new one, if exists).

It's probably useful to write programs in C, but I suspect it would be simpler for us to implement a wrapper for the lower-level function PKCS7_set_signed_attributes() since we have OpenSSL::X509::Attribute to represent an Attribute object. It's currently used by OpenSSL::X509::Request, aka. PKCS#10. So we can use something like:

attrValue = OpenSSL::ASN1::Set.new([OpenSSL::ASN1::OctetString.new("data")])
si = OpenSSL::PKCS7::SignerInfo.new(..)
si.signed_attributes = [
  OpenSSL::X509::Attribute.new("attrType", attrValue),
]

• Is it okay to use obj_to_asn1obj & ossl_asn1_get_value from outside of ossl_asn1.c? This would solve my compile error, but given my unfamiliarity with C I'm not sure how to make this happen. I'm pretty confident I only need a header declaration, but I'm sensitive to unintentionally clashing with another function somewhere.

They are not needed if we go with PKCS7_set_signed_attributes(), but - yes. Habitually ossl_ prefix is added to identifiers when exporting.

• I need to unwrap a given "value" from my attribute from a Ruby-style argument into a void *value suitable for ASN1_TYPE_set (via X509_ATTRIBUTE_create via add_attribute via PKCS7_add_signed_attribute). We already do this with a long case statement in ossl_asn1_get_asn1type — would it be okay to break this case statement out of get_asn1type? I'm currently calling get_asn1type then pulling the "value" from the returned ASN1_TYPE, which does not seem efficient or wise.

OpenSSL::X509::Attribute#value= currently does this. The only other user of ossl_asn1_get_asn1type, ASN1#to_der, doesn't really need ASN1_TYPE either, so I think it should be refactored.

• Am I correct in that a "tag" is the same as an "attrtype" for the purposes of PKCS7_add_signed_attribute? It seems that the last two arguments to PKCS7_add_signed_attribute are provided by the single typed value in #add_signed_attribute(oid, value)

I think so. PKCS7_add_signed_attribute()'s third parameter seems to be corresponding to the type of AttributeValue.

• How can I confirm that an ObjectId is an ObjectId in C?
• I've added the OpenSSL::PKCS7::PARTIAL constant but neglected PKCS7_STREAM, as I do not have a use case nor understand it. Is it acceptable to only continue with PARTIAL support?

I don't understand the difference. They were identical when it was first introduced. openssl/openssl@60f2063
I think PARTIAL only is fine.

• Because of OpenSSL's reliance on "numerical IDs" & the lack of connection in openssl-ruby between the numerical ID, we would need to raise an error when an un-registered OID is passed into any add_attribute-style method. Is it okay to explain in this error what's necessary to fix the issue? The PKCS7 class is notably devoid of documentation, which I'm not sure is intentional or not.

To my understanding, NIDs are mostly for convenience (so C programs can use NID_* macro) and there should be a way to pass ASN1_OBJECT instead, maybe as a lower-level function.

[rhenium]

PKCS7#add_data seems to do the similar thing as PKCS7_final(p7, in, PKCS7_BINARY) already.

[rhenium]

Style: Until we drop support for Ruby 2.6, we have to avoid C99 features (// comments and mixed declarations and statements).

[rxbynerd]

@rhenium so I've taken a stab at PKCS7::SignerInfo#signed_attributes= but unfortunately I've run into an issue: it simply isn't working. The code compiles and runs, but the resulting PKCS7 message doesn't contain any signed attributes.

I think something's going on with DupX509AttrPtr, and it's not correctly duplicating/retaining references, but I can't find an up_ref function for X509_ATTRIBUTE, and honestly I'm understanding why OpenSSL::PKCS7 doesn't have much in the way of documentation given the underlying OpenSSL code 😭

Do you have any advice for how to debug this issue?

[rhenium]

A problem is that OpenSSL::PKCS7#add_signer was not working correctly at all. #423

[rxbynerd]

I've moved this to the backburner as I've changed languages for this project, but I think I've got the PKCS7 authenticated attribute setter working! Testing just a setter is quite questionable though, so I think for feature completeness this PR will need a getter for the attributes of a parsed message as well. I'll figure this one out when I've got some spare cycles 👍