On Thu, Apr 25, 2013 at 3:55 AM, Daniel Stenberg <daniel_at_haxx.se> wrote:

> On Tue, 23 Apr 2013, Jerry Qassar wrote:
>
> easy: Increment engine reference in curl_easy_duphandle
>>
>> When external programs (such as git) try to set the SSL engine, they set
>> the engine in the default handle but subsequently (if using multi) obtain a
>> duplicate handle to do the actual work.
>>
>> curl_easy_duphandle did not do anything with state.engine if set; make it
>> do so by getting the engine ID of the source handle and incrementing the
>> reference count with another curl_ssl_set_engine call.
>>
>> To my limited knowledge this is the 'proper' way to handle additional
>> handles needing a non-default engine. Please advise if otherwise; handling
>> of the default engine flag across handles is not attempted.
>>
>
> I'm not aware of anyone usually hanging around here that is an expert on
> this subject so your ideas here are just as good as us others'. If the
> documentation and testing say this works, then it seems like a good idea.
>
> But I would like to ask you to make a full patch and send it separately
> from the config file load patch, since they are actually independent.
>
>
Mr. Stenberg,

Thank you very much for looking into this for me. I'll break them up and
resubmit the duphandle one again. As for the ssluse...

>
> ssluse: Add Petr Pisar's patch to read OpenSSL conf file
>>
>> In 2010 Petr Pisar supplied a patch to allow curl to parse OpenSSL
>> configuration files (either default or env-specified), enabling the use of
>> dynamic engines such as those used for smartcard support. Original
>> discussion of the patch terminated here:
>>
>
> First, the discussion was paused there (http://curl.haxx.se/mail/**
> archive-2010-03/0037.html<http://curl.haxx.se/mail/archive-2010-03/0037.html>)
> since nobody responded to Yang's fine comments as far as I can see. I think
> they still deserve getting addressed. For example, don't we risk hurting
> existing users/applications by suddenly doing this by default, or the other
> way: do we need a way to allow applications to switch this ability off?
>
> Secondly, the loading of config files for OpenSSL seems to be required for
> proper ENGINE use, but is somewhat problematic and we already have an open
> bug report about it that hasn't been resolved yet:
> http://sourceforge.net/p/curl/**bugs/1208/<http://sourceforge.net/p/curl/bugs/1208/>
>
> Related to the second point, docs/INTERNALS says we maintain compatibility
> with OpenSSL 0.9.6. We either do that and thus make sure we use later
> functions (as as these config file loading ones) conditionally, or we
> update the document...
>
>
Well, I'm definitely up for figuring out a safe way to make it happen. I
don't do *anything* with the PHP side of things, so I'm not sure I'm the
one to address it successfully.

As I see it, your concerns right now are:

- Making libcurl load *any* sort of OpenSSL config file (whether engine'd
or not) could change the behavior of existing libcurl-using applications.
- Malicious users could define a naughty engine in their own configuration
file, set OPENSSL_CONF, and cackle.

Right now, all the examples we (speaking broadly here about OpenSC users)
use to enable smartcards use the same OpenSSL configuration file snippet to
define the PKCS#11 engine dynamically, and that uses a section called
openssl_conf to set the engine properties. It looks like this:

---
openssl_conf  = openssl_def (at the top of the file)
 ...
[openssl_def]
engines = engine_section
[engine_section]
pkcs11 = pkcs11_section
[pkcs11_section]
....
---
openssl_conf is the default appname used in OpenSSL CONF_* calls, but that
section doesn't actually exist in the standard distribution; the standard
openssl.cnf file provided with Unix distributions sets parameters related
to certificate signing and certificate requests, not engine definitions or
connection methods.   Our patches are working because we use
CONF_modules_load_file(blah, NULL, 0) and OPENSSL_config(NULL),
respectively, and putting NULL in those arguments defaults them to use
openssl_conf per the documentation.  But we don't have to use
openssl_conf.  We can use another application/profile name, and that won't
break anything.  :)  I've just tested with libcurl_conf as an appname and
it works fine, for example.
OpenSSL itself respects OPENSSL_CONF since at least 0.9.6, though, so
anyone with the ability to add the environment variable can interfere with
both libcurl and OpenSSL (in my mind, you have equal problems at that
point.)  If that is still too perilous, we can try one of a couple of
approaches.
1) I would be remiss if I didn't point this out in the OPENSSL_config()
docs:
"It is also possible to automatically call OPENSSL_config() when an
application calls OPENSSL_add_all_algorithms() by compiling an application
with the preprocessor symbol OPENSSL_LOAD_CONF #define'd. In this way
configuration can be added without source changes. "
So, actually, we can get a default call for free to the system openssl.cnf
for 0.9.7 and up, no patch necessary.  (Oops...)  You would still have to
insert your engine definition into the openssl_conf definition.
Is this the best solution?  It is the most minimally invasive one, at
least.  Making it work for 0.9.6 requires an actual patch (the first
version of Mr. Pisar's), but that version doesn't even support engines
unless using the specific 'engine' release.  I don't really intuit a long
line of 0.9.6 users who *also* use dynamic engines who *also* would rather
stay on the experimental initial version than get all the bugs fixed in
0.9.7+ who *also* need libcurl.
2) As noted, it seems nicer to have a libcurl-specific section of the
OpenSSL config, either added to the default one or some non-standard config
file.  Even so, though, you can't unilaterally decide that there will be
one libcurl_def section to be used by all libcurl-using applications by
default.  So maybe we enable some parameters and backend functions
with terrible, terrible names:
 o --ssl-conf-file <path/to/openssl.cnf> = Specifies the OpenSSL
configuration file to read.
   o If not defined, OpenSSL configuration files are not read.
   o If defined without a parameter, reads the system OpenSSL config, and
respects OPENSSL_CONF.
   o If defined with a parameter, reads the specified OpenSSL configuration
file.  If not found, an error is returned _without_ attempting to read the
system OpenSSL config or OPENSSL_CONF.
 o --ssl-conf-section <string> = Specifies the section of the OpenSSL
configuration file to read.
   o Implies --ssl-conf-file with no parameters supplied, reading within
the system OpenSSL config and respecting OPENSSL_CONF.
   o If --ssl-conf-file is defined with a parameter, reads the specified
section from the specified config file.  If the section is not found, no
error occurs.  If the file is not found, error out.
...I think my patch just got bigger.  :/  Then again, I am perfectly fine
with 1).  Any suggestions?
Regards,
--Jerry

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-04-26