Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

instead of looking of NIDs and then using X509V3_EXT_nconf_nid, #141

Merged
merged 3 commits into from Aug 31, 2023

Conversation

mcr
Copy link
Contributor

@mcr mcr commented Aug 28, 2017

instead just pass strings to X509V3_EXT_nconf, which has all the logic for
processing dealing with generic extensions.
This is not yet right; it may insert two additional bytes to the value, and does not deal with the critical correctly. This pull request is for discussion.

@rhenium
Copy link
Member

rhenium commented Aug 29, 2017

The ruby-core mailing list or this GitHub issue tracker is the right place for questions about ruby-openssl.

From your email on openssl-users:

Like Bob Moskowitz who has been posting about IDevID, I have also been
creating certificates with custom/private extensions in aid of creating
IDevIDs.

I'm one of the authors of both:
    https://datatracker.ietf.org/doc/draft-ietf-anima-bootstrapping-keyinfra/
and https://datatracker.ietf.org/doc/draft-ietf-anima-voucher/

I want to create certificates with custom/private extensions programatically,
and I found it very difficult to do using the current ruby-openssl modules.

I was sure that it must be possible from the C-API, and found that this
following change to ruby's interface worked well for me:
    https://github.com/ruby/openssl/pull/141


    } I haven't found a seperate openssl-ruby list as yet, so please  {
    } excuse the BCC, and as this is not a security issue, I haven't  {
    } used that address.  Please redirect me.                         {


The critical change being:
-    ext = X509V3_EXT_nconf_nid(conf, ctx, nid, RSTRING_PTR(valstr));
+    ext = X509V3_EXT_nconf(conf, ctx, RSTRING_PTR(oid), RSTRING_PTR(value));

Because EXT_nconf does all the nid lookups, and processes the generics
itself, no point in the ruby-ssl code limiting itself to OIDs predefined
in objects.h by it's use of nid lookups directly.


    ** I'm asking the Openssl team if I'm using the API reasonably **
    ** Clearly, I have some possible garbage that has leaked in    **

My code looks like:

    @idevid.add_extension(ef.create_extension(
                          "subjectAltName",
                          sprintf("otherName:1.3.6.1.4.1.46930.1;UTF8:%s",
                                   self.sanitized_eui64),
                          false))

which let me put a private extension having my private hardware number into
the SAN.  That works just great, I think.  I have in fact extracted the
result in metdtls code in my constrained device a few months ago.

After my changes above, I could now also do: (46930 being my PEN)

@idevid.add_extension(ef.create_extension(
                      "1.3.6.1.4.1.46930.2",
                      "ASN1:UTF8String:http://www.sandelman.ca",
                      false))

which would insert a MASAURL (using a PEN until we get an id-pe OID
allocated) as described in: https://tools.ietf.org/html/draft-ietf-anima-bootstrapping-keyinfra-07#section-2.2

Of concern is that when I look at the resulting certificate:

dooku-[fountain/spec/certs](2.3.0) mcr 10006 %openssl x509 -noout -text -in
12-00-00-66-4D-02.crt
Certificate:
...
            X509v3 Subject Alternative Name:
                othername:
            1.3.6.1.4.1.46930.2:
                ..http://www.sandelman.ca

Looking at a hexdump I see "0x0c" and "0x17" prior to the http, but maybe
it's a length or something.... I wondered if there was garbage or a UTF-8 BOM
or something inserted..  so, I pointed asn1parse at the result, and I see:

400:d=5  hl=2 l=   9 prim: OBJECT            :1.3.6.1.4.1.46930.2
411:d=5  hl=2 l=  25 prim: OCTET STRING      [HEX DUMP]:0C17687474703A2F2F7777772E73616E64656C6D616E2E6361

So the 0xc and 0x17 are really there.

Clearly, it's not being take as an UTF8String (because I would expect to see
UTF8STRING as the type if it were), yet the ASN1:UTF8String is in fact being
processed somehow.

If you want to see the whole certificate result, as sample/test data to my
Registrar:
  https://github.com/mcr/fountain/blob/master/spec/certs/12-00-00-66-4D-02.crt

