Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uuid.getnode() should return the MAC address on Android #76380

Closed
xdegaye mannequin opened this issue Dec 2, 2017 · 19 comments
Closed

uuid.getnode() should return the MAC address on Android #76380

xdegaye mannequin opened this issue Dec 2, 2017 · 19 comments
Labels
3.7 easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@xdegaye
Copy link
Mannequin

xdegaye mannequin commented Dec 2, 2017

BPO 32199
Nosy @warsaw, @xdegaye, @serhiy-storchaka
PRs
  • bpo-32199: uuid.getnode() now returns the MAC address on Android #4696
  • [3.6] bpo-32199: The getnode() ip getter now uses 'ip link' instead of 'ip link list' (GH-4696) #4747
  • Files
  • ip_link.strace
  • ip_link_list.strace
  • archlinux-ip_link.strace
  • archlinux-ip_link_list.strace
  • 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 = <Date 2017-12-07.12:50:06.499>
    created_at = <Date 2017-12-02.14:29:39.460>
    labels = ['3.7', 'easy', 'type-bug', 'library']
    title = 'uuid.getnode() should return the MAC address on Android'
    updated_at = <Date 2017-12-07.16:39:39.466>
    user = 'https://github.com/xdegaye'

    bugs.python.org fields:

    activity = <Date 2017-12-07.16:39:39.466>
    actor = 'xdegaye'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-12-07.12:50:06.499>
    closer = 'xdegaye'
    components = ['Library (Lib)']
    creation = <Date 2017-12-02.14:29:39.460>
    creator = 'xdegaye'
    dependencies = []
    files = ['47322', '47323', '47326', '47327']
    hgrepos = []
    issue_num = 32199
    keywords = ['patch', 'easy']
    message_count = 19.0
    messages = ['307432', '307675', '307677', '307688', '307697', '307703', '307704', '307705', '307706', '307707', '307708', '307715', '307742', '307743', '307793', '307797', '307801', '307815', '307817']
    nosy_count = 3.0
    nosy_names = ['barry', 'xdegaye', 'serhiy.storchaka']
    pr_nums = ['4696', '4747']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32199'
    versions = ['Python 3.6', 'Python 3.7']

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 2, 2017

    Currently uuid.getnode() returns a random 48-bit number and so the UUIDs are not persistent across time. The reason is that on Android the 'ip link list' command fails.

    uuid._ip_getnode() should invoke the 'ip link' command instead.

    @xdegaye xdegaye mannequin added 3.7 stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 2, 2017
    @warsaw
    Copy link
    Member

    warsaw commented Dec 5, 2017

    Over in the PR I suggested:

    Here's another thought: what if you just added another getter that calls ip link list and placed that after one that calls ip link. Wouldn't that accomplish both goals? Then if ip link fails, we fall back to the old behavior, so nothing changes. It's uglier, but it doesn't special case for the Android platform, and eventually we can decide to remove ip link list altogether.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Dec 5, 2017

    Why the 'ip link list' command fails on Android at first place? Does Android use its own independent implementation? Or its version is based on the fork of very old version of iproute2 that didn't supported the list command (if there was such version)?

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 5, 2017

    The result of various 'ip' commands on Android, the last 'ip link list' command is run as root and succeeds (did not think about trying that before):

    generic_x86_64:/data/local/tmp/python $ ip link list
    request send failed: Permission denied

    1|generic_x86_64:/data/local/tmp/python $ ip link help
    request send failed: Permission denied

    1|generic_x86_64:/data/local/tmp/python $ ip link
    1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    3: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DORMANT group default qlen 1000
    link/ether 02:00:00:44:55:66 brd ff:ff:ff:ff:ff:ff
    5: hwsim0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default
    link/ieee802.11/radiotap 12:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
    6: sit0: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT group default
    link/sit 0.0.0.0 brd 0.0.0.0
    8: radio0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000
    link/ether 22:d5:92:86:1a:d8 brd ff:ff:ff:ff:ff:ff

    generic_x86_64:/data/local/tmp/python # ip link list
    1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    3: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DORMANT group default qlen 1000
    link/ether 02:00:00:44:55:66 brd ff:ff:ff:ff:ff:ff
    5: hwsim0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default
    link/ieee802.11/radiotap 12:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
    6: sit0: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT group default
    link/sit 0.0.0.0 brd 0.0.0.0
    8: radio0: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
    link/ether 22:d5:92:86:1a:d8 brd ff:ff:ff:ff:ff:ff

    @warsaw
    Copy link
    Member

    warsaw commented Dec 5, 2017

    On Dec 5, 2017, at 16:28, Xavier de Gaye <report@bugs.python.org> wrote:

    The result of various 'ip' commands on Android, the last 'ip link list' command is run as root and succeeds (did not think about trying that before):

    generic_x86_64:/data/local/tmp/python $ ip link list
    request send failed: Permission denied

    1|generic_x86_64:/data/local/tmp/python $ ip link help
    request send failed: Permission denied

    1|generic_x86_64:/data/local/tmp/python $ ip link
    …[output]…

    Well, that’s weird!
    -B

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Dec 6, 2017

    What if "ip link list" was intentionally prohibited "for security reasons", and "ip link" works just due to oversight? Xavier, could you please inspect the sources of the ip command on Android? Is it the standard iproute2 with additional patches prohibiting the part of the functionality?

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 6, 2017

    Whatever the change made to fix this issue, it is not possible to add a test case for this change.

    So following the suggestion made by Barry in PR 4696, we can add (in another issue) a new keyword parameter to getnode() named 'methods' whose value may be None (the default, meaning try all the known methods) or a tuple containing a subset of the following methods ('unix', 'ifconfig', 'ip', 'arp', 'lanscan', 'netstat', 'random') that would raise an exception if the value cannot be obtained using one of the requested method tried in the requested order. This would also improve the documentation on the methods getnode() is using. Then if we decide to make the change for 'ip link' in the current issue, one can add a test case that would first test for the avaibility of the ip command and if the command exists would fail if getnode(methods=('ip',)) raises an exception.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 6, 2017

    You may be right Serhiy. Those tests have been run on the emulator at API 24 (Android 7.0 Nougat, the first API version where SELinux is run in enforced mode) where 'ip link list' fails, but on my device (a Samsung API 21, Android 5.1 Lollipop) running the 'ip link list' (using the termux package installed from google PlayStore) the command is ok.

    The Android source of iproute2 can be:

    The Android SELinux policies are at:
    https://android.googlesource.com/platform/system/sepolicy/
    Does someone know how to read them ?

    Maybe we should just close this issue as 'wont fix' then.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 6, 2017

    'adb logcat' is a tool that monitors many kind of events on Android. Both ip commands prints a SElinux record on logcat:

    Upon the successfull 'ip link' command, logcat prints:
    12-06 09:17:24.119 2460 2460 W ip : type=1400 audit(0.0:8): avc: denied { search } for name="net" dev="vdc" ino=91 scontext=u:r🐚s0 tcontext=u:object_r:net_data_file:s0 tclass=dir permissive=0

    Upon the failed 'ip link' command, logcat prints:
    12-06 09:17:42.109 2461 2461 W ip : type=1400 audit(0.0:9): avc: denied { nlmsg_write } for scontext=u:r🐚s0 tcontext=u:r🐚s0 tclass=netlink_route_socket permissive=0

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 6, 2017

    Oops, the second failed command is 'ip link list' of course.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 6, 2017

    It is also possible that rather than an oversight in Android, it is a side effect of SELinux on the implementation of iproute2 if the 'ip link list' command does a little bit more than the 'ip link' command and if this 'little bit more' is prohibited by a SELinux policy. I guess this means diving into the source of iproute2 to confirm that :-(

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 6, 2017

    Attached ip_link.strace and ip_link_list.strace, the output of strace for the 'ip link' and 'ip link list' commands.

    At the end of the process, both commands create an PF_NETLINK socket to receive from the kernel link information through the NETLINK_ROUTE group. The sendto() function fails with EACCES for 'ip link list' and its SELinux avc record relates to this event (the netlink prefix used throughout the documentation [1] is 'nlmsg' and the resource denied in the avc record is 'nlmsg_write'). The SELinux avc record for 'ip link' relates to a failed attempt to open "/data/misc/net/group" (it does not exist).

    [1] http://man7.org/linux/man-pages/man7/netlink.7.html

    @warsaw
    Copy link
    Member

    warsaw commented Dec 6, 2017

    On Dec 6, 2017, at 02:06, Xavier de Gaye <report@bugs.python.org> wrote:

    Whatever the change made to fix this issue, it is not possible to add a test case for this change.

    Even with say, exception raising mocks for the getters?

    So following the suggestion made by Barry in PR 4696, we can add (in another issue) a new keyword parameter to getnode() named 'methods' whose value may be None (the default, meaning try all the known methods) or a tuple containing a subset of the following methods ('unix', 'ifconfig', 'ip', 'arp', 'lanscan', 'netstat', 'random') that would raise an exception if the value cannot be obtained using one of the requested method tried in the requested order. This would also improve the documentation on the methods getnode() is using. Then if we decide to make the change for 'ip link' in the current issue, one can add a test case that would first test for the avaibility of the ip command and if the command exists would fail if getnode(methods=('ip',)) raises an exception.

    I am thinking about this slightly differently.

    What if getnode() accepted a handler argument and the code was changed to something like this:

    1 file changed, 4 insertions(+), 2 deletions(-)
    Lib/uuid.py | 6 ++++--

    modified Lib/uuid.py
    @@ -656,7 +656,7 @@ def _random_getnode():

     _node = None

    -def getnode():
    +def getnode(handler=None):
    """Get the hardware address as a 48-bit positive integer.

         The first time this runs, it may launch a separate program, which could
    @@ -677,7 +677,9 @@ def getnode():
         for getter in getters + [_random_getnode]:
             try:
                 _node = getter()
    -        except:
    +        except Exception as error:
    +            if handler is not None:
    +                handler(getter, error)
                 continue
             if _node is not None:
                 return _node

    handler could log some diagnostics, reraise the exception, raise StopIteration, etc. Then we could use that in the test suite too, because we could mock a getter to raise an exception and then pass in a handler that verified the exception was raised with the expected getter.

    (Maybe we spell handler as error_handler.)

    @warsaw
    Copy link
    Member

    warsaw commented Dec 6, 2017

    Maybe we should just close this issue as 'wont fix' then.

    I would be okay with any of these resolutions:

    • Close as wont fix
    • Just call ip link (without list)
    • Add a new getter such that both ip link and ip link list are called.

    How to handle exceptions in the getters should be addressed in a new issue.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 7, 2017

    On archlinux it is easy to know precisely what patches are applied to iproute2 and how it is built (see https://git.archlinux.org/svntogit/packages.git/tree/trunk?h=packages/iproute2).

    The attached two files, archlinux-ip_link.strace and archlinux-ip_link_list.strace, contain the output of strace run on the commands 'ip link' and 'ip link list' on archlinux.

    • For 'ip link', the sendto() syscall uses RTM_GETLINK to get information about a specific network interface.
    • For 'ip link list', this sendto() syscall is preceded by another sendto() syscall using RTM_NEWLINK to *create* information about a specific network interface.

    Conclusions:

    1. Both commands are not equivalent, this seems to be a bug in iproute2 or its documentation (I did not read the whole iproute2 documentation).
    2. By using RTM_NEWLINK, 'ip link list' requests a write-like operation that may be denied by SELinux if there is no policy that allows netlink_route_socket (nlmsg_write). I may be wrong but on Android API 26 it seems that only few processes get that permission: dhcp, clatd, logd, netd, rild, ...
    3. From Python perspective it is more robust to call 'ip link' to handle platforms where SELinux is run in enforcing mode.

    I will update the PR to do only that change: s/ip link list/ip link/

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 7, 2017

    New changeset 961dbe0 by xdegaye in branch 'master':
    bpo-32199: The getnode() ip getter now uses 'ip link' instead of 'ip link list' (GH-4696)
    961dbe0

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 7, 2017

    New changeset 03031fb by xdegaye (Miss Islington (bot)) in branch '3.6':
    bpo-32199: The getnode() ip getter now uses 'ip link' instead of 'ip link list' (GH-4696) (bpo-4747)
    03031fb

    @xdegaye xdegaye mannequin closed this as completed Dec 7, 2017
    @warsaw
    Copy link
    Member

    warsaw commented Dec 7, 2017

    LGTM, and thanks!

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 7, 2017

    Thanks Serhiy and Barry for your comments and reviews :-)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants