show link to symlink target on analysis page #1282
Conversation
jstucke
force-pushed
the
link-symlink-target
branch
from
270b8d8 to
24cf135
Compare
There was a problem hiding this comment.
Overall I am hesitating to merge this as FACT's internals are not really built with support for symlinks in mind as can be seen by the above comments.
Of course, this feature is useful and desirable, but not possible to implement correctly atm.
I'll leave it to you or @dorpvom to merge this.
| for uid in session.execute(query.limit(1)).scalars(): | ||
| return uid |
There was a problem hiding this comment.
I am aware that we, by design, can not get this right, but I'd just like to note that using limit(1) to get a random target file is not necessarily correct.
There was a problem hiding this comment.
Normally, there should only be one result at this point (but there may be so weird edge cases that I haven't thought of)
There was a problem hiding this comment.
To elaborate a bit on the underlying idea: at this point we know the root FW of the file and the path that is mentioned in the file type ("link to '/a/b/c'). The idea is to look in the "virtual path" for paths that line up with this. Usually, this should be only one file. But there are two other possibilities: the file is missing from the file system or there are multiple file systems or containers with this path contained in the firmware. In the latter case, there isn't really a right or wrong result, since the file object represents all of these files at once. We could display all link targets, though.
| full_type = file_obj.processed_analysis['file_type']['result']['full'] | ||
| # if FO is a symlink, file_type analysis "full" will be something like "symbolic link to 'busybox'" | ||
| target_path = full_type.split("'")[1] | ||
| except (IndexError, KeyError): | ||
| return None |
There was a problem hiding this comment.
Again, we can't really do much about this, but this prone to be broken.
For example, it does not work with the link created by ln -s "./crash'es" "foo'bar".
Furthermore, I'd like to note that this is actually not standard file output,
but defined in src/install/internal_symlink_magic.
Edit: You could parse the file according to the logic defined in internal_symlink_magic which would be a bit more correct.
There was a problem hiding this comment.
prone to be broken
This is a bit of an overstatement. I have yet so to come across a file with a single quote in its file name in a firmware image. And even if that happened, we would simply not find the linked file and nothing more would break. Nevertheless, I changed the code so that it won't break even in this case.
| {{ firmware.uid | replace_uid_with_hid(root_uid=root_uid) | safe }}<br /> | ||
| {{ firmware.uid | replace_uid_with_hid(root_uid=root_uid) | safe }} | ||
| {% if link_target %} | ||
| ({{ link_target | safe }}) |
There was a problem hiding this comment.
This is not safe:
printf "\n" > '<script>alert("injection");'
ln -s \<script\>alert\(\"injection\"\)\; foobar
tar -cvf x.tar ./*While this does not run altert, it still breaks the whole site.
There was a problem hiding this comment.
Added a html.escape()
jstucke
force-pushed
the
link-symlink-target
branch
from
24cf135 to
ea4c2b5
Compare
jstucke
force-pushed
the
link-symlink-target
branch
from
ea4c2b5 to
e255107
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1282 +/- ##
==========================================
- Coverage 92.48% 91.85% -0.63%
==========================================
Files 379 378 -1
Lines 24115 21163 -2952
==========================================
- Hits 22302 19440 -2862
+ Misses 1813 1723 -90 ☔ View full report in Codecov by Sentry. |
No description provided.