WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#16904 closed defect (bug) (fixed)

CURLOPT_SSL_VERIFYHOST Should use 2 instead of true

Reported by: sivel Owned by:
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.1
Component: HTTP API Keywords: has-patch 3.2-early
Focuses: Cc:

Description

CURLOPT_SSL_VERIFYHOST Should use an integer of 2 instead of true to enforce proper SSL verification. Setting to true acts like 1, which only checks for the existence of a common name, and not that the common name matches the requested hostname.

This was reported in another ticket #16855

Attachments (3)

16904.diff (668 bytes) - added by sivel 4 years ago.
16904.patch (713 bytes) - added by hakre 4 years ago.
Found by cogmios
16904.2.patch (709 bytes) - added by hakre 4 years ago.
Make setting more visible.

Download all attachments as: .zip

Change History (9)

@sivel4 years ago

@hakre4 years ago

Found by cogmios

comment:1 follow-up: @hakre4 years ago

Related Comment

No need to be that strict on the evaluation of $ssl_verify btw.

@hakre4 years ago

Make setting more visible.

comment:2 @hakre4 years ago

Hmm, I just ran over the fact that any (bool) is infact an (int), if it's TRUE it is (int) 1, if it is false, it is (int) 0.

So in case we don't change the current implementation, users of the cUrl transport are able to additionally specify 2 here, which is also (bool) TRUE for the second setting. Same for 1, it's (bool) TRUE for the second setting as well.

The unpatched code:

   curl_setopt( $handle, CURLOPT_SSL_VERIFYHOST, $ssl_verify ); 
   curl_setopt( $handle, CURLOPT_SSL_VERIFYPEER, $ssl_verify );

Forcing $ssl_verify to result in either 2 or 0 for CURLOPT_SSL_VERIFYHOST will remove the possibility to set the value to 1.

comment:3 @hakre4 years ago

Hmm, and then there is the Streams context:

'ssl' => array(
		'verify_peer' => $ssl_verify,
		'verify_host' => $ssl_verify
)

I can not find the verify_host documented.

It's probably worth to list parameters and their current meaning.

With such a list we could more easily say which feature should be supported on the abstracted transports interface.

comment:4 in reply to: ↑ 1 @sivel4 years ago

Replying to hakre:

Related Comment

No need to be that strict on the evaluation of $ssl_verify btw.

In general we do try to be that strict with our checks in the HTTP API. We want to expect very specific values.

comment:5 @sivel4 years ago

  • Milestone changed from Future Release to 3.2

comment:6 @dd324 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [17616]) Verify certificate matches both the common name and the provided hostname. WP_HTTP_Curl requires CURLOPT_SSL_VERIFYHOST be either 2 or false. Props sivel. Fixes #16904

Note: See TracTickets for help on using tickets.