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

DoH: some options from user's transfer are not properly inherited #4578

Closed
3dyd opened this issue Nov 9, 2019 · 4 comments
Closed

DoH: some options from user's transfer are not properly inherited #4578

3dyd opened this issue Nov 9, 2019 · 4 comments

Comments

@3dyd
Copy link
Contributor

@3dyd 3dyd commented Nov 9, 2019

Take a look at these lines from dohprobe:

curl/lib/doh.c

Lines 277 to 282 in 8063c32

/* Inherit *some* SSL options from the user's transfer. This is a
best-guess as to which options are needed for compatibility. #3661 */
if(data->set.ssl.falsestart)
ERROR_CHECK_SETOPT(CURLOPT_SSL_FALSESTART, 1L);
if(data->set.ssl.primary.verifyhost)
ERROR_CHECK_SETOPT(CURLOPT_SSL_VERIFYHOST, 2L);

If data->set.ssl.primary.verifyhost is set to zero, appropriate option in DoH handle stays untouched. But its default value is 2. I.e. currently you cannot disable certificate's name verification for DoH request.

Not sure if this is intended behavior. And if it is, at least the comment is a bit misleading.

Affected options (whose default value is not zero):

  • CURLOPT_SSL_VERIFYHOST
  • CURLOPT_PROXY_SSL_VERIFYHOST
  • CURLOPT_SSL_VERIFYPEER
  • CURLOPT_PROXY_SSL_VERIFYPEER
  • + any used option whose default value would change in future.

Similar concern for the code constructions like the following one:

if(data->set.ssl.falsestart)
  ERROR_CHECK_SETOPT(CURLOPT_SSL_FALSESTART, 1L);

(Not sure though if this may generally happen in the library but) If option (falsestart in this case) would be expanded in further library versions (get more values than '0' or '1') then code like this will need to be revisited what is easy to overlook.

jay added a commit to jay/curl that referenced this issue Nov 9, 2019
Prior to this change some doh handle options inherited from the user's
easy handle were only inherited if they were turned on, not if they were
off.

[API option] : [Default API setting]
CURLOPT_NOSIGNAL              : OFF
CURLOPT_PROXY_SSL_VERIFYHOST  : ON    <-- affected
CURLOPT_PROXY_SSL_VERIFYPEER  : ON    <-- affected
CURLOPT_SSL_FALSESTART        : OFF
CURLOPT_SSL_VERIFYHOST        : ON    <-- affected
CURLOPT_SSL_VERIFYPEER        : ON    <-- affected
CURLOPT_SSL_VERIFYSTATUS      : OFF
CURLOPT_VERBOSE               : OFF

For example if CURLOPT_SSL_VERIFYPEER was turned off by the user then
that would not have been inherited by the doh handle and its verify peer
setting would have defaulted to on.

Prior to this change users were not able to disable SSL verification of
the DOH server.

Reported-by: 3dyd@users.noreply.github.com

Fixes curl#4578
Closes #xxxx
@jay
Copy link
Member

@jay jay commented Nov 9, 2019

Ref: 9e6af11

My fault, looks like that was my intention:

I did inherit insecure because I think that should still be in control of the user.

#4579 proposed fix

(Not sure though if this may generally happen in the library but) If option (falsestart in this case) would be expanded in further library versions (get more values than '0' or '1') then code like this will need to be revisited what is easy to overlook.

Yes, if those option values are expanded the code you cited should be amended. All the current internal members are bool, and they would have to be updated everywhere if they were changed from bool.

jay added a commit to jay/curl that referenced this issue Nov 9, 2019
Prior to this change some doh handle options inherited from the user's
easy handle were only inherited if they were turned on, not if they were
off.

[API option] : [Default API setting]
CURLOPT_NOSIGNAL              : OFF
CURLOPT_PROXY_SSL_VERIFYHOST  : ON    <-- affected
CURLOPT_PROXY_SSL_VERIFYPEER  : ON    <-- affected
CURLOPT_SSL_FALSESTART        : OFF
CURLOPT_SSL_VERIFYHOST        : ON    <-- affected
CURLOPT_SSL_VERIFYPEER        : ON    <-- affected
CURLOPT_SSL_VERIFYSTATUS      : OFF
CURLOPT_VERBOSE               : OFF

For example if CURLOPT_SSL_VERIFYPEER was turned off by the user then
that would not have been inherited by the doh handle and its verify peer
setting would have defaulted to on.

Prior to this change users were not able to disable SSL verification of
the DOH server.

Reported-by: 3dyd@users.noreply.github.com

Fixes curl#4578
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Nov 9, 2019
Prior to this change some doh handle options inherited from the user's
easy handle were only inherited if they were turned on, not if they were
off.

