classification
Title: _ssl module: possible integer overflow for very long strings (+2^31 bytes)
Type: Stage:
Components: Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, pitrou, python-dev, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2013-06-04 21:34 by haypo, last changed 2013-06-28 18:34 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
ssl_int.patch haypo, 2013-06-04 21:34
ssl_overflow.patch haypo, 2013-06-23 13:56 review
ssl_overflow-2.patch haypo, 2013-06-24 20:20 review
Messages (11)
msg190614 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-04 21:34
Our Windows 64-bit buildbot has interesting warnings:

..\Modules\_ssl.c(493): warning C4244: 'function' : conversion from 'SOCKET_T' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]
  ..\Modules\_ssl.c(1304): warning C4244: 'function' : conversion from 'SOCKET_T' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]
  ..\Modules\_ssl.c(1306): warning C4244: 'function' : conversion from 'SOCKET_T' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]
  ..\Modules\_ssl.c(1360): warning C4244: 'function' : conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]
  ..\Modules\_ssl.c(1655): warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]
  ..\Modules\_ssl.c(1659): warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]
  ..\Modules\_ssl.c(2109): warning C4244: 'return' : conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\_ssl.vcxproj]

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/2042/steps/compile/logs/warnings%20%28532%29

It looks like the _ssl.c module does mix int and size_t types. Attached patch should fix 3 warnings. I didn't test my patch except running test_ssl (with success on my Linux x64 box).
msg190647 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-06-05 09:59
The patch contains a lot of unrelated trailing spaces changes. Could you please commit they separately? See also issue15550.
msg191694 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-23 13:18
New changeset f0d934732ab1 by Victor Stinner in branch '3.3':
Issue #18135: Fix a possible integer overflow in ssl.SSLSocket.write()
http://hg.python.org/cpython/rev/f0d934732ab1

New changeset f90d82a75a43 by Victor Stinner in branch 'default':
(Merge 3.3) Issue #18135: Fix a possible integer overflow in
http://hg.python.org/cpython/rev/f90d82a75a43

New changeset d7e22acb2315 by Victor Stinner in branch '2.7':
Issue #18135: Fix a possible integer overflow in ssl.SSLSocket.write()
http://hg.python.org/cpython/rev/d7e22acb2315
msg191696 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-06-23 13:31
I'm sorry to chime in a bit late, but I think this isn't the correct solution. Right now partial writes are not possible on a SSL socket, but this commit makes them possible. See http://bugs.python.org/issue8240 and http://bugs.python.org/issue12197 for some background.

I think the right solution here would be to raise OverflowError, not truncate the output.
msg191700 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-23 13:45
> Right now partial writes are not possible on a SSL socket, but this commit makes them possible.

Oh, I didn't know (forgot) that SSL does allow partial write by default.

> I think the right solution here would be to raise OverflowError, not truncate the output.

Do you mean always? Or only if the SSL_MODE_ENABLE_PARTIAL_WRITE option is not set?
msg191701 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-06-23 13:48
> > I think the right solution here would be to raise OverflowError, not truncate the output.
> 
> Do you mean always? Or only if the SSL_MODE_ENABLE_PARTIAL_WRITE option is not set?

SSL_MODE_ENABLE_PARTIAL_WRITE is never set.
msg191702 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-23 13:56
> I think the right solution here would be to raise OverflowError, not truncate the output.

Here is a new patch (for Python 3.3) always raising OverflowError if the string is longer than INT_MAX bytes.
msg191798 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-24 20:20
Fixed patch: ssl_overflow-2.patch.
msg191818 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-24 22:44
New changeset bfede07268a1 by Victor Stinner in branch '3.3':
Issue #18135: ssl.SSLSocket.write() now raises an OverflowError if the input
http://hg.python.org/cpython/rev/bfede07268a1

New changeset 12a388024d5b by Victor Stinner in branch 'default':
(Merge 3.3) Issue #18135: ssl.SSLSocket.write() now raises an OverflowError if
http://hg.python.org/cpython/rev/12a388024d5b
msg191819 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-24 22:48
New changeset a29eaffa7d72 by Victor Stinner in branch '2.7':
Issue #18135: ssl.SSLSocket.write() now raises an OverflowError if the input
http://hg.python.org/cpython/rev/a29eaffa7d72
msg192007 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-28 18:13
Re-close?
History
Date User Action Args
2013-06-28 18:34:02hayposetstatus: open -> closed
2013-06-28 18:13:18terry.reedysetnosy: + terry.reedy
messages: + msg192007
2013-06-24 22:48:27python-devsetmessages: + msg191819
2013-06-24 22:44:02python-devsetmessages: + msg191818
2013-06-24 20:20:10hayposetfiles: + ssl_overflow-2.patch

messages: + msg191798
2013-06-23 13:56:11hayposetfiles: + ssl_overflow.patch

messages: + msg191702
2013-06-23 13:48:01pitrousetmessages: + msg191701
2013-06-23 13:45:54hayposetmessages: + msg191700
2013-06-23 13:31:25pitrousetstatus: closed -> open

messages: + msg191696
2013-06-23 13:19:11hayposetstatus: open -> closed
resolution: fixed
2013-06-23 13:18:47python-devsetnosy: + python-dev
messages: + msg191694
2013-06-05 09:59:24serhiy.storchakasetmessages: + msg190647
2013-06-05 04:41:30serhiy.storchakasetnosy: + serhiy.storchaka
2013-06-04 21:34:39haypocreate