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

SSL connection not working (with fix) #252

Closed
biospb opened this issue Mar 4, 2020 · 9 comments
Closed

SSL connection not working (with fix) #252

biospb opened this issue Mar 4, 2020 · 9 comments

Comments

@biospb
Copy link

biospb commented Mar 4, 2020

Due to bug in code curl is always initialized without SSL

BUGGED:

flags = !opt_benchmark
       && ( strncmp( rpc_url, "https:", 6 )
           || strncasecmp(rpc_url, "stratum+tcps://", 15 ) )
        ? ( CURL_GLOBAL_ALL & ~CURL_GLOBAL_SSL )
        : CURL_GLOBAL_ALL;

FIXED:

if (opt_benchmark)
	flags = CURL_GLOBAL_NOTHING;
else if ( strncmp(rpc_url, "https:", 6) && strncasecmp(rpc_url, "stratum+tcps://", 15) )
	flags = CURL_GLOBAL_ALL & ~CURL_GLOBAL_SSL;
else
	flags = CURL_GLOBAL_ALL;

if ( curl_global_init( flags ) )
{
	applog(LOG_ERR, "CURL initialization failed");
	return 1;
}
@JayDDee
Copy link
Owner

JayDDee commented Mar 4, 2020

Good catch, an always true trap,

Would it not be better to protect the curl_global_init call from opt_benchmark?

if ( !opt_benchmark )
{
if ( strncmp(rpc_url, "https:", 6) && strncasecmp(rpc_url, "stratum+tcps://", 15) )
flags = CURL_GLOBAL_ALL & ~CURL_GLOBAL_SSL;
else
flags = CURL_GLOBAL_ALL;
if ( curl_global_init( flags ) )
{
applog(LOG_ERR, "CURL initialization failed");
return 1;
}
}

I will probably change strncasecmp to strncmp also.

@JayDDee
Copy link
Owner

JayDDee commented Mar 6, 2020

cpuminer-opt-3.12.6 is release with a fix for this issue. It corrects the logic and uses strncasecmp
for both terms. Benchmark was not changed, as you suggested, to use CURL_GLOBAL_NOTHING.

The fix is missing from the release notes.

Please test and post results.

Also, if you cold, please explain the impact of the bug. stratum+tcps was recently tested and
reported to be working. Did it connect without ssl? Were there any errors or warnings?
How did you discover the bug?

If the miner silently fell back to an isecure connection without warning when the user explicitly
requested a secure connection it would be critical security issue in most cases. For crypto
mining is isn't such a big deal. Regardless of the type of application it should be fixed. I will look for
a way to either, error out if a secure connection can't be made, or, display a warning when falling
back.

@JayDDee
Copy link
Owner

JayDDee commented Mar 7, 2020

There is a problem with v3.12.6, it broke benchmark.

Another fix will be in the next release.

@biospb
Copy link
Author

biospb commented Mar 7, 2020

I was testing one algo on Rplant pool which supports SSL connection and another cpuminers including their fork worked fine, while your versions gave this error:

Stratum connection failed: SSL: couldn't create a context: error:140A90A1:lib(20):func(169):reason(161)

It was related to libcurl and CURL_GLOBAL_SSL.
This may depend on libcurl version and target system. Mine was Ubuntu 16 LTS.

@JayDDee
Copy link
Owner

JayDDee commented Mar 7, 2020

Thanks for testing. It looks like the fix made things worse (#254).

Try this code change, I just tested it with benchmark and SSL.

cpu-miner.c:3574

old

flags = !opt_benchmark
|| ( strncasecmp( rpc_url, "https:", 6 )
&& strncasecmp( rpc_url, "stratum+tcps://", 15 ) )
? ( CURL_GLOBAL_ALL & ~CURL_GLOBAL_SSL )
: CURL_GLOBAL_ALL;

new

flags = CURL_GLOBAL_ALL;
if ( !opt_benchmark )
if ( strncasecmp( rpc_url, "https:", 6 )
&& strncasecmp( rpc_url, "stratum+tcps://", 15 ) )
flags &= ~CURL_GLOBAL_SSL;

@rplant8
Copy link

rplant8 commented Mar 7, 2020

maybe !strncasecmp( rpc_url, "https:", 6 ) || !strncasecmp( rpc_url, "stratum+tcps://", 15 )
if one zero then ssl

@JayDDee
Copy link
Owner

JayDDee commented Mar 7, 2020

Thanks @rplant8.

I was trying to be too clever with the boolean algebra, was distracted by other issues and didn't
test properly.

The above change has been tested with standard stratum, SSL and benchmark.

@JayDDee
Copy link
Owner

JayDDee commented Mar 7, 2020

cpuminer-opt-3.12.6.1 is released with the fix. It should work now, please let me know otherwise.

@JayDDee
Copy link
Owner

JayDDee commented Mar 10, 2020

No problems reported, closing.

@JayDDee JayDDee closed this as completed Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants