Fix path traversal vulnerability.

[qkaiser]

Hi !

We've been working on various enhancements to ubi_reader at https://github.com/IoT-Inspector/ubi_reader/, one of them is a security fix for potential path traversals.

ubireader_extract_files could lead to path traversal when run against specifically crafted UBIFS files, allowing the attacker to overwrite files outside of the extraction directory (provided the process has write access to that file or directory).

This is due to the fact that a node name (dent_node.name) is considered trusted and joined to the extraction directory path during processing, then the node content is written to that joined path. By crafting a malicious UBIFS file with node names holding path traversal payloads (e.g. ../../tmp/outside.txt), we can make ubi_reader write outside of the extraction directory.

This is problematic, even more so if end users run ubi_reader with elevated privileges in order have the ability to create block devices.

Let me know if you need malicious UBIFS samples for your tests.

[jrspruitt]

Hi QKaiser,

Thank you for pointing this out, I was able to modify an image to demonstrate this, good catch, never thought of it. I am having a problem with the patch though. In is_safe_path, matchpath is an absolute path, while basedir is a relative path causing os.path.commonpath() to error out with "Can't mix absolute and relative paths".

I assume the fix here is giving basedir the same treatment as path.

Also just to note, there was an update recently to version 0.8.0 that fixed a major flaw, where in certain files would not be extracted accurately. Should not conflict with what you have done.

-Jason

[qkaiser]

I'll look into doing proper normalization prior to checking path so there is no mixup error being raised. Will keep you posted.

[qkaiser]

@jrspruitt I fixed the code and simplified the function, let me know if that works for you.

[jrspruitt]

Hi QKaiser,

It worked good, but I had to make one little fix. dent_path is set after is_safe_path() is called, so that if it fails, an error is thrown "extract_files Error: local variable 'dent_path' referenced before assignment" when trying to print the error.

I just had to put dent_path = ... before is_safe_path() like

    dent_path = os.path.realpath(os.path.join(path, dent_node.name))

    if not is_safe_path(path, dent_node.name):
        log(extract_dents, 'path traversal attempt: %s, discarding' % (dent_path))
        return

If you could make that change I can merge your changes, thanks.

-Jason

[qkaiser]

Good catch. I just changed the log call to use dent_node.name given that it's the value we're validating.

[jrspruitt]

Hi QKaiser,
Passes all my tests now, thank you for that.

I'm just wondering now, if this should be an error, which would show regardless of log settings. If someone went through the trouble to make an image like this, there could be more to it, and this would be a first line alert that more precaution could be needed with the contents. Something like

       error(extract_dents, 'Warn', 'Path traversal attempt: %s, discarding' % (dent_node.name))

This would only print a warning, unless error action is set to exit. If you don't want to mess with it, I can fix this in the next commit, I found a similar issue near by I'll need to fix anyhow.

Thank you for this bug fix.
-Jason

[qkaiser]

That makes sense, I edited my branch to use error().

[jrspruitt]

QKaiser thank you very much.

-Jason

[qkaiser]

Hi Jason,

We have an upcoming publication about similar vulnerabilities affecting different extractors in jefferson, yaffshiv, and binwalk. We requested CVEs for each of these vulnerability so that users are aware they should upgrade to the latest version (through dependabot for example).

The one that was fixed in ubi-reader is CVE-2023-0591, you're credited as remediation reviewer :)