Add RSA sign_pss() and verify_pss() methods #76

pkarman · 2016-10-11T17:07:43Z

Why: Supports Probabilistic Signature Scheme for RSA key signing.

rhenium

Thank you for the PR!

I think we should also provide a way to specify the PSS parameters, the salt length and the MGF. Since MGF1 is the only supported by OpenSSL (and the only used in wild) as the MGF, actually the two parameters would be the salt length and the hash algorithm which will be used in MGF1.

rhenium · 2016-10-12T05:10:00Z

ext/openssl/ossl_pkey_rsa.c

+
+    pkey_ctx = EVP_PKEY_CTX_new(pkey, NULL);
+    if (!pkey_ctx)
+        ossl_raise(ePKeyError, "EVP_PKEY_CTX_new");


This leaks. EVP_Digest{Sign,Verify}Init() will allocate a new EVP_PKEY_CTX.

you're saying it leaks because there is no corresponding EVP_PKEY_CTX_free call? I thought the same thing but got a segfault when I included it.

No, EVP_DigestSignInit() will overwrite the pkey_ctx variable and the EVP_PKEY_CTX allocated here is simply lost. The new EVP_PKEY_CTX allocated in EVP_DigestSignInit() will be free'd by EVP_MD_CTX_destroy().

rhenium · 2016-10-12T05:10:35Z

ext/openssl/ossl_pkey_rsa.c

+        ossl_raise(ePKeyError, "EVP_PKEY_CTX_new");
+
+    if (EVP_DigestSignInit(md_ctx, &pkey_ctx, md, NULL, pkey) != 1)
+        ossl_raise(ePKeyError, "EVP_DigestSignInit");


md_ctx is not free'd.

rhenium · 2016-10-12T05:11:52Z

ext/openssl/ossl_pkey_rsa.c

+
+    md_ctx = EVP_MD_CTX_new();
+    if (!md_ctx)
+        ossl_raise(ePKeyError, "EVP_MD_CTX_new");


Use eRSAError (OpenSSL::PKey::RSAError) instead of the generic OpenSSL::PKey::PKeyError in RSA-specific methods.

The indent style is incorrect. Ruby (unfortunately) uses an uncommon style. Replace 8 space characters with a \t.

I would prefer to brace all my if statements instead of relying on whitespace at all but was trying to follow convention I saw in original sign() method.

Is it ok to do:

if (!md_ctx) { ossl_raise(ePKeyError, "EVP_MD_CTX_new"); }

I personally prefer to omit brace for one-line if statements but acceptable.

rhenium · 2016-10-12T05:13:03Z

ext/openssl/ossl_pkey_rsa.c

+    StringValue(signature);
+    StringValue(data);
+
+    if (EVP_PKEY_missing_parameters(pkey))


This is unnecessary for RSA keys.

rhenium · 2016-10-12T05:26:54Z

ext/openssl/ossl_pkey_rsa.c

+    pkey = GetPrivPKeyPtr(self);
+    md = GetDigestPtr(digest);
+    StringValue(data);
+    signature = rb_str_new(0, EVP_PKEY_size(pkey)+16);


What is +16? EVP_PKEY_size(pkey) should be enough.

hmm. I lifted that directly from the original sign() method in ext/openssl/ossl_pkey.c

Are you saying that because this is a RSA-specific pkey its size is sufficient for the RSA signature? That would make sense to me.

EVP_PKEY_size() should return the maximum signature size regardless of the key type. The resulting signature may be smaller but not larger. The current code in PKey#sign and also RSA#{public,private}_{encrypt,decrypt}, DSA#syssign and EC#dsa_sign_asn1 must be incorrect.

It seems it was already in the very first commit b24b432#diff-b11f730c7415dc47e71a5ea29bc843b1R361, I'm not sure why... But eventually they will be fixed as well.

Fixed by 0be7f41.

rhenium · 2016-10-12T05:38:28Z

ext/openssl/ossl_pkey_rsa.c

+ *
+ * Probabilistic Signature Scheme for RSA sign().
+ *
+ * To sign the +String+ +data+, +digest+, an instance of OpenSSL::Digest, must


It's not your fault, but this is not really accurate. digest actually can be a String like "SHA1" since Ruby 1.9.0.

