classification
Title: Fix Bluetooth address parser
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, cheryl.sabella, maker, neologix, pitrou, serhiy.storchaka, steve.dower, vstinner
Priority: normal Keywords: patch

Created on 2013-07-26 17:59 by maker, last changed 2019-05-26 17:59 by maker.

Files
File name Uploaded Description Edit
btoverflow.patch maker, 2013-07-26 17:59 review
issue18564.1.patch maker, 2013-07-27 21:43 review
issue18564.2.patch maker, 2014-04-30 12:19 review
issue18564.3.patch maker, 2014-05-03 16:49 review
Pull Requests
URL Status Linked Edit
PR 12864 open python-dev, 2019-04-17 16:54
Messages (16)
msg193736 - (view) Author: Michele Orrù (maker) * Date: 2013-07-26 17:59
In Modules/socketmodule.c , the bluetooth address supplied is vulnerable to integer overflow.

Attaching patch and a couple of tests, which should be considered as a step forward in #7687.
msg193795 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-07-27 20:38
Instead of writing try / except / self.fail, you could simply use the context manager form of assertRaises.
msg196305 - (view) Author: Michele Orrù (maker) * Date: 2013-08-27 18:22
Ping.
msg196306 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-27 18:24
Ah, haven't you seen Charles-François' comments on the review tool?
Click on the "review" link next to your patch :-)
msg196309 - (view) Author: Michele Orrù (maker) * Date: 2013-08-27 18:44
oops, didn't see :) thanks.
msg217453 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-28 23:51
Michele, do you plan to update this patch?
msg217664 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-30 23:42
Interestingly, the tests are skipped here (Linux 3.11.0-20-generic). For some reason my socket module is built without bluetooth support (HAVE_BLUETOOTH_BLUETOOTH_H and HAVE_BLUETOOTH_H are both undefined).
msg217665 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-30 23:45
Ah, I had to install libbluetooth-dev. Sorry for the noise.
msg217825 - (view) Author: Michele Orrù (maker) * Date: 2014-05-03 17:15
Interestingly, <bluetooth/bluetooth.h> implements a function for parsing bluetooth addresses, but it's completely broken.
<https://git.kernel.org/cgit/bluetooth/bluez.git/tree/lib/bluetooth.c#n83> 
It would be much much more elegant to use str2ba() in our source code though.

I am thinking about patching it there and then open another ticket here in order to adopt str2ba(). This way we can close this ticket for now.
Does this sound reasonable to you, Antoine?
msg218128 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-05-08 22:15
> I am thinking about patching it there and then open another ticket
> here in order to adopt str2ba(). This way we can close this ticket for > now.

Well, if some str2ba() versions are notoriously buggy, we should probably not use it, IMHO.
msg220189 - (view) Author: Michele Orrù (maker) * Date: 2014-06-10 18:43
ping.
msg339896 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-04-10 22:36
Michele Orrù,

Would you be interested in making a GitHub pull request for your patch?  Thanks!
msg340421 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-17 17:00
> In Modules/socketmodule.c , the bluetooth address supplied is vulnerable to integer overflow.

Attached PR 12864 modifies the following code:


  unsigned int b0, b1, b2, b3, b4, b5;
  char ch;
  int n;
  n = sscanf(name, "%X:%X:%X:%X:%X:%X%c", &b5, &b4, &b3, &b2, &b1, &b0, &ch);

Can someone please elaborate how this code can trigger an integer overflow? What is the consequence of an integer overflow? Does Python crash?
msg340444 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-04-17 20:46
Seconded Viktor's question.
msg340676 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-04-22 19:06
According to a couple of scanf docs I found, the '%x' format expects to write into unsigned int*, just as we already do. So it shouldn't be possible to overflow there.

The following line (or-ing all the values and checking that it's less than 256) handles the overflow already.

Limiting each %x specifier to two characters has exactly the same effect, and could potentially fix overflow errors in C runtimes that assume a larger destination without the data size prefix ('%zx' or '%llx'), but I don't know of any of those.

All that said, I'm not opposed to adding the tests. If the parsing logic is a sticking point, then that can be undone, but I think it's also okay.
msg340694 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-23 07:11
Hum, maybe I'm just confused by "integer overflow". To me, "integer overflow" means that an operation goes out of the bounds of a C integer type. But here, the problem is more than the parser accepts invalid Bluetooth addresses?

Please use a better title than "Fix integer overflow in socketmodule". Maybe "Fix Bluetooth address parser"?
History
Date User Action Args
2019-05-26 17:59:02makersettitle: Integer overflow in the socket function parsing a Bluetooth address -> Fix Bluetooth address parser
2019-04-23 07:11:46vstinnersetmessages: + msg340694
2019-04-22 19:06:05steve.dowersetnosy: + steve.dower
messages: + msg340676
2019-04-17 20:46:09serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg340444
2019-04-17 17:00:44vstinnersettitle: Integer overflow in socketmodule -> Integer overflow in the socket function parsing a Bluetooth address
versions: + Python 3.7, Python 3.8
2019-04-17 17:00:09vstinnersetmessages: + msg340421
2019-04-17 16:54:43python-devsetstage: patch review
pull_requests: + pull_request12791
2019-04-10 22:36:55cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg339896
2014-06-10 18:43:32makersetmessages: + msg220189
2014-05-11 11:55:12Arfreversetnosy: + Arfrever
2014-05-08 22:15:36pitrousetmessages: + msg218128
2014-05-03 17:15:36makersetmessages: + msg217825
2014-05-03 16:49:48makersetfiles: + issue18564.3.patch
2014-04-30 23:45:50pitrousetmessages: + msg217665
2014-04-30 23:42:09pitrousetmessages: + msg217664
2014-04-30 12:19:21makersetfiles: + issue18564.2.patch
2014-04-28 23:51:15pitrousetmessages: + msg217453
2013-08-27 18:44:00makersetmessages: + msg196309
2013-08-27 18:24:06pitrousetmessages: + msg196306
2013-08-27 18:22:59makersetmessages: + msg196305
2013-07-27 23:54:48vstinnersetnosy: + vstinner
2013-07-27 21:43:16makersetfiles: + issue18564.1.patch
2013-07-27 20:38:27pitrousetnosy: + neologix, pitrou
messages: + msg193795
2013-07-26 17:59:05makercreate