gh-106687: _ssl: use uint64_t for SSL options by vstinner · Pull Request #106700 · python/cpython

vstinner · 2023-07-12T22:30:31Z

SSL_CTX_get_options() uses uint64_t for options:
https://www.openssl.org/docs/man3.1/man3/SSL_CTX_get_options.html

Fix this compiler warning on Windows with MSC:

conversion from 'uint64_t' to 'long', possible loss of data

Issue: New warning in ssl.c: conversion from 'uint64_t' to 'long', possible loss of data #106687

vstinner · 2023-07-12T22:32:47Z

cc @gpshead @erlend-aasland

vstinner · 2023-07-12T23:58:29Z

Hum, tests fail on Windows. I modified set_options() to use unsigned long long, instead of unsigned long.

Example of failure:

ERROR: test_create_server_ssl_verify_failed (test.test_asyncio.test_events.SelectEventLoopTests.test_create_server_ssl_verify_failed)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\1\s\Lib\test\test_asyncio\test_events.py", line 1052, in test_create_server_ssl_verify_failed
    server, host, port = self._make_ssl_server(
                         ^^^^^^^^^^^^^^^^^^^^^^
  File "D:\a\1\s\Lib\test\test_asyncio\test_events.py", line 972, in _make_ssl_server
    sslcontext = self._create_ssl_context(certfile, keyfile)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\a\1\s\Lib\test\test_asyncio\test_events.py", line 967, in _create_ssl_context
    sslcontext.options |= ssl.OP_NO_SSLv2
    ^^^^^^^^^^^^^^^^^^
  File "D:\a\1\s\Lib\ssl.py", line 562, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
OverflowError: Python int too large to convert to C unsigned long

gpshead · 2023-07-13T01:03:08Z

Modules/_ssl.c

    );