good to know. Should I open a separate PR to fix the doc for sign() ?

That's really nice, thanks!

rhenium · 2016-10-12T05:40:11Z

ext/openssl/ossl_pkey_rsa.c

+#if (OPENSSL_VERSION_NUMBER >= 0x10000000)
+/*
+ * call-seq:
+ *    rsa.sign_pss -> String


(digest, data) is dropped.

rhenium · 2016-10-12T05:46:17Z

ext/openssl/ossl_pkey_rsa.c

+    void *pkey_ptr;
+    const BIGNUM *n, *e;
+
+    GetPKey(self, pkey);


Use GetPKeyRSA() macro.

rhenium · 2016-10-12T05:47:28Z

ext/openssl/ossl_pkey_rsa.c

+
+    pkey_ptr = EVP_PKEY_get0(pkey);
+    if (EVP_PKEY_base_id(pkey) != EVP_PKEY_RSA)
+        ossl_raise(ePKeyError, "key must be RSA");


This is done in GetPKeyRSA().

rhenium · 2016-10-12T05:52:34Z

ext/openssl/ossl_pkey_rsa.c

+    if (!result)
+	    ossl_raise(ePKeyError, NULL);
+
+    assert((long)buf_len <= RSTRING_LEN(signature));


rb_str_set_len() will do this.

oh good. should I open another PR to change that in the original sign() method as well?

I fixed by 0c187b3.

pkarman · 2016-10-13T20:11:36Z

Thanks for the great review @rhenium. I obviously copy/pasted a bunch from the existing sign|verify methods and then updated (some but not enough) to use the newer openssl API. I will get a follow-up commit posted soon for your review.

rhenium · 2016-10-14T03:00:22Z

ext/openssl/ossl_pkey_rsa.c

+
+    result = EVP_DigestSignFinal(md_ctx, (unsigned char *)RSTRING_PTR(signature), &buf_len);
+
+    EVP_MD_CTX_destroy(md_ctx);


I forgot to add: use EVP_MD_CTX_free() instead. EVP_MD_CTX_destroy() was renamed in OpenSSL 1.1.0 (we have a compat macro in ext/openssl/openssl_missing.h for old versions of OpenSSL).

pkarman · 2016-10-14T16:15:52Z

@rhenium I pushed 9133300 with refactors according to your feedback. I'm happy to squash before merging, once this PR reaches an approved place.

pkarman · 2016-10-14T16:16:37Z

I realized I missed this comment:

I think we should also provide a way to specify the PSS parameters, the salt length and the MGF. Since MGF1 is the only supported by OpenSSL (and the only used in wild) as the MGF, actually the two parameters would be the salt length and the hash algorithm which will be used in MGF1.

I'll submit another pass with that feature.

pkarman · 2016-10-14T16:38:44Z

I might need some help with the MGF piece. Reading https://www.openssl.org/docs/manmaster/crypto/EVP_PKEY_CTX_ctrl.html I see EVP_PKEY_CTX_set_rsa_pss_saltlen but am puzzled by MGF. Any ideas?

rhenium · 2016-10-15T06:32:00Z

There is an undocumented macro EVP_PKEY_CTX_set_rsa_mgf1_md(). It seems it was added in OpenSSL 1.0.1, so #if needs to be updated.

rhenium · 2016-10-15T06:32:51Z

ext/openssl/ossl_pkey_rsa.c

+    if (!pkey_ctx) {
+        error = 1;
+        goto err;
+    }


I might not be clear enough. Just remove this. (also from #verify)

rhenium · 2016-10-15T06:33:52Z

ext/openssl/ossl_pkey_rsa.c

+    }
+
+    if (EVP_DigestSignInit(md_ctx, &pkey_ctx, md, NULL, pkey) != 1) {
+        error = 2;


The value of error is only used to determine if an error happens or not. I think this can be made more simple.

if (EVP_abc() != 1) { goto err; } if (EVP_def() != 1) { goto err; } EVP_MD_CTX_free(...); return signature; err: EVP_MD_CTX_free(...); ossl_raise(...)

rhenium · 2016-10-15T06:34:11Z

ext/openssl/ossl_pkey_rsa.c

+        goto err;
+    }
+
+    EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING);


Check the return value. (also in #verify)

rhenium · 2016-10-15T06:37:00Z

ext/openssl/ossl_pkey_rsa.c

+
+    if (EVP_DigestSignFinal(md_ctx, (unsigned char *)RSTRING_PTR(signature), &buf_len) != 1) {
+        error = 4;
+	    goto err;


The indentation should be like:

<four spaces>if (...) { <one tab character>error = 0; <four spaces>}

Indentation width is 4, but every 8 consecutive spaces are replaced with one tab character.

rhenium · 2016-10-15T06:37:38Z

ext/openssl/ossl_pkey_rsa.c

+    rb_str_set_len(signature, buf_len);
+
+ err:
+    EVP_MD_CTX_destroy(md_ctx);


Use EVP_MD_CTX_free().

rhenium · 2016-10-15T06:37:53Z

ext/openssl/ossl_pkey_rsa.c

+ err:
+    EVP_MD_CTX_destroy(md_ctx);
+    if (error) {
+        ossl_raise(ePKeyError, NULL);


Use eRSAError.

rhenium · 2016-10-15T06:40:28Z

ext/openssl/ossl_pkey_rsa.c

+    StringValue(data);
+
+    pkey_ptr = EVP_PKEY_get0(pkey);
+    RSA_get0_key(pkey_ptr, &n, &e, NULL);


In this case, EVP_PKEY_get0_RSA() that returns an RSA * should be used to avoid implicit type conversion between void *.

rhenium · 2016-10-15T06:41:18Z

ext/openssl/ossl_pkey_rsa.c

+        goto err;
+    }
+
+    result = EVP_DigestVerifyFinal(md_ctx, (unsigned char *)RSTRING_PTR(signature), RSTRING_LENINT(signature));


RSTRING_LENINT() raises a RangeError if a too long String is passed as the signature, and md_ctx will leak. It is a fault in the original code... I'll fix soon.

Fixed by e9deede.

rhenium · 2016-10-15T06:42:40Z

ext/openssl/ossl_pkey_rsa.c

+    if (!result)
+	    ossl_raise(ePKeyError, NULL);
+
+    assert((long)buf_len <= RSTRING_LEN(signature));


I fixed by 0c187b3.

**Why**: Supports Probabilistic Signature Scheme for RSA key signing.

pkarman · 2016-10-19T20:06:18Z

@rhenium ok I think I've all your comments and added salt + mgf to the pss params. The method signatures are quite long now, and I just don't know if, idiomatically, it makes sense to make salt+mgf optional with sane defaults. I didn't immediately seen other examples of that idiom in this project.

Thoughts?

tarcieri · 2016-10-19T21:42:49Z

Since this gem targets 2.3+, why not use keyword arguments?

rhenium · 2016-10-20T13:11:16Z

ext/openssl/ossl_pkey_rsa.c

+    if (EVP_DigestSignUpdate(md_ctx, RSTRING_PTR(data), RSTRING_LEN(data)) != 1)
+	goto err;
+
+    if (EVP_DigestSignFinal(md_ctx, (unsigned char *)RSTRING_PTR(signature), &buf_len) != 1)


I overlooked in previous reviews, but buf_len needs to be set to EVP_PKEY_size(pkey) before calling EVP_DigestSignFinal(). Travis CI seems to be broken because of this.

https://www.openssl.org/docs/manmaster/crypto/EVP_DigestSignInit.html:

EVP_DigestSignFinal() signs the data in ctx places the signature in sig. If sig is NULL then the maximum size of the output buffer is written to the siglen parameter. If sig is not NULL then before the call the siglen parameter should contain the length of the sig buffer, if the call is successful the signature is written to sig and the amount of data written to siglen.

rhenium · 2016-10-20T13:17:34Z

ext/openssl/ossl_pkey_rsa.c

+    if (EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING) != 1)
+	goto err;
+
+    if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkey_ctx, NUM2INT(saltlen)) != 1)


NUM2INT() raises RangeError if saltlen is too big for int, so it must be called before allocating the EVP_MD_CTX. Note that NUM2INT() also performs the type check. (also applies to #verify_pss.)

rhenium · 2016-10-20T13:19:58Z

test/test_pkey_rsa.rb

+    signature = key.sign_pss(digest, data, salt_len, hash_alg)
+    assert_equal(true, key.verify_pss(digest, signature, data, salt_len, hash_alg))
+    assert_equal(false, key.verify_pss(digest, signature, invalid_data, salt_len, hash_alg))
+  end


This test needs to be skipped if the OpenSSL version does not support PSS.

rhenium · 2016-10-20T14:13:49Z

ext/openssl/ossl_pkey_rsa.c

+    if (EVP_DigestVerifyUpdate(md_ctx, RSTRING_PTR(data), RSTRING_LEN(data)) != 1)
+	goto err;
+
+    result = EVP_DigestVerifyFinal(md_ctx, (unsigned char *)RSTRING_PTR(signature), RSTRING_LENINT(signature));


Calls for RSTRING_LENINT() must be made before allocating md_ctx because it may raise RangeError.

rhenium · 2016-10-20T14:18:03Z

ext/openssl/ossl_pkey_rsa.c

+ *
+ * The +Integer+ +salt_len+ should be the salt length to use.
+ *
+ * The +String+ +hash_alg+ should be the hash algorithm used in MGF.


Please make it clear that this is a parameter of MGF1, which is just one implementation of MGF (mask generation function).

rhenium · 2016-10-20T14:22:56Z

ext/openssl/ossl_pkey_rsa.c

+
+    md_ctx = EVP_MD_CTX_new();
+    if (!md_ctx)
+	ossl_raise(eRSAError, "EVP_MD_CTX_new");


Why not use goto err here? EVP_MD_CTX_free() can be called against NULL.

pkarman · 2016-10-20T14:58:19Z

@tarcieri I agree. Are there existing examples of named arguments in this gem?

rhenium · 2016-10-20T15:08:35Z

@pkarman Currently, no. Ruby provides rb_get_kwargs() for this purpose. (see https://github.com/ruby/ruby/blob/6f22fc6b70b989245d6ee4d825ea1d9677779b11/doc/extension.rdoc#method-definition, and there are some usage examples in Ruby core code.)

pkarman · 2016-10-20T15:13:52Z

@rhenium thanks, I have made updates based on your review in fd37542

pkarman · 2016-10-20T15:16:42Z

figuring out named arguments is going to take bit more time than I have this week. I can return to that next week.

rhenium · 2016-11-07T02:10:57Z

@pkarman status? Note that the deadline for Ruby 2.4 inclusion is about the end of this month (see https://bugs.ruby-lang.org/projects/ruby-trunk/wiki/ReleaseEngineering24).

pkarman · 2016-11-07T15:45:11Z

I will get to this this week, thanks for the nudge.

Thoughts on what the preferred Ruby method signature should look like? Ideas:

rsa_key.sign_pss(digest, data, saltlen, hash_alg)  # current, no named args
rsa_key.sign_pss(digest, data, saltlen: saltlen, hash_alg: hash_alg)  # named args for optional values
rsa_key.sign_pss(digest: digest, data: data, saltlen: saltlen, hash_alg: hash_alg)  # named args for everything

I kind of like the 2nd one because it has parity with the existing sign and emphasizes that the optional bits are "extra". OTOH, named args for everything would be most consistent.

Same thing for verify_pss.

pkarman · 2016-11-14T16:59:24Z

@rhenium thoughts on syntax?

rhenium · 2016-11-15T16:24:41Z

I like the second one, too, because the two parameters are distinct.

The keyword names can be better. I don't think we need to inherit the abbreviation 'saltlen' here. 'hash_alg' is not clear because PSS can technically use other MGFs than MGF1, while the hash algorithm is specific to MGF1. I suggest 'mgf1_hash'.

As you might have noticed, the manpage of EVP_PKEY_CTX_set_rsa_pss_saltlen() says it accepts two special values, -1 and -2. I think we should provide a better way rather than forcing users to write the magic number in their code. I imagine something like this:

sig = rsa_key.sign_pss(digest, data, salt_length: OpenSSL::PKey::RSA::PSS_MAX_SALT_LENGTH) # too long?
sig = rsa_key.sign_pss(digest, data, salt_length: :max_length)

pkarman · 2016-11-21T21:28:28Z

I've got some of the named args work started but I can tell I'm not going to get it done before end of the month. I'm happy to wait and do it in Dec. Up to you.

rhenium · 2016-11-30T15:46:18Z

Sorry for the delay, I missed the notification...

That's OK, let's integrate in the next release. In that case, the #ifs can be removed, as we won't support OpenSSL < 1.0.1.

KevinLapagna · 2017-08-18T16:14:19Z

Any news on when this will be integrated?

And should it not even be the default (.sign) instead of an alternative function (.sign_pss)? According to the information in this article, the use of these signatures without PSS are considered to be insecure:
Why PKCS#1v1.5 Encryption Should Be Put Out of Our Misery

rhenium · 2017-11-03T17:05:15Z

Ping. I'd be happy to take over this patch and finish the kwargs work so we can make this into our v2.1.0 release which will happen this month.

The behavior of OpenSSL::PKey::RSA#sign is not changing, though, because of compatibility. A PR that improves the documentation would be welcome.

karpet · 2017-11-03T18:17:37Z

Sorry to delay so long on this @rhenium - I moved on from the project I was working on that needed it.
IIRC I got hung up on figuring out how to do the named args.
Happy to have you pick it up and finish if you have time.

Support Probabilistic Signature Scheme for RSA key signing. [ky: the patch was originally submitted as GitHub Pull Request ruby#76. finish keyword arguments handling, update docs, and fix tests.]

rhenium · 2017-11-13T07:17:59Z

See #169.

pkarman mentioned this pull request Oct 11, 2016

Encrypt PII at rest 18F/identity-idp#498

Closed

rhenium reviewed Oct 12, 2016

View reviewed changes

rhenium reviewed Oct 14, 2016

View reviewed changes

rhenium reviewed Oct 15, 2016

View reviewed changes

Peter Karman added 5 commits October 19, 2016 11:16

Add RSA sign_pss() and verify_pss() methods

0a2c888

**Why**: Supports Probabilistic Signature Scheme for RSA key signing.

Fix *_pss documentation

37bf8ce

refactor to avoid leaks, other review feedback

ab1793a

review feedback, mostly whitespace indentation fixes

f9eef35

add salt_len and hash_alg arguments to {sign|verify}_pss

f1f8355

pkarman force-pushed the rsa-sign-pss branch from 9133300 to f1f8355 Compare October 19, 2016 19:56

rhenium reviewed Oct 20, 2016

View reviewed changes

review updates

fd37542

Peter Karman added 2 commits November 16, 2016 09:27

update docs to reflect named args

36f8ca7

add tests for pss

eabbca7

WIP variadic arguments

542e0c7

rhenium mentioned this pull request Nov 13, 2017

[v2] Add RSA sign_pss() and verify_pss() methods #169

Merged

rhenium closed this Nov 13, 2017


		result = EVP_DigestSignFinal(md_ctx, (unsigned char *)RSTRING_PTR(signature), &buf_len);

		EVP_MD_CTX_destroy(md_ctx);

Add RSA sign_pss() and verify_pss() methods #76

Add RSA sign_pss() and verify_pss() methods #76

Conversation

pkarman commented Oct 11, 2016

rhenium left a comment

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

rhenium Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkarman commented Oct 13, 2016

rhenium Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

pkarman commented Oct 14, 2016

pkarman commented Oct 14, 2016

pkarman commented Oct 14, 2016

rhenium commented Oct 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkarman commented Oct 19, 2016

tarcieri commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkarman commented Oct 20, 2016

rhenium commented Oct 20, 2016

pkarman commented Oct 20, 2016

pkarman commented Oct 20, 2016

rhenium commented Nov 7, 2016

pkarman commented Nov 7, 2016

pkarman commented Nov 14, 2016

rhenium commented Nov 15, 2016

pkarman commented Nov 21, 2016

rhenium commented Nov 30, 2016

KevinLapagna commented Aug 18, 2017

rhenium commented Nov 3, 2017

karpet commented Nov 3, 2017

rhenium commented Nov 13, 2017

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 12, 2016 •

edited

Loading

rhenium Oct 14, 2016 •

edited

Loading