Hey all - I'd like start a discussion around pull request 221
(https://github.com/php/php-src/pull/221).
In short, there's a high volume of [incorrect] code out there which looks like:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, true);
Instead of what, in all likelyhood, the code meant to do:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
This is due to the convert_to_long_ex call which converts "true" to
1L. CURLOPT_SSL_VERIFYHOST being set to 1L bypasses common name
validation within libcurl.
My solution was to check the type for CURLOPT_SSL_VERIFYHOST: if it is
boolean and true, the opt value for libcurl is set to 2L.
I understand that engineers should have the proper option value to
begin with but weighing the impact of this (MITM attacks) against
doing what they probably meant anyways is worth the presumption.
Please discuss and adjust the patch if necessary.
- JJ
Hey all - I'd like start a discussion around pull request 221
(https://github.com/php/php-src/pull/221).In short, there's a high volume of [incorrect] code out there which looks like:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, true);
Instead of what, in all likelyhood, the code meant to do:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
This is due to the convert_to_long_ex call which converts "true" to
1L.CURLOPT_SSL_VERIFYHOSTbeing set to 1L bypasses common name
validation within libcurl.My solution was to check the type for CURLOPT_SSL_VERIFYHOST: if it is
boolean and true, the opt value for libcurl is set to 2L.I understand that engineers should have the proper option value to
begin with but weighing the impact of this (MITM attacks) against
doing what they probably meant anyways is worth the presumption.Please discuss and adjust the patch if necessary.
- JJ
--
While I think it's a good idea to set the value of the option to 2, as
is recommended for production in the documentation, I think the idea
of implicitly converting a bool(true) to 2L internally might lead to
unexpected behavior since some people might actually depend on normal
PHP behavior to cast a bool(true) to 1 (and that might be what they
actually intended).
I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.
We should probably just elaborate on this point a bit more in the
documentation. Perhaps add a note and an example to illustrate. I
notice that people tend to pay more attention to examples than
anything else in the docs.
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:
I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.
I highly doubt code that sets CURLOPT_SSL_VERIFYHOST => true meant to
imply CURLOPT_SSL_VERIFYHOST => 1...which essentially bypasses host
verification.
According to libcurl, CURLOPT_SSL_VERIFYHOST => 1 is "not ordinarily a
useful setting".
- JJ
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.
They may have, even in spite of it being a bad idea, since that's how
boolean → integer conversion works in PHP. I don't think we can assume
that every single person who's written that line of code didn't check
whether CURLOPT_SSL_VERIFYHOST was a boolean or integer option.
According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".
I agree, but I don't think we can start arbitrarily changing well
defined type conversion behaviour for one corner case. The
CURLOPT_SSL_VERIFYHOST option has been documented as expecting integer
0, 1 or 2 since at least April 2002 (and probably quite a bit earlier
than that), complete with the meanings of each value — there's only so
much we can do to protect developers from themselves. (In fairness,
the wording strongly recommending using option 2 only came in last
August thanks to Ilia, but nobody should have been treating the option
as a boolean option to start with.)
Fundamentally, it's a bad API on the part of curl, but that's a
separate issue. There's nothing stopping somebody proposing a saner
API on top of libcurl (as Anthony did recently with the password
hashing API atop crypt()).
In summary, I'm against changing ext/curl here.
I do have a couple of specific comments on elements of the patch
itself in the event that the changed behaviour is wanted, but I'll
post those on GitHub, since it's probably a better UI for that sort of
granular discussion.
Adam
CURLOPT_SSL_VERIFYHOST - this option just sounds like "should I verify
host?", when it must sound like "what verifying strategy should I use?"
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.They may have, even in spite of it being a bad idea, since that's how
boolean → integer conversion works in PHP. I don't think we can assume
that every single person who's written that line of code didn't check
whetherCURLOPT_SSL_VERIFYHOSTwas a boolean or integer option.According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".I agree, but I don't think we can start arbitrarily changing well
defined type conversion behaviour for one corner case. The
CURLOPT_SSL_VERIFYHOSToption has been documented as expecting integer
0, 1 or 2 since at least April 2002 (and probably quite a bit earlier
than that), complete with the meanings of each value — there's only so
much we can do to protect developers from themselves. (In fairness,
the wording strongly recommending using option 2 only came in last
August thanks to Ilia, but nobody should have been treating the option
as a boolean option to start with.)Fundamentally, it's a bad API on the part of curl, but that's a
separate issue. There's nothing stopping somebody proposing a saner
API on top of libcurl (as Anthony did recently with the password
hashing API atopcrypt()).In summary, I'm against changing ext/curl here.
I do have a couple of specific comments on elements of the patch
itself in the event that the changed behaviour is wanted, but I'll
post those on GitHub, since it's probably a better UI for that sort of
granular discussion.Adam
I completely agree with Adam and others, we should not change the
behaviour to add any magic.
The ext/curl api was made to stay as close as possible from the
original libcurl api and it should stay the same (even if it's not
always implicit). A lot of people are often referring to the libcurl C
documentation to get more informations. Since it's supposed to be
almost a 1 to 1 binding everything (even bad use of
CURLOPT_SSL_VERIFYHOST with boolean) should work as expected without
any magic.
Pierrick
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.They may have, even in spite of it being a bad idea, since that's how
boolean → integer conversion works in PHP. I don't think we can assume
that every single person who's written that line of code didn't check
whetherCURLOPT_SSL_VERIFYHOSTwas a boolean or integer option.According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".I agree, but I don't think we can start arbitrarily changing well
defined type conversion behaviour for one corner case. The
CURLOPT_SSL_VERIFYHOSToption has been documented as expecting integer
0, 1 or 2 since at least April 2002 (and probably quite a bit earlier
than that), complete with the meanings of each value — there's only so
much we can do to protect developers from themselves. (In fairness,
the wording strongly recommending using option 2 only came in last
August thanks to Ilia, but nobody should have been treating the option
as a boolean option to start with.)Fundamentally, it's a bad API on the part of curl, but that's a
separate issue. There's nothing stopping somebody proposing a saner
API on top of libcurl (as Anthony did recently with the password
hashing API atopcrypt()).In summary, I'm against changing ext/curl here.
I do have a couple of specific comments on elements of the patch
itself in the event that the changed behaviour is wanted, but I'll
post those on GitHub, since it's probably a better UI for that sort of
granular discussion.Adam
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.
That's not our place to start guessing what the user did or did not
intend. The fact remains that a cast of a boolean true to int is in
fact 1. The fact also remains that CURLOPT_SSL_VERIFYHOST expects an
int as per the documented behavior. The fact additionally remains that
no user would expect var_dump((int) true) to return int(2).
According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".
Again, you're confusing users who don't read documentation and/or
don't understand it with users who may have fully read documentation
and understood it perfectly well and ever intention of setting
CURLOPT_SSL_VERIFYHOST to 1.
I understand your intentions here are good, but we should not be
magically trying to guess what the user wants. We should document the
behavior clearly and this solution makes documentation completely
unclear. Nowhere in the PHP manual do we say "we might break the
promise of a boolean cast to int and make it int(2) instead of
int(1)".
- JJ
On Wed, Oct 24, 2012 at 11:21 PM, Sherif Ramadan theanomaly.is@gmail.comwrote:
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.That's not our place to start guessing what the user did or did not
intend. The fact remains that a cast of a boolean true to int is in
fact 1. The fact also remains thatCURLOPT_SSL_VERIFYHOSTexpects an
int as per the documented behavior. The fact additionally remains that
no user would expect var_dump((int) true) to return int(2).According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".Again, you're confusing users who don't read documentation and/or
don't understand it with users who may have fully read documentation
and understood it perfectly well and ever intention of setting
CURLOPT_SSL_VERIFYHOSTto 1.I understand your intentions here are good, but we should not be
magically trying to guess what the user wants. We should document the
behavior clearly and this solution makes documentation completely
unclear. Nowhere in the PHP manual do we say "we might break the
promise of a boolean cast to int and make it int(2) instead of
int(1)".
- JJ
--
Correct me if I'm wrong, but 2 is already the default value, right? I've
also seen plenty of cases where TRUE is mistakenly used instead of 2. I
would also agree that this behavior is somewhat counter-intuitive.
However, I think we'd be opening a can of worms if we started making
arbitrary tweaks to change the documented behavior of third-party
libraries. I think you do make a very good case for changing this
behavior, but that case would (in my opinion, at least) need to be made to
the folks over at libcurl, not here.
That said, I can't think of any rational use-case for passing TRUE instead
of 1. Every time I've seen this, the intended behavior was 2.
What if, instead of changing the behavior, we have it throw a notice or
warning if a boolean value is passed here? Because this is such a common
error, I think it could be really beneficial in helping developers catch
this early. Thoughts?
--Kris
What if, instead of changing the behavior, we have it throw a notice or
warning if a boolean value is passed here? Because this is such a common
error, I think it could be really beneficial in helping developers catch
this early. Thoughts?
Yes please. My thoughts exactly. A notice or a warning really would be best
in this case.
Regards,
All,
On Thu, Oct 25, 2012 at 3:14 AM, Guillaume Rossolini
g.rossolini@gmail.comwrote:
What if, instead of changing the behavior, we have it throw a notice or
warning if a boolean value is passed here? Because this is such a common
error, I think it could be really beneficial in helping developers catch
this early. Thoughts?Yes please. My thoughts exactly. A notice or a warning really would be best
in this case.
Regards,
I completely agree here.
Right now, the current behavior LOOKS like it should verify the peer when
given a boolean. But in reality it doesn't. The boolean triggers insecure
behavior. Which is exactly the opposite of VERIFY_PEER (which accepts a
boolean parameter).
Since this case does have fairly significant security implications, I would
suggest raising either a notice or a warning when a boolean true is passed
in.
This should be pretty easy to do, by simply altering
http://lxr.php.net/xref/PHP_5_4/ext/curl/interface.c#1681 to put VERIFYHOST
as the first case statement, and checking if Z_TYPE_PP(zvalue) == IS_BOOL,
and then handling that case (letting it fall through afterwards)...
I would strongly support this. As using boolean true and expecting the cast
to go to 1 would be... well... rather an edge case. I'd believe that most
people don't realize that it takes an integer parameter. And if you're
relying on a cast, well, you're doing it wrong. It's like the people
relying on the string cast from an array producing "array". Sure, it may
cause a few minor issues for a few users. But the vast majority of the red
flags that it raises will be completely valid. And considering this has
security impact, I strongly feel that we should implement the
notice/warning...
Anthony
On Thu, Oct 25, 2012 at 7:26 AM, Anthony Ferrara ircmaxell@gmail.comwrote:
All,
On Thu, Oct 25, 2012 at 3:14 AM, Guillaume Rossolini <
g.rossolini@gmail.com> wrote:What if, instead of changing the behavior, we have it throw a notice or
warning if a boolean value is passed here? Because this is such a
common
error, I think it could be really beneficial in helping developers catch
this early. Thoughts?Yes please. My thoughts exactly. A notice or a warning really would be
best
in this case.
Regards,I completely agree here.
Right now, the current behavior LOOKS like it should verify the peer when
given a boolean. But in reality it doesn't. The boolean triggers insecure
behavior. Which is exactly the opposite of VERIFY_PEER (which accepts a
boolean parameter).Since this case does have fairly significant security implications, I
would suggest raising either a notice or a warning when a boolean true is
passed in.This should be pretty easy to do, by simply altering
http://lxr.php.net/xref/PHP_5_4/ext/curl/interface.c#1681 to put
VERIFYHOST as the first case statement, and checking if Z_TYPE_PP(zvalue)
== IS_BOOL, and then handling that case (letting it fall through
afterwards)...I would strongly support this. As using boolean true and expecting the
cast to go to 1 would be... well... rather an edge case. I'd believe that
most people don't realize that it takes an integer parameter. And if you're
relying on a cast, well, you're doing it wrong. It's like the people
relying on the string cast from an array producing "array". Sure, it may
cause a few minor issues for a few users. But the vast majority of the red
flags that it raises will be completely valid. And considering this has
security impact, I strongly feel that we should implement the
notice/warning...Anthony
If there are no objections, I'll go ahead and draft an RFC for the
notice/docs idea later today. If anyone would like to co-author it with
me, please drop me an email and I'll send you the wiki link once I've
created the page.
--Kris
If there are no objections, I'll go ahead and draft an RFC for the
notice/docs idea later today. If anyone would like to co-author it with
me, please drop me an email and I'll send you the wiki link once I've
created the page.
Do we even need an RFC for this? I would personally just open a bug and
commit the change.
But if people want an RFC, that's fine... It just seems like a huge
undertaking for such a minor change...
Anthony
If there are no objections, I'll go ahead and draft an RFC for the
notice/docs idea later today. If anyone would like to co-author it with
me, please drop me an email and I'll send you the wiki link once I've
created the page.Do we even need an RFC for this? I would personally just open a bug and
commit the change.But if people want an RFC, that's fine... It just seems like a huge
undertaking for such a minor change...
I see no need for an RFC just to add a helpful notice here. Just do it.
-Rasmus
Do we even need an RFC for this? I would personally just open a bug and
commit the change.
But if people want an RFC, that's fine... It just seems like a huge
undertaking for such a minor change...I see no need for an RFC just to add a helpful notice here. Just do it.
Thanks Rasmus,
I'll open a bug and commit the fix today.
Anthony
FYI: the bug is: 63363
https://bugs.php.net/bug.php?id=63363
On Thu, Oct 25, 2012 at 12:06 PM, Anthony Ferrara ircmaxell@gmail.comwrote:
Do we even need an RFC for this? I would personally just open a bug and
commit the change.
But if people want an RFC, that's fine... It just seems like a huge
undertaking for such a minor change...I see no need for an RFC just to add a helpful notice here. Just do it.
Thanks Rasmus,
I'll open a bug and commit the fix today.
Anthony
Rasmus et al,
I see no need for an RFC just to add a helpful notice here. Just do it.
-Rasmus
Should this target master only (5.5)? Or 5.3/5.4 as well?
My tendancy would be to target 5.5 only, but I can see the argument made
that it should target 5.3/5.4 as well... Thoughts?
Anthony
Hi!
My tendancy would be to target 5.5 only, but I can see the argument made
that it should target 5.3/5.4 as well... Thoughts?
I think it'd be fine for 5.4. Please do a pull req first though to be sure.
Thanks,
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Agreed.
https://github.com/johnj/php-src/commit/905f7121fa664380c97f71ff9cbc4b6c04396374
- JJ
I see no need for an RFC just to add a helpful notice here. Just do it.
-Rasmus
Stas suggested this should throw a notice instead of a warning, I've
amended. Thx all.
https://github.com/php/php-src/pull/221
- JJ
Agreed.
https://github.com/johnj/php-src/commit/905f7121fa664380c97f71ff9cbc4b6c04396374
- JJ
I see no need for an RFC just to add a helpful notice here. Just do it.
-Rasmus
Stas suggested this should throw a notice instead of a warning, I've
amended. Thx all.
The PR has been merged in and closed. It has been merged into 5.4 and
master.
Thanks for the contribution!
Anthony
On Wed, Oct 24, 2012 at 11:21 PM, Sherif Ramadan theanomaly.is@gmail.comwrote:
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.That's not our place to start guessing what the user did or did not
intend. The fact remains that a cast of a boolean true to int is in
fact 1. The fact also remains thatCURLOPT_SSL_VERIFYHOSTexpects an
int as per the documented behavior. The fact additionally remains that
no user would expect var_dump((int) true) to return int(2).According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".Again, you're confusing users who don't read documentation and/or
don't understand it with users who may have fully read documentation
and understood it perfectly well and ever intention of setting
CURLOPT_SSL_VERIFYHOSTto 1.I understand your intentions here are good, but we should not be
magically trying to guess what the user wants. We should document the
behavior clearly and this solution makes documentation completely
unclear. Nowhere in the PHP manual do we say "we might break the
promise of a boolean cast to int and make it int(2) instead of
int(1)".
- JJ
--
Correct me if I'm wrong, but 2 is already the default value, right? I've
also seen plenty of cases whereTRUEis mistakenly used instead of 2. I
would also agree that this behavior is somewhat counter-intuitive.
However, I think we'd be opening a can of worms if we started making
arbitrary tweaks to change the documented behavior of third-party
libraries. I think you do make a very good case for changing this
behavior, but that case would (in my opinion, at least) need to be made to
the folks over at libcurl, not here.That said, I can't think of any rational use-case for passing
TRUEinstead
of 1. Every time I've seen this, the intended behavior was 2.What if, instead of changing the behavior, we have it throw a notice or
warning if a boolean value is passed here? Because this is such a common
error, I think it could be really beneficial in helping developers catch
this early. Thoughts?--Kris
So you propose to implement strict type checking of parameters because a
few bozos don't read the documentation? That doesn't make much sense to me.
What makes more sense is that the extension perform its own type
checking where that is appropriate. I have plenty of subroutine code
that checks what it was passed, it isn't difficult to do the job right
instead of twiddling the language every time somebody complains.
And by the way, is there any effort in-progress to make the basic
subroutine calls more uniform, or are is_array() and isset() and friends
going to forever remain underscore versus non-underscore?
Perhaps I have no clue, but it sounded as though the language was to be
changed to "fix" an error in an extension, 'scuse me if I'm talking out
of my nether orifice.
So you propose to implement strict type checking of parameters because a
few bozos don't read the documentation? That doesn't make much sense to me.What makes more sense is that the extension perform its own type checking
where that is appropriate. I have plenty of subroutine code that checks
what it was passed, it isn't difficult to do the job right instead of
twiddling the language every time somebody complains.And by the way, is there any effort in-progress to make the basic
subroutine calls more uniform, or areis_array()and isset() and friends
going to forever remain underscore versus non-underscore?Perhaps I have no clue, but it sounded as though the language was to be
changed to "fix" an error in an extension, 'scuse me if I'm talking out of
my nether orifice.
Well, at least you're living up to your username, crankypuss. Still,
though, if your goal is to persuade as opposed to simply trolling, you're
probably not going to be very successful by engaging in name-calling and
going on a tangent about the consistency of function names. If you'd like
to ask about function names, please start a separate thread for that.
Attempting to hijack this thread to discuss your unrelated grievance won't
get you very far.
Also, I've seen this error quite a few times. It's not just "a few bozos,"
as you put it. It's an easy error to make even if you're knowledgeable. I
agree that any behavioral changes should happen in libcurl and not PHP, but
that doesn't mean this isn't a problem or that we should just belittle
those who have encountered it and ignore it. Updating the documentation to
address this specific mistake might help reduce its frequency somewhat.
Throwing a warning or notice if a bool is passed would also be helpful in
my opinion.
Let's not troll the OP for suggesting a solution to this issue just because
we don't agree with it. It's just a matter of mutual respect. This isn't
4chan or YouTube, after all. ;)
--Kris
So you propose to implement strict type checking of parameters because a
few bozos don't read the documentation? That doesn't make much sense to me.What makes more sense is that the extension perform its own type checking
where that is appropriate. I have plenty of subroutine code that checks
what it was passed, it isn't difficult to do the job right instead of
twiddling the language every time somebody complains.And by the way, is there any effort in-progress to make the basic
subroutine calls more uniform, or areis_array()and isset() and friends
going to forever remain underscore versus non-underscore?Perhaps I have no clue, but it sounded as though the language was to be
changed to "fix" an error in an extension, 'scuse me if I'm talking out of
my nether orifice.Well, at least you're living up to your username, crankypuss.
Apparently I need to look into gmane settings in order to avoid
receiving direct email replies in addition to seeing replies on the list.
Still,
though, if your goal is to persuade as opposed to simply trolling, you're
probably not going to be very successful by engaging in name-calling
What name-calling? People who pass the wrong data to subroutines are
bozos, plain and simple, by definition; if they actually test their
code, they'll figure out what they've done wrong, thus showing that they
aren't bozos after all. That applies to anybody, me included: if you
write bad code you are a bozo, end of story. It's neither incurable nor
fatal but being nice to bozos for the sake of political correctness is
just silly.
and
going on a tangent about the consistency of function names. If you'd like
to ask about function names, please start a separate thread for that.
Attempting to hijack this thread to discuss your unrelated grievance won't
get you very far.
Let's stick to the issues: don't change the language to make up for a
poorly written extension used by people who don't read the doc and test
their code, is that difficult to grasp?
Also, I've seen this error quite a few times. It's not just "a few bozos,"
as you put it. It's an easy error to make even if you're knowledgeable. I
agree that any behavioral changes should happen in libcurl and not PHP,
Then we agree, what's the reason for your rant?
but
that doesn't mean this isn't a problem or that we should just belittle
those who have encountered it and ignore it. Updating the documentation to
address this specific mistake might help reduce its frequency somewhat.
Throwing a warning or notice if a bool is passed would also be helpful in
my opinion.
If you limit it to the offending extension, file; if you want to throw a
warning every time a boolean is passed to a subroutine, I'll be very
disappointed in the PHP development community.
Let's not troll the OP for suggesting a solution to this issue just because
we don't agree with it. It's just a matter of mutual respect. This isn't
4chan or YouTube, after all. ;)
Get over that stuff please, it's a simple matter, one does not modify a
language to pamper poorly written extensions unless he wants the
language to rapidly become a kludge.
On Wed, Oct 24, 2012 at 10:34 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.I highly doubt code that sets
CURLOPT_SSL_VERIFYHOST=> true meant to
implyCURLOPT_SSL_VERIFYHOST=> 1...which essentially bypasses host
verification.According to libcurl,
CURLOPT_SSL_VERIFYHOST=> 1 is "not ordinarily a
useful setting".
The curl stream wrapper sets this option to 1 when using the
curl_verify_ssl_host context option.
I imagine that should be fixed too then?
-Hannes
Hey all - I'd like start a discussion around pull request 221
(https://github.com/php/php-src/pull/221).In short, there's a high volume of [incorrect] code out there which looks like:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, true);
Instead of what, in all likelyhood, the code meant to do:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
This is due to the convert_to_long_ex call which converts "true" to
1L.CURLOPT_SSL_VERIFYHOSTbeing set to 1L bypasses common name
validation within libcurl.My solution was to check the type for CURLOPT_SSL_VERIFYHOST: if it is
boolean and true, the opt value for libcurl is set to 2L.I understand that engineers should have the proper option value to
begin with but weighing the impact of this (MITM attacks) against
doing what they probably meant anyways is worth the presumption.Please discuss and adjust the patch if necessary.
- JJ
--
While I think it's a good idea to set the value of the option to 2, as
is recommended for production in the documentation, I think the idea
of implicitly converting a bool(true) to 2L internally might lead to
unexpected behavior since some people might actually depend on normal
PHP behavior to cast a bool(true) to 1 (and that might be what they
actually intended).I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.We should probably just elaborate on this point a bit more in the
documentation. Perhaps add a note and an example to illustrate. I
notice that people tend to pay more attention to examples than
anything else in the docs.
Booleans ought to be 1 and 0. Casting a boolean to 2 is just wrong, a
way to fix badly written code a few people have written and in so doing
risk the breakage of far more code that is correct.
2012/10/25 crankypuss fullmoon@newsguy.com
Hey all - I'd like start a discussion around pull request 221
(https://github.com/php/php-**src/pull/221https://github.com/php/php-src/pull/221
).In short, there's a high volume of [incorrect] code out there which
looks like:curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, true);
Instead of what, in all likelyhood, the code meant to do:
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
This is due to the convert_to_long_ex call which converts "true" to
1L.CURLOPT_SSL_VERIFYHOSTbeing set to 1L bypasses common name
validation within libcurl.My solution was to check the type for CURLOPT_SSL_VERIFYHOST: if it is
boolean and true, the opt value for libcurl is set to 2L.I understand that engineers should have the proper option value to
begin with but weighing the impact of this (MITM attacks) against
doing what they probably meant anyways is worth the presumption.Please discuss and adjust the patch if necessary.
- JJ
--
While I think it's a good idea to set the value of the option to 2, as
is recommended for production in the documentation, I think the idea
of implicitly converting a bool(true) to 2L internally might lead to
unexpected behavior since some people might actually depend on normal
PHP behavior to cast a bool(true) to 1 (and that might be what they
actually intended).I understand there are people out there that don't read the
documentation and aren't aware of the difference between
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); and curl_setopt($ch,
CURLOPT_SSL_VERIFYHOST, true); but still... I don't think this is a
good idea either.We should probably just elaborate on this point a bit more in the
documentation. Perhaps add a note and an example to illustrate. I
notice that people tend to pay more attention to examples than
anything else in the docs.Booleans ought to be 1 and 0. Casting a boolean to 2 is just wrong, a way
to fix badly written code a few people have written and in so doing risk
the breakage of far more code that is correct.
Thats not completely true. Boolean 'false' is equal to 0 and Boolean 'true'
is something different than 0, that may be 1 (and in most cases is), but
it's not limited too. Just said.
--
2012/10/25 crankypuss fullmoon@newsguy.com
Booleans ought to be 1 and 0. Casting a boolean to 2 is just wrong, a way
to fix badly written code a few people have written and in so doing risk
the breakage of far more code that is correct.Thats not completely true. Boolean 'false' is equal to 0 and Boolean 'true'
is something different than 0, that may be 1 (and in most cases is), but
it's not limited too. Just said.
In general that's true, but the PHP manual explicitly documents the
result of a boolean to integer conversion as 0 and 1:
http://au1.php.net/types.integer
Adam
Hi,
Am 25.10.2012 um 07:03 schrieb JJ jawed@php.net:
[...]
My solution was to check the type for CURLOPT_SSL_VERIFYHOST: if it is
boolean and true, the opt value for libcurl is set to 2L.I understand that engineers should have the proper option value to
begin with but weighing the impact of this (MITM attacks) against
doing what they probably meant anyways is worth the presumption.Please discuss and adjust the patch if necessary.
Good find. I would suggest to not actually change the behavior but throw a warning when a boolean is passed and advise the user to either pass int(1) explicitly or use int(2). Link to the manual in the warning and be good.
cu,
Lars