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.

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

Ping.

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

oops, didn't see :) thanks.

Michele, do you plan to update this patch?

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

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

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?

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

ping.

Michele Orrù,

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

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

Seconded Viktor's question.

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.

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