[API option] : [Default API setting]
CURLOPT_CERTINFO              : OFF
CURLOPT_NOSIGNAL              : OFF
CURLOPT_PROXY_SSL_VERIFYHOST  : ON    <-- affected
CURLOPT_PROXY_SSL_VERIFYPEER  : ON    <-- affected
CURLOPT_SSL_FALSESTART        : OFF
CURLOPT_SSL_VERIFYHOST        : ON    <-- affected
CURLOPT_SSL_VERIFYPEER        : ON    <-- affected
CURLOPT_SSL_VERIFYSTATUS      : OFF
CURLOPT_VERBOSE               : OFF

For example if CURLOPT_SSL_VERIFYPEER was turned off by the user then
that would not have been inherited by the doh handle and its verify peer
setting would have defaulted to on.

Prior to this change users were not able to disable SSL verification of
the DOH server.

Reported-by: 3dyd@users.noreply.github.com

Fixes curl#4578
Closes #xxxx
@bagder
Copy link
Member

@bagder bagder commented Nov 10, 2019

I think what's possibly more complicated is that CURLOPT_SSL_VERIFYHOST is for verifying the host curl connects to and it doesn't necessarily means that we should do the same thing for the host it connects to when resolving the host name using DoH...

@3dyd
Copy link
Contributor Author

@3dyd 3dyd commented Nov 10, 2019

Isn't this concern relevant for all inherited options?

One of strong libcurl sides is reasonable defaults. In context of this topic it would be great to keep current approach as it is simple to use, better than noop, and (subjectively) more expected behavior. Then maybe add as alternative some CURLOPT_XXXFUNCTION to tune DoH handle if one would need grained control over it?

@jay
Copy link
Member

@jay jay commented Nov 10, 2019

I think what's possibly more complicated is that CURLOPT_SSL_VERIFYHOST is for verifying the host curl connects to and it doesn't necessarily means that we should do the same thing for the host it connects to when resolving the host name using DoH...

That was the intention though, rather than establish a separate set of DOH options inherit relevant options from the user's handle, and I think that's true of --insecure. This bug was just a mistake on my part.

@bagder bagder closed this in 96a617b Mar 28, 2020
jay added a commit to jay/curl that referenced this issue Feb 11, 2021
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST,
  CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the
  same as their respective counterparts.

- New curl tool options --doh-insecure and --doh-cert-status do the same
  as their respective counterparts.

Ref: curl#4579 (comment)

Fixes curl#4578
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Feb 12, 2021
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST,
  CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the
  same as their respective counterparts.

- New curl tool options --doh-insecure and --doh-cert-status do the same
  as their respective counterparts.

Prior to this change DOH SSL certificate verification settings for
verifyhost and verifypeer were supposed to be inherited respectively
from CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER, but due to a bug
were not. As a result DOH verification remained at the default, ie
enabled, and it was not possible to disable. This commit changes
behavior so that the DOH verification settings are independent and not
inherited.

Ref: curl#4579 (comment)

Fixes curl#4578
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Feb 14, 2021
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST,
  CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the
  same as their respective counterparts.

- New curl tool options --doh-insecure and --doh-cert-status do the same
  as their respective counterparts.

Prior to this change DOH SSL certificate verification settings for
verifyhost and verifypeer were supposed to be inherited respectively
from CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER, but due to a bug
were not. As a result DOH verification remained at the default, ie
enabled, and it was not possible to disable. This commit changes
behavior so that the DOH verification settings are independent and not
inherited.

Ref: curl#4579 (comment)

Fixes curl#4578
Closes #xxxx
jay added a commit that referenced this issue Feb 14, 2021
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST,
  CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the
  same as their respective counterparts.

- New curl tool options --doh-insecure and --doh-cert-status do the same
  as their respective counterparts.

Prior to this change DOH SSL certificate verification settings for
verifyhost and verifypeer were supposed to be inherited respectively
from CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER, but due to a bug
were not. As a result DOH verification remained at the default, ie
enabled, and it was not possible to disable. This commit changes
behavior so that the DOH verification settings are independent and not
inherited.

Ref: #4579 (comment)

Fixes #4578
Closes #6597
jay added a commit that referenced this issue Mar 26, 2021
- Add description: Explain that some options aren't inherited because
  they are not relevant for the DoH SSL connections or may result in
  unexpected behavior.

- Remove the reference to #4578 (SSL verify options not inherited) since
  that was fixed by #6597 (separate DoH-specific options for verify).

- Explain that DoH-specific options (those created by #6597) are
  available: CURLOPT_DOH_SSL_VERIFYHOST, CURLOPT_DOH_SSL_VERIFYPEER and
  CURLOPT_DOH_SSL_VERIFYSTATUS.

- Add a reference to #6605 and explain that the user's debug function is
  not inherited because it would be unexpected to pass internal handles
  (ie DoH handles) to the user's callback.

Closes #6605
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants