Skip to content

Validate CN if CURLOPT_SSL_VERIFYHOST is set to true#221

Merged
php-pulls merged 1 commit intophp:masterfrom
johnj:JJ_CURL_SSL_MASTER
Oct 25, 2012
Merged

Validate CN if CURLOPT_SSL_VERIFYHOST is set to true#221
php-pulls merged 1 commit intophp:masterfrom
johnj:JJ_CURL_SSL_MASTER

Conversation

@johnj
Copy link
Contributor

@johnj johnj commented Oct 25, 2012

Per https://crypto.stanford.edu/~dabo/pubs/abstracts/ssl-client-bugs.html - if CURLOPT_SSL_VERIFYHOST is set to "true", convert_to_long_ex sets the LVAL to 1.

libcurl handles the CURLOPT_SSL_VERIFYHOST=1 differently than the default value, 2. CURLOPT_SSL_VERIFYHOST=1 will not check the common name (essentially allowing a MITM attack). A long value of 2 will validate the common name.

http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTSSLVERIFYHOST

I realize this may have impact to code previously setting CURLOPT_SSL_VERIFYHOST to "true" and not worrying about the CN validation - which may now break because libcurl will execute the CN validation. Don't think this is a particularly negative thing - presuming peoples' code had it set to "true" to begin with for peer + host validation.

[amended] - after discussion on internals, setting CURLOPT_SSL_VERIFYHOST should trigger a warning but the lval should remain 1.

[amended] Stas recommends this should be a notice and not a warning, makes sense to me.

@johnj
Copy link
Contributor Author

johnj commented Oct 25, 2012

Stas - I saw your comment (not sure where it went now), I've changed it to a notice instead of a warning.

@ircmaxell
Copy link
Contributor

ircmaxell commented Oct 25, 2012

Please note that this will cause errors that are not intended. The CURLOPT_SSL_VERIFYHOST option needs to be moved to the top of the list. The check will currently run for CURLOPT_SSL_VERSION as well (etc).

Additionally, a test for the new functionality will be needed. I've whipped up a new test for it. It should be implemented at bug63363.phpt under CURL. Here's the body:

--TEST--
Bug #63363 (CURL silently accepts boolean value for SSL_VERIFYHOST)
--SKIPIF--
<?php
if (!extension_loaded("curl")) {
        exit("skip curl extension not loaded");
}

?>
--FILE--
<?php
$ch = curl_init();
var_dump(curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false));
/* Case that should throw an error */
var_dump(curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, true));
var_dump(curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 0));
var_dump(curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 1));
var_dump(curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2));

curl_close($ch);
?>
--EXPECTF--
bool(true)

Notice: curl_setopt(): CURLOPT_SSL_VERIFYHOST should not be set to a boolean true, use 2 instead in %s on line %d
bool(true)
bool(true)
bool(true)
bool(true)

@johnj
Copy link
Contributor Author

johnj commented Oct 25, 2012

Updated in PR - thx.

@php-pulls php-pulls merged commit 3b85d09 into php:master Oct 25, 2012
@dunglas dunglas mentioned this pull request Sep 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@johnj @ircmaxell @php-pulls