-    if (!PyArg_Parse(arg, "l", &new_opts))
+    if (!PyArg_Parse(arg, "O!", &PyLong_Type, &new_opts_obj)) {


Try the "K" conversion code for unsigned long long similar to the old "l" code, instead of "O!" with a type. No overflow checking that way, but the previous code had no overflow checking either.

A modern alternative would be to convert this to use argument clinic.

Ignoring overflow sounds like a bad idea for a security module.

gpshead · 2023-07-13T01:13:35Z

File "D:\a\1\s\Lib\test\test_asyncio\test_events.py", line 967, in _create_ssl_context
    sslcontext.options |= ssl.OP_NO_SSLv2
    ^^^^^^^^^^^^^^^^^^
  File "D:\a\1\s\Lib\ssl.py", line 562, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
OverflowError: Python int too large to convert to C unsigned long

the weird things there are numerous... "C unsigned long" is not a type this PR should be dealing with. and that constant... ssl.OP_NO_SSLv2 is an IntEnum that... on my local linux build appears to be 0 so that's a double WTF. Is this the PyErr_Occurred() triggering due to an lingering unchecked exception set elsewhere in the program?

/usr/include/openssl/ssl.h:# define SSL_OP_NO_SSLv2                                 0x0

gpshead · 2023-07-13T01:19:44Z

$ ./python 
Python 3.13.0a0 (..., Jul 10 2023, 15:00:57) [GCC 12.2.0] on linux
>>> import ssl, enum
>>> ssl.OP_NO_SSLv2
<Options.OP_NO_SSLv2: 0>
>>> isinstance(ssl.OP_NO_SSLv2, int)
True
>>> isinstance(ssl.OP_NO_SSLv2, enum.Enum)
True
>>> isinstance(ssl.OP_NO_SSLv2, enum.IntEnum)
False
>>> import struct
>>> struct.pack('LQ', ssl.OP_NO_SSLv2, ssl.OP_NO_SSLv2)
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
>>>

so yeah I really don't understand how that error would be the direct fault of what I see in this PR.

vstinner · 2023-07-13T01:56:58Z

On the updated PR, the error is now:

FAIL: test_options (test.test_ssl.ContextTests.test_options)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_ssl.py", line 961, in test_options
    self.assertEqual(default, ctx.options)
AssertionError: <Opti[69 chars]FERENCE|OP_NO_SSLv3|80: 38928464> != <Opti[69 chars]FERENCE|OP_NO_SSLv3|18446744071562068048: 18446744071600996432>

vstinner · 2023-07-13T02:00:09Z

About the surprising NO SSL2 error: test is like ctx.options = ctx.options ( since NO SSL2 is just 0), but it could not parse current options as unsigned long. Yeah, i also failed to understand the error at the beginning 😁

vstinner · 2023-07-13T12:11:36Z

On Windows, this PR changes the value of SSLContext.options: it returns a big integer, instead of returning a negative number! I propose to use unsigned integers in Python 3.13, but solve the warning differently on older Python versions: without changing the value.

test_options() fails because ssl.OP_ALL is a negative value instead of a positive value. We should either use signed integers everywhere, or unsigned integers everywhere, and it comes to "options" (and associated SSL constants).

gpshead · 2023-07-13T20:20:58Z

Ahha, no wonder. I believe Windows is a platform with sizeof(long) == 4 (even on 64-bit platforms). IL32,P64 instead of I32,PL64.

I agree, use unsigned for the options (effectively bit field flags) on 3.13. If done before rc1 this feels reasonable to do for 3.12 as it may prevent some future headaches.

SSL_CTX_get_options() uses uint64_t for options: https://www.openssl.org/docs/man3.1/man3/SSL_CTX_get_options.html Fix this compiler warning on Windows with MSC: conversion from 'uint64_t' to 'long', possible loss of data

vstinner · 2023-07-17T15:55:20Z

@vstinner vstinner removed the needs backport to 3.11

This PR can change ssl.OP_ALL value on Windows: it may be surprising to backport it to Python 3.11 which may need a different fix?

miss-islington · 2023-07-17T15:55:34Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

bedevere-bot · 2023-07-17T15:55:51Z

GH-106827 is a backport of this pull request to the 3.12 branch.

SSL_CTX_get_options() uses uint64_t for options: https://www.openssl.org/docs/man3.1/man3/SSL_CTX_get_options.html Fix this compiler warning on Windows with MSC: conversion from 'uint64_t' to 'long', possible loss of data (cherry picked from commit ad95c72) Co-authored-by: Victor Stinner <vstinner@python.org>

vstinner · 2023-07-17T16:00:19Z

Ahha, no wonder. I believe Windows is a platform with sizeof(long) == 4 (even on 64-bit platforms). IL32,P64 instead of I32,PL64.

What makes the problem worse is that the old OpenSSL API (ex: OpenSSL 1.0.2) uses signed long for options! See: https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_get_options.html

Maybe it only became a problem in OpenSSL 3 which got more options which don't git into a 32-bit signed integer? Or at least, that some options see their value becoming negative as 32-bit signed integer?

vstinner · 2023-07-17T16:09:32Z

On a recent 3.11 GHA job, I see that the Windows x64 job uses OpenSSL 1.1.1u 30 May 2023 and has a negative value for ssl.OP_ALL: https://github.com/python/cpython/actions/runs/5559894464/jobs/10156437844

ssl.OPENSSL_VERSION: OpenSSL 1.1.1u  30 May 2023
ssl.OP_ALL: -0x7fffffac

ssl.HAS_SNI: True
ssl.OPENSSL_VERSION_INFO: (1, 1, 1, 21, 15)
ssl.OP_NO_TLSv1_1: 0x10000000
ssl.SSLContext.maximum_version: -1
ssl.SSLContext.minimum_version: 771
ssl.SSLContext.options: 38928468
ssl.SSLContext.protocol: 16
ssl.SSLContext.verify_mode: 2
ssl.default_https_context.maximum_version: -1
ssl.default_https_context.minimum_version: 771
ssl.default_https_context.options: 38928468
ssl.default_https_context.protocol: 16
ssl.default_https_context.verify_mode: 2
ssl.stdlib_context.maximum_version: -1
ssl.stdlib_context.minimum_version: 771
ssl.stdlib_context.options: 38928468
ssl.stdlib_context.protocol: 16
ssl.stdlib_context.verify_mode: 0

But there is no compiler warnings:

Build succeeded.
    0 Warning(s)
    0 Error(s)

OpenSSL versions depending on the Python branch:

main and 3.12 branches: Windows uses OpenSSL 3.0.9, macOS uses OpenSSL 1.1.1u
3.11 branch: Windows uses OpenSSL 1.1.1u and macOS uses OpenSSL 1.1.1u

I'm not sure if my change works on OpenSSL 1.1 and 3.0: I didn't check if it does emit compiler warnings with OpenSSL 1.1 (which uses long for options, not uint64_t).

vstinner · 2023-07-17T16:25:58Z

Python 3.11 doesn't seem to emit a compiler warning. So I prefer to leave it unchanged for now, but only change the code if it starts to emit a warning: see #99079 which suggests upgrading OpenSSL from 1.1 to 3.0.

…6827) gh-106687: _ssl: use uint64_t for SSL options (GH-106700) SSL_CTX_get_options() uses uint64_t for options: https://www.openssl.org/docs/man3.1/man3/SSL_CTX_get_options.html Fix this compiler warning on Windows with MSC: conversion from 'uint64_t' to 'long', possible loss of data (cherry picked from commit ad95c72) Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-app · 2024-03-12T16:46:01Z

GH-116665 is a backport of this pull request to the 3.11 branch.

gh-106687: _ssl: use uint64_t for SSL options (#106700) SSL_CTX_get_options() uses uint64_t for options: https://www.openssl.org/docs/man3.1/man3/SSL_CTX_get_options.html Fix this compiler warning on Windows with MSC: conversion from 'uint64_t' to 'long', possible loss of data (cherry picked from commit ad95c72)

bedevere-bot added the awaiting core review label Jul 12, 2023

bedevere-bot mentioned this pull request Jul 12, 2023

New warning in ssl.c: conversion from 'uint64_t' to 'long', possible loss of data #106687

Closed

vstinner added the skip news label Jul 12, 2023

vstinner force-pushed the ssl_uint64 branch from 806f31e to 0a80833 Compare July 12, 2023 22:33

vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jul 12, 2023

gpshead approved these changes Jul 12, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 12, 2023

vstinner added skip news and removed skip news labels Jul 12, 2023

gpshead reviewed Jul 13, 2023

View reviewed changes

pythongh-106687: _ssl: use uint64_t for SSL options

0d9f542

SSL_CTX_get_options() uses uint64_t for options: https://www.openssl.org/docs/man3.1/man3/SSL_CTX_get_options.html Fix this compiler warning on Windows with MSC: conversion from 'uint64_t' to 'long', possible loss of data

vstinner force-pushed the ssl_uint64 branch from f3f6c45 to 0d9f542 Compare July 17, 2023 15:03

vstinner removed the needs backport to 3.11 only security fixes label Jul 17, 2023

vstinner merged commit ad95c72 into python:main Jul 17, 2023

bedevere-bot removed the awaiting merge label Jul 17, 2023

vstinner deleted the ssl_uint64 branch July 17, 2023 15:55

bedevere-bot removed the needs backport to 3.12 only security fixes label Jul 17, 2023

vstinner mentioned this pull request Jul 17, 2023

[3.12] gh-106687: _ssl: use uint64_t for SSL options (GH-106700) #106827

Merged

vstinner mentioned this pull request Jul 17, 2023

Use OpenSSL 3.0.x in our binary builds #99079

Closed

Uh oh!

Conversation

vstinner commented Jul 12, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jul 12, 2023

Uh oh!

vstinner commented Jul 12, 2023

Uh oh!

gpshead Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

gpshead Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

gpshead commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpshead commented Jul 13, 2023

Uh oh!

vstinner commented Jul 13, 2023

Uh oh!

vstinner commented Jul 13, 2023

Uh oh!

vstinner commented Jul 13, 2023

Uh oh!

gpshead commented Jul 13, 2023

Uh oh!

vstinner commented Jul 17, 2023

Uh oh!

miss-islington commented Jul 17, 2023

Uh oh!

bedevere-bot commented Jul 17, 2023

Uh oh!

vstinner commented Jul 17, 2023

Uh oh!

vstinner commented Jul 17, 2023

Uh oh!

vstinner commented Jul 17, 2023

Uh oh!

bedevere-app bot commented Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Jul 12, 2023 •

edited by bedevere-bot

Loading

gpshead commented Jul 13, 2023 •

edited

Loading