Re: [racket-dev] [patch] OpenSSL ECDH(E) + DHE support.

2014-02-10 Thread Edward Lee
Full disclosure:  I am not an OpenSSL expert and this patch is still
a work in progress.  I will hopefully have a more finalized patch
closer to next week, but I am not qualified to audit this code.

On Sun, Feb 09, 2014 at 04:40:44PM -0500, Neil Van Dyke wrote:
 Edward Lee wrote at 02/08/2014 05:52 PM:
 [...]
 Racket's OpenSSL bindings do not currently enable the ECDH(E) and the
 DHE ciphers, which are needed for perfect forward secrecy.
 
 I've attached a patch that:
 [...]
 
 First, thanks for taking the initiative and contributing.  Second, I
 feel a little embarrassed saying this, since I don't have time to
 volunteer myself right now, but I think the following is
 important...
 
 I suggest that any contributions touching SSL in the core need
 careful auditing by someone who understands the mechanics fully.  If
 Edward is an expert on this aspect of OpenSSL, his audit would be
 fine, but otherwise someone else has to audit it.
 
 To emphasize: The SSL infrastructure and OpenSSL are both hairy, and
 a small mistake can defeat SSL altogether without being detected for
 years.  This has actually happened before, notably with SSL traffic
 on Debian and Ubuntu systems being compromised for over a year by a
 one-line mistake 
 (https://www.schneier.com/blog/archives/2008/05/random_number_b.html;).
 From a Racket extension perspective, in addition to possible logic
 errors of API usage, there's also possible C memory errors through
 the FFI.
 
 Neil V.
_
  Racket Developers list:
  http://lists.racket-lang.org/dev


Re: [racket-dev] [patch] OpenSSL ECDH(E) + DHE support.

2014-02-10 Thread Ryan Culpepper

On 02/08/2014 05:52 PM, Edward Lee wrote:

Hi.

Racket's OpenSSL bindings do not currently enable the ECDH(E) and the
DHE ciphers, which are needed for perfect forward secrecy.

I've attached a patch that:
   - Embeds reasonable defaults for DHE mode.
   - Adds two functions, ssl-server-context-enable-dhe! and
 ssl-server-context-enable-ecdhe! that when given
 DHE/ECDHE setup arguments (for DHE, a DH parameter file path,
 for ECDHE, the name of one of the built-in OpenSSL elliptic curves
 [currently, only secp521r1])
   - (unrelated, but also useful) Adds bindings for TLS 1.1/1.2-only
 server/client contexts.

This patch is currently a work in progress (it currently only supports
one elliptic curve name) that works well enough for what I am using it
for, but I'm interested in getting this patch upstream.


Here are a few comments:

What is SSL_CTRL_SET_ECDH_AUTO? I couldn't find it in the openssl 
headers, and I searched a few recent versions. It seems unused here, anyway.


I think it would be better to read the DH params file into memory using 
Racket file operations and then use a memory BIO for 
PEM_read_bio_DHparams. Two reasons: the file access will then 
automatically go through Racket's security-guard checks, and the file 
read will go through Racket's IO system, so if the read blocks it won't 
block all Racket threads.


In ...enable-ecdhe!, the 'case' form doesn't need 'quote'. It should be 
this instead:


  (case name
[(secp521r1) NID_secp521r1]
[else ___])

As it is, the function will accept 'quote as the name of a curve.


What's the exact process for this?


If you have a github account, you can fork the repo there and submit a 
pull request. But we can certainly handle patches like this too.


Ryan

_
 Racket Developers list:
 http://lists.racket-lang.org/dev


Re: [racket-dev] [patch] OpenSSL ECDH(E) + DHE support.

2014-02-10 Thread Edward Lee
On Mon, Feb 10, 2014 at 06:37:56PM -0500, Ryan Culpepper wrote:
 On 02/08/2014 05:52 PM, Edward Lee wrote:
 Hi.
 
 Racket's OpenSSL bindings do not currently enable the ECDH(E) and the
 DHE ciphers, which are needed for perfect forward secrecy.
 
 I've attached a patch that:
- Embeds reasonable defaults for DHE mode.
- Adds two functions, ssl-server-context-enable-dhe! and
  ssl-server-context-enable-ecdhe! that when given
  DHE/ECDHE setup arguments (for DHE, a DH parameter file path,
  for ECDHE, the name of one of the built-in OpenSSL elliptic curves
  [currently, only secp521r1])
- (unrelated, but also useful) Adds bindings for TLS 1.1/1.2-only
  server/client contexts.
 
 This patch is currently a work in progress (it currently only supports
 one elliptic curve name) that works well enough for what I am using it
 for, but I'm interested in getting this patch upstream.
 
 Here are a few comments:
 
 What is SSL_CTRL_SET_ECDH_AUTO? I couldn't find it in the openssl
 headers, and I searched a few recent versions. It seems unused here,
 anyway.
It's not defined in OpenSSL 1.0.1, I think.  It can be used for enabling
ECDH(E) in OpenSSL = 1.0.2, with OpenSSL automatically selecting the
curve - https://www.openssl.org/news/changelog.html, Support for
automatic EC temporary key parameter selection...
 
 I think it would be better to read the DH params file into memory
 using Racket file operations and then use a memory BIO for
 PEM_read_bio_DHparams. Two reasons: the file access will then
 automatically go through Racket's security-guard checks, and the
 file read will go through Racket's IO system, so if the read blocks
 it won't block all Racket threads.
I will investigate this, and time permitting, will change my patch to
use Racket's IO system for loading the parameters.
 
 In ...enable-ecdhe!, the 'case' form doesn't need 'quote'. It should
 be this instead:
 
   (case name
 [(secp521r1) NID_secp521r1]
 [else ___])
 
 As it is, the function will accept 'quote as the name of a curve.
Thanks for pointing this out.
 
 What's the exact process for this?
 
 If you have a github account, you can fork the repo there and submit
 a pull request. But we can certainly handle patches like this too.
 
 Ryan
Great, thanks.

--Edward
_
  Racket Developers list:
  http://lists.racket-lang.org/dev