Skip to content

fido2: plug some resource leaks in the client - #781

Open
kevans91 wants to merge 1 commit into
Nitrokey:mainfrom
kevans91:ke/fido2
Open

fido2: plug some resource leaks in the client#781
kevans91 wants to merge 1 commit into
Nitrokey:mainfrom
kevans91:ke/fido2

Conversation

@kevans91

@kevans91 kevans91 commented Aug 7, 2026

Copy link
Copy Markdown

The main target here is list_ctaphid_devices: on FreeBSD, this actually breaks nitropy list because our uhid/u2f drivers won't allow a second open() of the device. The devices that do not match will leak the fd, so later _list() calls (or at least the Nitrokey 3 one) cannot enumerate them at all.

filtered_devices() was split out to trivially plug the same leak in find_device().

Apologies in advance if my style/wrapping is atrocious, I only get to write python a little bit every few years.

@kevans91
kevans91 requested a review from a team as a code owner August 7, 2026 16:18
@kevans91
kevans91 requested review from daringer, james-knippes, mmerklinger, robin-nitrokey and sosthene-nitrokey and removed request for a team August 7, 2026 16:18
@robin-nitrokey

Copy link
Copy Markdown
Member

Thanks for the PR! Alternatively, we could also just iterate device descriptors and only connect to the devices that match our selector, like here:

https://github.com/Nitrokey/nitrokey-sdk-py/blob/77aa6f91b408103f516aacd352712f7f0c6cbb94/src/nitrokey/trussed/_device.py#L250-L255

What do you think?

@kevans91

kevans91 commented Aug 8, 2026

Copy link
Copy Markdown
Author

Thanks for the PR! Alternatively, we could also just iterate device descriptors and only connect to the devices that match our selector, like here:

https://github.com/Nitrokey/nitrokey-sdk-py/blob/77aa6f91b408103f516aacd352712f7f0c6cbb94/src/nitrokey/trussed/_device.py#L250-L255

What do you think?

Yes, good point. I see that still implies a transient open() in fido2.hid, but it's much cleaner here and in consumers if they can just filter the list without worrying as much about what they're discarding. I'll take a look at what that means for consumers of the API, but it seems like it will probably Just Work(TM) as long as find_device continues to open the connection.

The main target here is list_ctaphid_devices: on FreeBSD, this actually
breaks `nitropy list` because our uhid/u2f drivers won't allow a second
open() of the device.  The devices that *do not* match will leak the fd,
so later _list() calls (or at least the Nitrokey 3 one) cannot enumerate
them at all.

find_device() has the same leak in the path that finds one device by
serial, so that path is altered to similarly open the connection only
after applying our filtering.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
@kevans91

kevans91 commented Aug 8, 2026

Copy link
Copy Markdown
Author

I don't have a device that can use the fido2 client on-hand, so the find_device() part is untested. I realized after writing my last comment that list_ctaphid_devices should just continue opening connections, it's just that we want to apply the filtering on a descriptor list instead. I implemented that both there and for the one path in find_device.

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.

2 participants