Correct the check of RSA_FLAG_SIGN_VER

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Correct the check of RSA_FLAG_SIGN_VER

Eichenberger, John-2

Honeywell Internal

 

I think I have an answer for why this commit needed to be reverted:

Author:                Dr. Stephen Henson <[hidden email]>

Author date:      2 years ago (12/20/2015 10:18:43 AM)

Commit date:    2 years ago (12/20/2015 11:27:03 AM)

Commit hash:    6656ba7152dfe4bba865e327dd362ea08544aa80

Children:              1c7de36f62

Parent(s):            17592f323a

 

Don't check RSA_FLAG_SIGN_VER.

 

Reviewed-by: Richard Levitte <[hidden email]>

 

The change made in that commit was to simply remove the attempt to check for the RSA_FLAG_SIGN_VER flag. But that’s not what is wrong with this code that required changing.

The change should be to add “meth” prior to flags:

 

@@ -84,7 +89,7 @@ int RSA_sign(int type, const unsigned char *m, unsigned int m_len,

         return 0;

     }

#endif

-    if ((rsa->flags & RSA_FLAG_SIGN_VER) && rsa->meth->rsa_sign) {

+    if ((rsa->meth->flags & RSA_FLAG_SIGN_VER) && rsa->meth->rsa_sign) {

         return rsa->meth->rsa_sign(type, m, m_len, sigret, siglen, rsa);

     }

     /* Special case: SSL signature, just check the length */

@@ -293,7 +298,7 @@ int RSA_verify(int dtype, const unsigned char *m, unsigned int m_len,

                const unsigned char *sigbuf, unsigned int siglen, RSA *rsa)

{

 

-    if ((rsa->flags & RSA_FLAG_SIGN_VER) && rsa->meth->rsa_verify) {

+    if ((rsa->meth->flags & RSA_FLAG_SIGN_VER) && rsa->meth->rsa_verify) {

         return rsa->meth->rsa_verify(dtype, m, m_len, sigbuf, siglen, rsa);

     }

 

--

 

-Ike-

  John Eichenberger

Intermec by Honeywell
Principal Engineer: Sustaining Engineering

425.921.4507


--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: Correct the check of RSA_FLAG_SIGN_VER

Richard Levitte - VMS Whacker-2
In message <[hidden email]> on Tue, 3 Apr 2018 22:55:06 +0000, "Eichenberger, John" <[hidden email]> said:

John.Eichenberger> Honeywell Internal
John.Eichenberger>
John.Eichenberger> I think I have an answer for why this commit needed to be reverted:
John.Eichenberger>
John.Eichenberger> Author: Dr. Stephen Henson <[hidden email]>
John.Eichenberger> Author date: 2 years ago (12/20/2015 10:18:43 AM)
John.Eichenberger> Commit date: 2 years ago (12/20/2015 11:27:03 AM)
John.Eichenberger> Commit hash: 6656ba7152dfe4bba865e327dd362ea08544aa80
John.Eichenberger> Children: 1c7de36f62
John.Eichenberger> Parent(s): 17592f323a
John.Eichenberger>
John.Eichenberger> Don't check RSA_FLAG_SIGN_VER.
John.Eichenberger>
John.Eichenberger> Reviewed-by: Richard Levitte <[hidden email]>
John.Eichenberger>
John.Eichenberger> The change made in that commit was to simply remove
John.Eichenberger> the attempt to check for the RSA_FLAG_SIGN_VER flag.
John.Eichenberger> But that’s not what is wrong with this code that
John.Eichenberger> required changing. The change should be to add
John.Eichenberger> “meth” prior to flags:

Well, not quite, actually.  We can easily study the code prior to this
change by looking at the 1.0.2 source.

1. in RSA_new_method(), which is used to create new instances of the
   RSA structure, there's this line:

    ret->flags = ret->meth->flags & ~RSA_FLAG_NON_FIPS_ALLOW;

   So that makes the check of rsa->flags valid, no need to go via
   rsa->meth

2. In rsa.h (crypto/rsa/rsa.h in 1.0.2), you'll find this comment in
   the middle of the definition of rsa_meth_st

    /*
     * New sign and verify functions: some libraries don't allow arbitrary
     * data to be signed/verified: this allows them to be used. Note: for
     * this to work the RSA_public_decrypt() and RSA_private_encrypt() should
     * *NOT* be used RSA_sign(), RSA_verify() should be used instead. Note:
     * for backwards compatibility this functionality is only enabled if the
     * RSA_FLAG_SIGN_VER option is set in 'flags'.
     */

   Do note the note on backward compatibility...  you see, there were
   versions of OpenSSL where the fields 'rsa_sign' and 'rsa_verify'
   didn't exist (they appeared in OpenSSL 0.9.5), so for the sake of
   allowing older applications to work with the newer OpenSSL without
   recompilation, we required all new RSA method implementations to
   use that flag to have the 'rsa_sign' and 'rsa_verify' functions
   used.  Without that flag, those functions were assumed not to
   exist, that the RSA method structure was pre-0.9.5.  However, this
   was somewhere in 2000.

   Fast forward to 2015, when we were starting to make certain types
   opaque, and someone noticed this flag still hanging around, and we
   figured that 0.9.5 is long gone, and 1.0.1 was a year away from its
   end of life, and we figured that the reason to have this flag at
   all was a matter of years long past, it was time to simply drop its
   use.  It had grown to become irrelevant a long time ago.

Cheers,
Richard

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: [External] Re: Correct the check of RSA_FLAG_SIGN_VER

Eichenberger, John-2
Honeywell Internal

Your answer #1 below presumes that RSA_new_method() is called AFTER RSA_set_method().  Is that a valid presumption?  How is that documented as a requirement?

When the flag is set in a call to RSA_set_method() after a call to RSA_new(), the flag gets ignored with the current implementation.

-Ike-
  John Eichenberger
Intermec by Honeywell
Principal Engineer: Sustaining Engineering
425.921.4507

-----Original Message-----
From: openssl-users [mailto:[hidden email]] On Behalf Of Richard Levitte
Sent: Tuesday, April 03, 2018 7:43 PM
To: [hidden email]
Subject: [External] Re: [openssl-users] Correct the check of RSA_FLAG_SIGN_VER

In message <[hidden email]> on Tue, 3 Apr 2018 22:55:06 +0000, "Eichenberger, John" <[hidden email]> said:

John.Eichenberger> Honeywell Internal
John.Eichenberger>
John.Eichenberger> I think I have an answer for why this commit needed to be reverted:
John.Eichenberger>
John.Eichenberger> Author: Dr. Stephen Henson <[hidden email]> John.Eichenberger> Author date: 2 years ago (12/20/2015 10:18:43 AM) John.Eichenberger> Commit date: 2 years ago (12/20/2015 11:27:03 AM) John.Eichenberger> Commit hash: 6656ba7152dfe4bba865e327dd362ea08544aa80
John.Eichenberger> Children: 1c7de36f62
John.Eichenberger> Parent(s): 17592f323a John.Eichenberger> John.Eichenberger> Don't check RSA_FLAG_SIGN_VER.
John.Eichenberger>
John.Eichenberger> Reviewed-by: Richard Levitte <[hidden email]> John.Eichenberger> John.Eichenberger> The change made in that commit was to simply remove John.Eichenberger> the attempt to check for the RSA_FLAG_SIGN_VER flag.
John.Eichenberger> But that's not what is wrong with this code that John.Eichenberger> required changing. The change should be to add John.Eichenberger> "meth" prior to flags:

Well, not quite, actually.  We can easily study the code prior to this change by looking at the 1.0.2 source.

1. in RSA_new_method(), which is used to create new instances of the
   RSA structure, there's this line:

    ret->flags = ret->meth->flags & ~RSA_FLAG_NON_FIPS_ALLOW;

   So that makes the check of rsa->flags valid, no need to go via
   rsa->meth

2. In rsa.h (crypto/rsa/rsa.h in 1.0.2), you'll find this comment in
   the middle of the definition of rsa_meth_st

    /*
     * New sign and verify functions: some libraries don't allow arbitrary
     * data to be signed/verified: this allows them to be used. Note: for
     * this to work the RSA_public_decrypt() and RSA_private_encrypt() should
     * *NOT* be used RSA_sign(), RSA_verify() should be used instead. Note:
     * for backwards compatibility this functionality is only enabled if the
     * RSA_FLAG_SIGN_VER option is set in 'flags'.
     */

   Do note the note on backward compatibility...  you see, there were
   versions of OpenSSL where the fields 'rsa_sign' and 'rsa_verify'
   didn't exist (they appeared in OpenSSL 0.9.5), so for the sake of
   allowing older applications to work with the newer OpenSSL without
   recompilation, we required all new RSA method implementations to
   use that flag to have the 'rsa_sign' and 'rsa_verify' functions
   used.  Without that flag, those functions were assumed not to
   exist, that the RSA method structure was pre-0.9.5.  However, this
   was somewhere in 2000.

   Fast forward to 2015, when we were starting to make certain types
   opaque, and someone noticed this flag still hanging around, and we
   figured that 0.9.5 is long gone, and 1.0.1 was a year away from its
   end of life, and we figured that the reason to have this flag at
   all was a matter of years long past, it was time to simply drop its
   use.  It had grown to become irrelevant a long time ago.

Cheers,
Richard

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: [External] Re: Correct the check of RSA_FLAG_SIGN_VER

Eichenberger, John-2
Knowing now that RSA_new_method() copies the RSA_METHOD flags field to the RSA flags field I modified my instance of wpa_supplicant such that it works even without changing OpenSSL.
However, I still say that this is a bug with OpenSSL.  Let me tell you the short story that leads me to that conclusion.

wpa_supplicant has a bit of code that
1.  Allocates and defines a RSA_METHOD structure.
2. calls RSA_new();
3. calls RSA_set_method().

In that code the rsa_sign and rsa_verify members of the RSA_METHOD structure are not defined, thus making it compatible with the really old versions of OpenSSL.
But I need to change that and use the rsa_sign method, so I set the RSA_FLAG_SIGN_VER bit of the RSA_METHOD structure to indicate that I need to use that.
But my rsa_sign method was never called.

My "fix" is to set that same bit in the RSA flags field before calling RSA_set_method.
        rsa->flags |= RSA_FLAG_SIGN_VER;

But shouldn't OpenSSL work without me having to do that?
Perhaps RSA_set_method() should also copy the flags field to match what RSA_new_method does?

Either that, or the meth->flags field should be tested.
Which is it?

-Ike-
  John Eichenberger
Intermec by Honeywell
Principal Engineer: Sustaining Engineering
425.921.4507

-----Original Message-----
From: openssl-users [mailto:[hidden email]] On Behalf Of Eichenberger, John
Sent: Wednesday, April 04, 2018 9:07 AM
To: [hidden email]
Subject: Re: [openssl-users] [External] Re: Correct the check of RSA_FLAG_SIGN_VER

Honeywell Internal

Your answer #1 below presumes that RSA_new_method() is called AFTER RSA_set_method().  Is that a valid presumption?  How is that documented as a requirement?

When the flag is set in a call to RSA_set_method() after a call to RSA_new(), the flag gets ignored with the current implementation.

-Ike-
  John Eichenberger
Intermec by Honeywell
Principal Engineer: Sustaining Engineering
425.921.4507

-----Original Message-----
From: openssl-users [mailto:[hidden email]] On Behalf Of Richard Levitte
Sent: Tuesday, April 03, 2018 7:43 PM
To: [hidden email]
Subject: [External] Re: [openssl-users] Correct the check of RSA_FLAG_SIGN_VER

In message <[hidden email]> on Tue, 3 Apr 2018 22:55:06 +0000, "Eichenberger, John" <[hidden email]> said:

John.Eichenberger> Honeywell Internal
John.Eichenberger>
John.Eichenberger> I think I have an answer for why this commit needed to be reverted:
John.Eichenberger>
John.Eichenberger> Author: Dr. Stephen Henson <[hidden email]> John.Eichenberger> Author date: 2 years ago (12/20/2015 10:18:43 AM) John.Eichenberger> Commit date: 2 years ago (12/20/2015 11:27:03 AM) John.Eichenberger> Commit hash: 6656ba7152dfe4bba865e327dd362ea08544aa80
John.Eichenberger> Children: 1c7de36f62
John.Eichenberger> Parent(s): 17592f323a John.Eichenberger> John.Eichenberger> Don't check RSA_FLAG_SIGN_VER.
John.Eichenberger>
John.Eichenberger> Reviewed-by: Richard Levitte <[hidden email]> John.Eichenberger> John.Eichenberger> The change made in that commit was to simply remove John.Eichenberger> the attempt to check for the RSA_FLAG_SIGN_VER flag.
John.Eichenberger> But that's not what is wrong with this code that John.Eichenberger> required changing. The change should be to add John.Eichenberger> "meth" prior to flags:

Well, not quite, actually.  We can easily study the code prior to this change by looking at the 1.0.2 source.

1. in RSA_new_method(), which is used to create new instances of the
   RSA structure, there's this line:

    ret->flags = ret->meth->flags & ~RSA_FLAG_NON_FIPS_ALLOW;

   So that makes the check of rsa->flags valid, no need to go via
   rsa->meth

2. In rsa.h (crypto/rsa/rsa.h in 1.0.2), you'll find this comment in
   the middle of the definition of rsa_meth_st

    /*
     * New sign and verify functions: some libraries don't allow arbitrary
     * data to be signed/verified: this allows them to be used. Note: for
     * this to work the RSA_public_decrypt() and RSA_private_encrypt() should
     * *NOT* be used RSA_sign(), RSA_verify() should be used instead. Note:
     * for backwards compatibility this functionality is only enabled if the
     * RSA_FLAG_SIGN_VER option is set in 'flags'.
     */

   Do note the note on backward compatibility...  you see, there were
   versions of OpenSSL where the fields 'rsa_sign' and 'rsa_verify'
   didn't exist (they appeared in OpenSSL 0.9.5), so for the sake of
   allowing older applications to work with the newer OpenSSL without
   recompilation, we required all new RSA method implementations to
   use that flag to have the 'rsa_sign' and 'rsa_verify' functions
   used.  Without that flag, those functions were assumed not to
   exist, that the RSA method structure was pre-0.9.5.  However, this
   was somewhere in 2000.

   Fast forward to 2015, when we were starting to make certain types
   opaque, and someone noticed this flag still hanging around, and we
   figured that 0.9.5 is long gone, and 1.0.1 was a year away from its
   end of life, and we figured that the reason to have this flag at
   all was a matter of years long past, it was time to simply drop its
   use.  It had grown to become irrelevant a long time ago.

Cheers,
Richard

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users