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

ble: allow configuring device privacy #2401

Closed
wants to merge 1 commit into from

Conversation

ssievert42
Copy link
Contributor

@ssievert42 ssievert42 commented Aug 14, 2023

This allows using a random private resolvable address to prevent being tracked.

There are now two new methods: NRF.setPrivacy() and NRF.getPrivacy(), to set and get the current privacy settings.

One thing that (currently) cannot be configured is our IRK. Should be possible to add this as well, but I'd rather not do that, because that would complicate things a bit.

I always get a bit nervous when working directly with pointers, hopefully I'm not leaking any memory and / or freeing memory too early.

The initial setup with Gadgetbridge can be kind of weird and create a bunch of devices that don't actually exist, but everything works without (noticable) issues after that.

Something that bugs me is that if we run this on startup in addition to NRF.setSecurity() we may trigger a softdevice restart twice.

ToDo:

  • add nicer docs to explain what exactly NRF.setPrivacy() expects.
  • try to figure out why Gadgetbridge may add devices that don't exist on initial setup

@gfwilliams
Copy link
Member

I didn't even realise this was a thing - seems like a good idea. But there are definitely memory leaks from handling JsVars.

if we run this on startup in addition to NRF.setSecurity() we may trigger a softdevice restart twice.

It's probably not a huge deal, but what if we just add these as options to setSecurity and getSecurityStatus? It feels like it'd be cleaner than having another set of API functions, and it avoids the restart issue

@ssievert42 ssievert42 force-pushed the ble_privacy branch 2 times, most recently from bfebf94 to 8b3c42c Compare August 14, 2023 16:20
@ssievert42
Copy link
Contributor Author

The leaks should be fixed now; I'm still not used to working with refcounting manually like this 😅

About adding these as options to NRF.setSecurity() and NRF.getSecurityStatus():
I tried doing that, but just can't get myself to like having that many, only kind of related, options in one place.
If privacy was a part of NRF.setSecurity(), the description that it "sets the security options used when connecting/pairing" wouldn't quite fit anymore, because the privacy stuff is relevant no matter the connection state. The same would apply to NRF.getSecurityStatus() that currently returns "information about the security state of the current peripheral connection", which is not where I would expect to find the current privacy config. Nordic also provides separate functions for security and privacy.
I guess I could live with two softdevice restarts, especially since you said that it's not a huge deal. Although I've got no idea what the cost of having more API functions is.
Obsiously it's your call where this goes in the end :)

#ifdef NRF52_SERIES
// Set MAC address
JsVar *v = jsvObjectGetChildIfExists(execInfo.hiddenRoot, BLE_NAME_MAC_ADDRESS);
if (v) {
ble_gap_addr_t p_addr;
if (bleVarToAddr(v, &p_addr)) {
#if PEER_MANAGER_ENABLED
err_code = pm_id_addr_set(&p_addr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is here because the docs state that, if the peer manager is used, sd_ble_gap_addr_set must not be called and pm_id_addr_set should be used instead.
I don't know what might happen if we ignore that, but "must not be called" sounds pretty serious to me.
Ideally this shouldn't break anything.

This allows using a random private resolvable address to prevent being tracked.
@bobrippling
Copy link
Contributor

I've been tinkering with BLE addresses recently and would be interested in this - is there much left to bring it to being mergeable?

@ssievert42
Copy link
Contributor Author

ssievert42 commented May 24, 2024

@bobrippling I haven't touched this in some time, but a slightly modified version has been working fine for me for the last couple of months (firmware I'm currently running).

Well, apart from the button on my Bangle randomly deciding to remain pressed and triggering reboots, that cause the watch to forget all stored bonding info and sometimes also the time 😢

The issue with Gadgetbridge adding new devices (seemingly at random, but likely while the phone has GPS turned on, the main screen listing devices is visible and the watch decides to change its address) still exists. I wanted to take a look at the Gadgetbridge code, but somehow never got around to doing that.
That was / is the main blocker from my perspective.

This feature also uses some additional flash, which is apparently enough for the Puck.js and Pixl.js builds to fail. (At least on my "flash_me" branch.)

I've since mostly come to terms with having the random address options as part of NRF.setSecurity(), but haven't done that change (yet).

Something I don't really like, is that the random address does not only change every 15 minutes, but also every time the softdevice restarts, which happens every time there is a non-fastload transition between apps. But I guess this isn't really avoidable.

Speaking of softdevice restarts: Having random addresses becomes pretty useless (at least if your intention is to avoid being trackable) if doing a non-fastload transition between apps may shortly send a beacon with your actual address, before the bootloader is able to configure the watch to use a random address. Which is why my "flash_me" branch includes some changes made by Gordon here, along with a commit by me (that I should probably clean up) with the title "ble: cleaner init", to prevent us from restarting the softdevice before all config options are set, and accidentally sending our real address. Not sure if the stuff I added actually does what I want it to do though 😅

Having random addresses also quickly becomes useless if we are sending beacons that contain a unique-enough device name, like "Bangle.js ffff". For that reason, I've changed my bootloader to call NRF.setAdvertising({}, {showName: false});, but this has to be turned off for pairing with Gadgetbridge to work.

There's also at least one philosophical question, which is which address NRF.getAddress() should return.

In short: I've been using this mostly without serious issues and could definitely tidy this up a bit to make it mergeable :)

@ssievert42
Copy link
Contributor Author

Superseded by #2506

@ssievert42
Copy link
Contributor Author

This can be closed now.

@ssievert42 ssievert42 closed this May 26, 2024
@bobrippling
Copy link
Contributor

I see, fair points about the name being shown and the brief window before the softdevice can be configured - thanks for the details. That lines up / helps answer some thoughts/questions with what I've been tinkering with.

I guess I'd not be able to use the random private addresses with OpenHaystack too, since it pretty much fixes the address as a public one, but I've not looked into OpenHaystack to see if we can advertise a public key as part of a private random address - an investigation in progress!

There's also at least one philosophical question, which is which address NRF.getAddress() should return.

Certainly! I found this and ended up with #2503

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants