new device API #32
new device API #32
Conversation
qubesusbproxy/core3ext.py
Outdated
| untrusted_device_desc = untrusted_device_desc.decode( | ||
| 'unicode_escape', errors='ignore') |
There was a problem hiding this comment.
Is this really safe for untrusted input? I’d expect this is normally used for trusted input. I recommend reimplementing this in pure Python. Alternatively, is there a reason that this is needed at all?
There was a problem hiding this comment.
To be honest, I'm not sure, but what type of attack could be conducted here?
There was a problem hiding this comment.
Assuming this is implemented in C (which I suspect it is), the C code might be vulnerable to e.g. buffer overflows. I’m not saying it is vulnerable, but it is extra attack surface.
Also, unicode_escape is not a very efficient codec, and results in backslashes (which are special in various places) in strings. What about using base64 instead?
There was a problem hiding this comment.
I convinced that decode is used in many places within Qubes itself and has never been considered non-tursted, so I do not see reason to change that.
Regarding unicode_escape, you're right, which is why you have the string sanitation right below it (lines 227-229), which filters out any malicious characters.
There was a problem hiding this comment.
I convinced that
decodeis used in many places within Qubes itself and has never been considered non-tursted, so I do not see reason to change that.
decode calls into different C functions depending on what codec is used, so it is possible that it is safe with one codec (such as ascii) but not with another (such as unicode_escape). The code in CPython actually has undefined behavior (out-of-bounds pointer arithmetic) on strings such as \x due to an incorrect bounds check (s+1 < end instead of end - s > 1), but otherwise looks okay. Making sure the string does not end with \x should be enough to prevent the undefined behavior.
There was a problem hiding this comment.
The concern is that malicious input might be able to attack the C implementation of unicode_escape before the character set filtering happens. Character set filtering will not protect against memory corruption that happens earlier.
Do you want me to provide a simple pure-Python reimplementation?
There was a problem hiding this comment.
I don't think it's a realistic concern.
There was a problem hiding this comment.
But semantic-wise, what's the intended supported encoding? if just \xXX escaping, then maybe it really should support just this one?
There was a problem hiding this comment.
That would be my preference indeed.
There was a problem hiding this comment.
I implemented a simple parser in pure Python
qubesusbproxy/core3ext.py
Outdated
| result = {"vendor": unknown, | ||
| "product": unknown, | ||
| "manufacturer": unknown, | ||
| "name": unknown, | ||
| "serial": unknown} |
There was a problem hiding this comment.
I noticed that this does not include the port the device is attached through. Qubes Manager will need this to display a GUI asking the user what device they plugged into a port.
There was a problem hiding this comment.
This is only data provided by the device itself (possibly fake/malicious), the port number is contained within 'ident' and is independent of the device itself (the device does not inform us which port it is connected to).
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
aefc84f
to
2fe6691
Compare
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
2fe6691
to
14a9406
Compare
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024062013-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024052808-4.3&flavor=update
Failed tests12 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/101100#dependencies 37 fixed
Unstable tests
|
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
505e4f8
to
7896afb
Compare
qubesusbproxy/core3ext.py
Outdated
| untrusted_device_desc = untrusted_device_desc.decode( | ||
| 'unicode_escape', errors='ignore') | ||
| return ''.join( | ||
| c if c in set(safe_chars) else '_' for c in untrusted_device_desc |
There was a problem hiding this comment.
Please move set out of the loop.
qubesusbproxy/core3ext.py
Outdated
| untrusted_device_desc = untrusted_device_desc.decode( | ||
| 'unicode_escape', errors='ignore') |
There was a problem hiding this comment.
I looked at the underlying C code, and it does not give me confidence. It’s probably correct, but I would not trust it to be safe with malicious input. For instance, it supports \N{NAME} escapes, where NAME is the name of a Unicode character.
I recommend one of the following, in order of preference:
- Switch to a different codec, such as base64.
- Reimplement the functionality that is actually needed in pure Python.
|
Integration tests need an update (see a long list of failures in the updated bot comment above). |
|
test_010_assign still fails, this is what I got from logs: But I'm not sure what is wrong, I do see |
qubesusbproxy/core3ext.py
Outdated
| device = assignment.device | ||
| await self.on_device_attach_usb(vm, '', device, options={}) | ||
| for assignment in vm.devices['usb'].get_assigned_devices(): | ||
| asyncio.ensure_future(self.attach_and_notify( |
There was a problem hiding this comment.
I think this should be await instead of asyncio.ensure_future and that's what the test detected (see the "Task exception was never retrieved" part). And the actual attach probably failed because test already started cleaning up (but that's just my guess)...
|
@piotrbartman there is an issue changing core3ext.py... Currently this repo is using the same branch for all qubes releases (R4.1, R4.2, R4.3, ...), so merging this change into main will break R4.2. Do you see some way to make it compatible with R4.2 (some if, condition on importing device_protocol or something) and be not too ugly? Or maybe the endpoint for device plugins needs to be renamed when changing the API (see setup.py)? |
device-added:usb device-removed:usb
fix device-list-change events
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
5c41479
to
5cb2a8f
Compare
qubesusbproxy/core3ext.py
Outdated
| untrusted_device_desc = untrusted_device_desc.decode( | ||
| 'unicode_escape', errors='ignore') |
There was a problem hiding this comment.
The concern is that malicious input might be able to attack the C implementation of unicode_escape before the character set filtering happens. Character set filtering will not protect against memory corruption that happens earlier.
Do you want me to provide a simple pure-Python reimplementation?
qubesusbproxy/core3ext.py
Outdated
| if i >= len(untrusted_device_desc): | ||
| break | ||
| hex_code = untrusted_device_desc[i - 1: i + 1] | ||
| c = chr(int(hex_code, base=16)) |
There was a problem hiding this comment.
Add a check that these are actually hex digits?
| hex_value = int(hex_code, 16) | ||
| c = chr(hex_value) |
There was a problem hiding this comment.
I’d do an explicit check for both characters being hex digits. int() is rather loose.
|
|
||
| result = "" | ||
| i = 0 | ||
| while i < len(untrusted_device_desc): |
There was a problem hiding this comment.
This loop never ends on "Test\x20device"
qubesusbproxy/core3ext.py
Outdated
| break | ||
| hex_code = untrusted_device_desc[i - 1: i + 1] | ||
| try: | ||
| for i in range(2): |
after QubesOS/qubes-core-admin/pull/579
QubesOS/qubes-issues#4626