--
]               Never tell me the odds!                 | ipv6 mesh networks [
]   Michael Richardson, Sandelman Software Works        | network architect  [
]     mcr@sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [

@rhenium
Copy link
Member

rhenium commented Aug 29, 2017

The critical change being:
-    ext = X509V3_EXT_nconf_nid(conf, ctx, nid, RSTRING_PTR(valstr));
+    ext = X509V3_EXT_nconf(conf, ctx, RSTRING_PTR(oid), RSTRING_PTR(value));

Because EXT_nconf does all the nid lookups, and processes the generics
itself, no point in the ruby-ssl code limiting itself to OIDs predefined
in objects.h by it's use of nid lookups directly.

NIDs can be added at run time with OpenSSL::ASN1::ObjectId.register (which calls OBJ_create()), but yes, this should be fixed.

For whatever reason, OpenSSL::X509::ExtensionFactory#create_ext has accepted long names which aren't handled by the non-generic extensions path of X509V3_EXT_nconf(). For compatibility I guess it will be like this.

const char *oid_cstr = StringValueCStr(oid);
int nid = OBJ_ln2nid(oid_cstr);
if (nid != NID_undef)
    oid_cstr = OBJ_nid2sn(nid);
ext = X509V3_EXT_nconf(conf, ctx, oid_cstr, RSTRING_PTR(valstr));
@idevid.add_extension(ef.create_extension(
                      "1.3.6.1.4.1.46930.2",
                      "ASN1:UTF8String:http://www.sandelman.ca",
                      false))
[...]

400:d=5  hl=2 l=   9 prim: OBJECT            :1.3.6.1.4.1.46930.2
411:d=5  hl=2 l=  25 prim: OCTET STRING      [HEX DUMP]:0C17687474703A2F2F7777772E73616E64656C6D616E2E6361

So the 0xc and 0x17 are really there.

Clearly, it's not being take as an UTF8String (because I would expect to see
UTF8STRING as the type if it were), yet the ASN1:UTF8String is in fact being
processed somehow.

It's working as expected. The ASN.1 type definition of Extension is:

Extension  ::=  SEQUENCE  {
     extnID      OBJECT IDENTIFIER,
     critical    BOOLEAN DEFAULT FALSE,
     extnValue   OCTET STRING
                 -- contains the DER encoding of an ASN.1 value
                 -- corresponding to the extension type identified
                 -- by extnID
     }

The leading "\x0c\x17" is the BER tag and the length of the UTF8String encapsulated in the 'extnValue'.

require "openssl"
require "pp"

ef = OpenSSL::X509::ExtensionFactory.new
# Workaround -- allocate a NID
OpenSSL::ASN1::ObjectId.register("1.3.6.1.4.1.46930.2", "x-oid", "x-oid")
e = ef.create_extension("x-oid",
                        "ASN1:UTF8String:http://www.sandelman.ca",
                        false)

pp x = OpenSSL::ASN1.decode(e.to_der)
# =>
# #<OpenSSL::ASN1::Sequence:0x0000000000ba6f58
#  @infinite_length=false,
#  @tag=16,
#  @tag_class=:UNIVERSAL,
#  @tagging=nil,
#  @value=
#   [#<OpenSSL::ASN1::ObjectId:0x0000000000ba73b8
#     @infinite_length=false,
#     @tag=6,
#     @tag_class=:UNIVERSAL,
#     @tagging=nil,
#     @value="x-oid">,
#    #<OpenSSL::ASN1::OctetString:0x0000000000ba7200
#     @infinite_length=false,
#     @tag=4,
#     @tag_class=:UNIVERSAL,
#     @tagging=nil,
#     @value="\f\x17http://www.sandelman.ca">]>

pp OpenSSL::ASN1.decode(x.value[1].value)
# =>
# #<OpenSSL::ASN1::UTF8String:0x0000000000b70908
#  @infinite_length=false,
#  @tag=12,
#  @tag_class=:UNIVERSAL,
#  @tagging=nil,
#  @value="http://www.sandelman.ca">

@mcr
Copy link
Contributor Author

mcr commented Aug 29, 2017 via email

@rhenium
Copy link
Member

rhenium commented Aug 30, 2017

I did not find a way to call OBJ_create() from ruby. Is there one?
Many OpenSSL FAQs suggest you need to hack objects.h and recompile, which is
clearly a PITA if you are trying to live above distribute ruby binaries, so I
was looking for another way.

OpenSSL::ASN1::ObjectId.register (in ext/openssl/ossl_asn1.c) calls.

Are there regression tests which cover that?
I was hoping travis would tell me about such failures that I didn't know
about :-)

No, sadly. It would be helpful if you could add some to test/test_x509ext.rb.

@mcr
Copy link
Contributor Author

mcr commented Sep 5, 2017

I have revised the code:

  • retains the critical check
  • includes the nid lookup you suggested
  • moves all declations to the top.

@mcr
Copy link
Contributor Author

mcr commented Sep 5, 2017

I will investigate adding a regress test case for this, can you give me an example of the nid lookup that would not have worked without the ln2nid() call?

@rhenium
Copy link
Member

rhenium commented Sep 15, 2017

The changes in ext/openssl/ossl_x509ext.c look good. Thanks.

I will investigate adding a regress test case for this, can you give me an example of the nid lookup that would not have worked without the ln2nid() call?

For example...

$ ruby -ropenssl -e'p OpenSSL::ASN1::ObjectId.new("keyUsage").ln'              
"X509v3 Key Usage"

@ioquatix
Copy link
Member

@mcr do you mind rebasing on master and explaining where we are at with this PR?

@mcr
Copy link
Contributor Author

mcr commented Dec 5, 2019 via email

domq pushed a commit to domq/Crypt-OpenSSL-CA that referenced this pull request Oct 28, 2021
It seems like we can now look up `X509V3_EXT`s directly by their name, instead of having to go through the NID system.

- With inspiration from (open and unmerged for 4 years) ruby/openssl#141, use `X509V3_EXT_nconf` whenever the NID is *not* known at build time
- As a result, `new_from_X509V3_EXT_METHOD()` now takes a string instead of an NID as the second parameter
- Relax failure test over unsupported CRL extensions (so as to OK on the new error message)
@rhenium rhenium modified the milestones: v3.0, v3.1 Sep 8, 2022
@rhenium
Copy link
Member

rhenium commented Aug 31, 2023

I'm sorry it took so long to reply. This patch is still relevant and the changes in ext/openssl looks good to me.

Let me add some test cases.

@rhenium rhenium force-pushed the master branch 2 times, most recently from d1c9078 to f638f2e Compare August 31, 2023 06:03
The test case test_error_data utilizes the error message generated by
X509V3_EXT_nconf_nid(). The next commit will use X509V3_EXT_nconf(),
which generates a slightly different error message. Let's adapt the
check to it.
OpenSSL::X509::ExtensionFactory#create_ext and #create_extensions
accepts both sn (short names) and ln (long names) for registered OIDs.

This is different from the behavior of the openssl command-line utility
which accepts only sn in openssl.cnf keys.

Add a test case to check this.
instead of looking of NIDs and then using X509V3_EXT_nconf_nid,
instead just pass strings to X509V3_EXT_nconf, which has all the logic for
processing dealing with generic extensions
also process the oid through ln2nid() to retain compatibility.

[rhe: tweaked commit message and added a test case]
@rhenium
Copy link
Member

rhenium commented Aug 31, 2023

I had to add a cast from const char * to char * for OpenSSL 1.0.2's X509V3_EXT_nconf().

@mcr
Copy link
Contributor Author

mcr commented Aug 31, 2023

I had to add a cast from const char * to char * for OpenSSL 1.0.2's X509V3_EXT_nconf().

Given how insecure OpenSSL 1.0.x is, do we really care?
It would be nice to bring us up to 1.1.x as the minimum, as we try to get to 3.x :-)

@junaruga
Copy link
Member

junaruga commented Aug 31, 2023

I had to add a cast from const char * to char * for OpenSSL 1.0.2's X509V3_EXT_nconf().

Given how insecure OpenSSL 1.0.x is, do we really care? It would be nice to bring us up to 1.1.x as the minimum, as we try to get to 3.x :-)

The openssl/ruby currently maintains the old OpenSSL versions even when these are the end of life (EOL).

- openssl-1.0.2u # EOL
- openssl-1.1.0l # EOL
- openssl-1.1.1v
- openssl-3.0.10
- openssl-3.1.2

As a real use case, Red Hat Enterprise Linux (RHEL) 7's latest version RHEL 7.9 has OpenSSL version 1.0.2k in my investigation. As RHEL 7's end of life is in June 2024, according to this announcement. I assume that some people may use the openssl gem in the environment until the EOL, June 2024.

@rhenium
Copy link
Member

rhenium commented Aug 31, 2023

I personally don't believe OpenSSL 1.0.2 support is a strict requirement for us, but we should keep master branch compatible with 1.0.2 until we officially drop it.

@rhenium
Copy link
Member

rhenium commented Aug 31, 2023

CI is happy with -Werror now. Let's merge this.

@rhenium rhenium merged commit a685991 into ruby:master Aug 31, 2023
43 checks passed
@rhenium
Copy link
Member

rhenium commented Aug 31, 2023

Thank you so much!

@ioquatix
Copy link
Member

Nice work team!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants