Fix Bluetooth address parser

[mmaker]

BPO	18564
Nosy	@pitrou, @vstinner, @mmaker, @serhiy-storchaka, @zooba, @csabella
PRs	gh-62764: Fix integer overflow in socketmodule. #12864
Files	btoverflow.patch issue18564.1.patch issue18564.2.patch issue18564.3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-07-26.17:59:05.783>
labels = ['extension-modules', '3.8', 'type-bug', '3.7']
title = 'Fix Bluetooth address parser'
updated_at = <Date 2019-05-26.17:59:02.673>
user = 'https://github.com/mmaker'

bugs.python.org fields:

activity = <Date 2019-05-26.17:59:02.673>
actor = 'maker'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2013-07-26.17:59:05.783>
creator = 'maker'
dependencies = []
files = ['31041', '31056', '35112', '35146']
hgrepos = []
issue_num = 18564
keywords = ['patch']
message_count = 16.0
messages = ['193736', '193795', '196305', '196306', '196309', '217453', '217664', '217665', '217825', '218128', '220189', '339896', '340421', '340444', '340676', '340694']
nosy_count = 8.0
nosy_names = ['pitrou', 'vstinner', 'Arfrever', 'neologix', 'maker', 'serhiy.storchaka', 'steve.dower', 'cheryl.sabella']
pr_nums = ['12864']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18564'
versions = ['Python 3.7', 'Python 3.8']

[mmaker]

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 bpo-7687.

[pitrou]

Instead of writing try / except / self.fail, you could simply use the context manager form of assertRaises.

[mmaker]

Ping.

[pitrou]

Ah, haven't you seen Charles-François' comments on the review tool?
Click on the "review" link next to your patch :-)

[mmaker]

oops, didn't see :) thanks.

[pitrou]

Michele, do you plan to update this patch?

[pitrou]

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).

[pitrou]

Ah, I had to install libbluetooth-dev. Sorry for the noise.

[mmaker]

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?

[pitrou]

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.

[mmaker]

ping.

[csabella]

Michele Orrù,

Would you be interested in making a GitHub pull request for your patch? Thanks!

[vstinner]

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?

[serhiy-storchaka]

Seconded Viktor's question.

[zooba]

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.

[vstinner]

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"?

[serhiy-storchaka]

Closing, since there is no the claimed integer overflow here, and the PR was broken since 2019.