Extract files by node version order && support 'force' arg to overwri… #43
Conversation
| # Byte-compiled / optimized / DLL files | ||
| __pycache__/ | ||
| *.py[cod] |
There was a problem hiding this comment.
This is out of scope of this pull request. Either remove it or move it into a dedicated commit.
| def is_safe_path(basedir, path, follow_symlinks=True): | ||
| if follow_symlinks: | ||
| matchpath = os.path.realpath(path) | ||
| else: | ||
| matchpath = os.path.abspath(path) | ||
| return basedir == os.path.commonpath((basedir, matchpath)) | ||
|
|
There was a problem hiding this comment.
Why would you remove this ? It was introduced in 41f654e to protect againt path traversal through malicious JFFS2.
There was a problem hiding this comment.
I patched this file based on the old commit, so it doesn't include your new code. I'll close this PR
There was a problem hiding this comment.
Do you plan on submitting a PR based on the current tree ? Otherwise I can create a PR based on your code fix for version handling. Your call :)
There was a problem hiding this comment.
That would be better if you create a PR and fix it with some of my code. Because now you are the maintner of jefferson. Your safe patch is incompatible with my "Firmware Analyzer". It reports path traversal errors when I use normal firmware.
| class Jffs2_raw_xattr(cstruct.CStruct): | ||
| __byte_order__ = cstruct.LITTLE_ENDIAN | ||
| __def__ = """ | ||
| struct { | ||
| jint16_t magic; | ||
| jint16_t nodetype; /* = JFFS2_NODETYPE_XATTR */ | ||
| jint32_t totlen; | ||
| jint32_t hdr_crc; | ||
| jint32_t xid; /* XATTR identifier number */ | ||
| jint32_t version; | ||
| uint8_t xprefix; | ||
| uint8_t name_len; | ||
| jint16_t value_len; | ||
| jint32_t data_crc; | ||
| jint32_t node_crc; | ||
| uint8_t data[0]; | ||
| } | ||
| """ | ||
|
|
||
| class Jffs2_raw_summary(cstruct.CStruct): | ||
| __byte_order__ = cstruct.LITTLE_ENDIAN | ||
| __def__ = """ | ||
| struct { | ||
| jint16_t magic; | ||
| jint16_t nodetype; /* = JFFS2_NODETYPE_SUMMARY */ | ||
| jint32_t totlen; | ||
| jint32_t hdr_crc; | ||
| jint32_t sum_num; /* number of sum entries*/ | ||
| jint32_t cln_mkr; /* clean marker size, 0 = no cleanmarker */ | ||
| jint32_t padded; /* sum of the size of padding nodes */ | ||
| jint32_t sum_crc; /* summary information crc */ | ||
| jint32_t node_crc; /* node crc */ | ||
| jint32_t sum[0]; /* inode summary info */ | ||
| } | ||
| """ | ||
|
|
||
|
|
||
| class Jffs2_raw_xref(cstruct.CStruct): | ||
| __byte_order__ = cstruct.LITTLE_ENDIAN | ||
| __def__ = """ | ||
| struct { | ||
| jint16_t magic; | ||
| jint16_t nodetype; /* = JFFS2_NODETYPE_XREF */ | ||
| jint32_t totlen; | ||
| jint32_t hdr_crc; | ||
| jint32_t ino; /* inode number */ | ||
| jint32_t xid; /* XATTR identifier number */ | ||
| jint32_t xseqno; /* xref sequencial number */ | ||
| jint32_t node_crc; | ||
| } | ||
| """ |
There was a problem hiding this comment.
Why re-introduce those structures ? Quoting the commit messages, we removed them because:
These nodes are never used to reconstruct the filesystem so we can just skip them to make jefferson faster.
| Jffs2_raw_summary = Jffs2_raw_summary.parse( | ||
| Jffs2_raw_summary.__def__, | ||
| __name__=Jffs2_raw_summary.__name__, | ||
| __byte_order__=endianness, | ||
| ) | ||
|
|
||
| Jffs2_raw_xattr = Jffs2_raw_xattr.parse( | ||
| Jffs2_raw_xattr.__def__, | ||
| __name__=Jffs2_raw_xattr.__name__, | ||
| __byte_order__=endianness, | ||
| ) | ||
|
|
||
| Jffs2_raw_xref = Jffs2_raw_xref.parse( | ||
| Jffs2_raw_xref.__def__, | ||
| __name__=Jffs2_raw_xref.__name__, | ||
| __byte_order__=endianness, | ||
| ) |
There was a problem hiding this comment.
Not needed because those node types are not used to reconstruct the filesystem on disk.
| fs[JFFS2_NODETYPE_XATTR] = {} | ||
| fs[JFFS2_NODETYPE_XREF] = {} | ||
| fs[JFFS2_NODETYPE_SUMMARY] = [] |
There was a problem hiding this comment.
Not needed because those node types are not used to reconstruct the filesystem on disk.
| xref = Jffs2_raw_xref() | ||
| xref.unpack(content_mv[0 + offset :]) | ||
|
|
||
| if xref.ino in fs[JFFS2_NODETYPE_XREF]: | ||
| if xref.version > fs[JFFS2_NODETYPE_XREF][xref.ino].version: | ||
| fs[JFFS2_NODETYPE_XREF][xref.ino] = xref | ||
| else: | ||
| fs[JFFS2_NODETYPE_XREF][xref.ino] = xref | ||
| if verbose: | ||
| print("0x%08X:" % (offset), xref) |
There was a problem hiding this comment.
Not needed because those node types are not used to reconstruct the filesystem on disk. We parse structures that are never used afterwards.
| xattr = Jffs2_raw_xattr() | ||
| xattr.unpack(content_mv[0 + offset :]) | ||
|
|
||
| if xattr.ino in fs[JFFS2_NODETYPE_XATTR]: | ||
| if xattr.version > fs[JFFS2_NODETYPE_XATTR][xattr.ino].version: | ||
| fs[JFFS2_NODETYPE_XATTR][xattr.ino] = xattr | ||
| else: | ||
| fs[JFFS2_NODETYPE_XATTR][xattr.ino] = xattr | ||
| if verbose: | ||
| print("0x%08X:" % (offset), xattr) |
There was a problem hiding this comment.
Not needed because those node types are not used to reconstruct the filesystem on disk. We parse structures that are never used afterwards.
| summary = Jffs2_raw_summary() | ||
| summary.unpack(content_mv[0 + offset :]) | ||
| fs[JFFS2_NODETYPE_SUMMARY].append(summary) | ||
| if verbose: | ||
| print("0x%08X:" % (offset), summary) |
There was a problem hiding this comment.
Not needed because those node types are not used to reconstruct the filesystem on disk. We parse structures that are never used afterwards.
| if not is_safe_path(target, target_path): | ||
| print(f"Path traversal attempt to {target_path}, discarding.") | ||
| continue |
There was a problem hiding this comment.
Any reason behind this removal ?
| @@ -485,7 +585,7 @@ def main(): | |||
| if not os.path.exists(dest_path): | |||
| os.mkdir(dest_path) | |||
|
|
|||
| dump_fs(fs, dest_path) | |||
| dump_fs(fs, dest_path, verbose=args.verbose, force=args.force) | |||
There was a problem hiding this comment.
Please move the addition of verbose mode into its own commit, same thing for force mode. We're trying to use atomic git commits.
|
Thanks for submitting your first PR to jefferson, I just did a first review pass :) |
|
@gorgiaxx I fixed the bug you reported and released version 0.4 so users can track which version to use. It was a regression introduced in a recent commit. I also created #45 and #46 to keep track of verbosity and force overwrite option that you wanted to introduce with this MR. Thanks again for reporting this ! |
According to this document. The blocks should be sorted by
inode.version.https://sourceware.org/jffs2/jffs2-slides-transformed.pdf