Fix path traversal vulnerabilities#31808
Conversation
|
@burmistrzak No but wouldn't be possible without a separate vulnerability in the container runtime.. Arguably the external converters/external extensions interfaces are RCE vectors by design, so fixing the path traversal alone is perhaps ignoring the elephant in the room. Yeah a |
|
Should always reject, not sometimes reject, sometimes replace, for more consistent UX. Should use a project-wide pattern per intended use (e.g. filename - extension can be validated separately). Should not ignore coverage (and if unreachable, code is not needed). Although, there are plenty of other cases that would still allow bypass (e.g. null bytes, unicodes, a bunch of OS-specific stuff, also using If a bad user has enough access to even contemplate doing any of this, there is a much bigger security issue in the setup (out-of-scope of Z2M). A path traversal at that point, would be the last of the worries, since the user would have access to the entire smart home... A software like Z2M must be secured from untrusted users at the setup level; anything untrusted below that, opens up unending security issues, since the whole point is to manage a network of devices. |
Taking this position would require a significant public change of stance, such as:
Right now a regular user has no reason to suspect that Z2M deployed as per documentation would give immediate remote code execution as your own user (assuming you followed the standard "Linux" or "Windows" installation sections) - a scenario which is a whole lot different from allowing others to mess up their Zigbee network, and is the kind of thing that triggers should trigger immediate CVE and public security advisory. |
The base64 image regex used a greedy `.+` for the type capture, letting values like `data:image/png/../../foo;base64,...` flow into path.join() in saveBase64DeviceIcon. Anchor the regex on the data URI structure and replace the post-hoc extension Set with a single mime → file-extension lookup that matchBase64File consults at parse time. The lookup, not the regex, gates which types are accepted, so the regex only needs to identify the data URI structure. The lookup also corrects two latent bugs in the previous shape: the `[A-Za-z0-9]+` extension class rejected the standard `image/svg+xml` mime while accepting non-standard `image/svg`, and `image/jpeg` was saved as `.jpeg` rather than the conventional `.jpg`.
The `name` field on bridge/request/{converter,extension}/{save,remove}
flowed unvalidated into path.join(basePath, name), letting MQTT clients
write JS or delete files outside the external_* directories.
Add a shared `resolveSafeChildPath(base, name)` helper that resolves
the target and asserts it is a direct child of base. Relies on Node's
path normalization to handle traversal, absolute paths, null bytes,
unicode, and OS quirks uniformly. Use it in externalJS.getFilePath,
combined with a separate `path.extname` allowlist for .js/.cjs/.mjs.
Includes regression tests for traversed, sub-directory, non-JS, ".."
and absolute-path names.
payload.hex.file_name on bridge/request/device/ota_update/{update,
schedule} flowed unvalidated into join(baseDir, fileName), letting
MQTT clients write attacker-controlled firmware bytes to arbitrary
paths. Resolve the target via the shared resolveSafeChildPath helper
and reject anything that isn't a direct child of <data>/ota. Wrap the
writeFirmwareHexToDataDir calls in try/catch so the validation throw
surfaces as an error response on the bridge topic instead of crashing
the handler.
Replace the unschedule rmSync prefix check with the same helper.
A string-based startsWith with path.sep is bypassable by storing a
traversed payload.url ("<data>/ota/../../etc/passwd") at schedule
time: the prefix matches but the syscall resolves outside the OTA
dir. resolveSafeChildPath normalizes via path.resolve and rejects
anything whose parent isn't <data>/ota.
Includes regression tests for ".." / "." / "../escape.hex" file_name
on update + schedule, and a traversal-url unschedule case.
filips
force-pushed
the
fix-path-traversal
branch
from
c2d4db5 to
ddfe782
Compare
|
I have pushed fixes adressing the comments so far. |
|
I'd agree with the overall sentiment of @kennylevinsen's comment. Smart home is essentially (becoming) critical infrastructure at a massive scale and being able to compromise (or hold for ransom) thousand of installations is no good. It takes just one supply-chain attack on e.g. an unrelated HA app that uses MQTT to potentially pwn an entire home. People reasonably assume that e.g. their Zigbee-based IAS is still secure when switching to an open source solution. |
|
I think you missed the point I was trying to make. There are several notes here that are out of scope, or not properly described. For example, "remote code execution" is only "remote" if you allow public access, else it is just local code execution as intended for customs (extensions/converters). I'll create a docs PR and we can continue the discussion there instead of crowding this PR with unrelated stuff. |
4 participants
These vulnerabilities let you access (read/write/delete) arbitrary files that the user running z2m has access to, provided you have access to MQTT directly or alternatively an unauthenticated frontend.
Each commit has more details about the issue it addresses.