-
-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom RAW format conversion tool support #929
Open
fierceX
wants to merge
16
commits into
photoview:master
Choose a base branch
from
fierceX:add_CustomRaw
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
a720468
add custome raw
fierceX ea56250
fix
fierceX 9141661
Fix UI not serving media and tests expecting localhost:3000 as base U…
davidecavestro bd05169
reordered cli arguments for ffmpeg to improve speed to generate thumb…
AndGem e726258
add cross compile support for arm64 to amd64 (#888)
cicadabear 5516c1e
feat: complete PL translation (#909)
DorianMazur 8e81d86
chore(docker-compose): add sqlite variable and volumes example (#851)
alcaprar de0b902
Fix invalid UTC offset during datetime parsing (#823)
yosev 31b836b
move mp4 moov atom to the beginning (#883)
kabakaev 4363047
Don't stop scanning album on media fail (#892)
kkovaletp 76d94f1
Remove login page non-empty password requirement (#828)
subtlepseudonym 4ba309f
Make face detection optional at build-time (#881)
emersion f10694d
Fix audit issues in production (#933)
jordy2254 9c98767
fix
fierceX ef75e3f
Merge branch 'master' into add_CustomRaw
fierceX c0eea6e
fix
fierceX File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this lead to a command injection attack, by crafting a malicious environment variable?
And if so is it a problem?
Just wanted to mention it to hear what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viktorstrate, This is a great catch!
Yes, from a security point of view, this is a critical vulnerability, as we cannot know in advance which executable should be called by the Photoview and rely on the external config (environment variable in this case), which can be modified.
To mitigate this we need to statically resolve the path to the executable at the build time and accept later only the commandline parameters from the variables. This also means that we will need to preinstall all supported executables inside the same image and define paths to them in Dockerfile providing the ability to choose an executable from the predefined list in the scanner options on the Settings page. A similar approach might work for a directly hosted app, where all paths should be provided in some form at the build time, so they are available on the Settings later. If a user wants to add a custom executable (a self-developed one or just something, not tested by us and not supported from the project perspective) in the case of deployment with docker, a local rebuild of the Photoview image will be required.
An alternative way would be to define the AppArmor profile for the Photoview image and allow only a predefined list of executables to be executed inside the container. However, this is more difficult to maintain and would require strong Linux admin skills for a user in the case of direct deployment to a host instead of using docker (which, I believe, is a stop-factor for us, as our target audience is not only Linux gurus but also regular users), while doesn't provide much more flexibility, as we just shift the place of executables hardcoding from the code to the AppArmor profile, so to add a custom executable, the profile should be changed, which requires an image rebuild as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this issue is within the scope of consideration. The prerequisite for attacking through custom environment variables is that the attacker has obtained the permissions of the machine. If the attacker has obtained the permissions of the machine, then the security issue does not require us to consider it. In addition, regarding the docker image, the program and environment variables used for RAW conversion have been pre-installed in the official image. If the program or the value can be artificially modified, the attacker still needs to obtain the permissions of the machine. From what perspective, this should not be an issue that ordinary applications need to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An attacker might get access to the docker container and put there a new malicious executable, then manipulate with env variables to replace the RAW processing one, so that the malicious executable is called instead. In this case, there is no need to have access to the host.
If Photoview is deployed on the host directly, this is the same system, so an attacker has access to the host, as a 1st step.
But in both cases, there are different levels of access: to put a file to the home folder and modify an env variable you don't need admin permissions. The
photoview
user can do that. While to modify an existing executable in the system (owned by root), which is registered in the Photoview app, an attacker should become an admin 1st.Please see the PR #863, where I implemented many security enhancements for our docker image, so now the Photoview app is executed there under non-root user, which has write access to his home folder and mounted app cache (which by default is also inside the home folder). All executables remain owned by root and cannot be modified without root permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an attacker can modify environment variables and restart photoview, he also has permission to directly execute the malicious program. In addition, environment variables can be changed, so even under existing circumstances, the default RAW converter and video converter will still be replaced without requiring root permission. Just modify the PATH environment variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this argument, this PR would not make Photoview any less secure because of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we need to prepare an AppArmor profile, which will at least forbid the execution of any apps from non-standard locations, or allow the execution of only allowed binaries - I need to think about this more...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, if we provide a way to register any external RAW processing app, do we want to provide the same approach for video-converter integration as well?
How does the scanner create thumbnails for non-RAW images: does it use the same RAW-processing app integration? if not, probably, we need to change that as well? I assume that there is a low probability that a user wants to use a non-default app to process RAW files while keeping the standard one for regular images.
@viktorstrate, @jordy2254, @fierceX, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first message I don't think it's something that we need to do unless anyone raises a concern. Ultimatly the attack vector is minimised in a docker environment and on a bare metal install I'd expect people to manage it as they see fit based on there use case.
For the latter message In my opinion out of scope for this piece of work. lets keep things in scope and make additional issues/PRs for them as needed to try and keep risk to a minimum and follow CI/CD better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you refer to the "do we want to provide the same approach for video-converter integration as well?" message, sure, I agree that this might be a new PR. My question was just from a product functionality perspective: do you see the inconsistency between the flexibility of the RAW processing app integration and video / thumbnails converters, possibly some other workers?
This is a good approach to give the user an opportunity to redefine what a 3rd-party app should process the incoming media. I'd like to have it available for all types of workers at the final stage (after all corresponding PRs are merged)