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
nixos/etc: sanitize /etc setup script #41276
Conversation
nixos/modules/system/etc/make-etc.sh
Outdated
modes_=($modes) | ||
users_=($users) | ||
groups_=($groups) | ||
eval "sources_=($sources)" |
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.
What does the value of $sources looks like in that case?
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.
So this brought to my attention that $sources actually shouldn't be escaped, because it's a list of paths, that when escaped don't get properly imported into the store. It also means the script is passed a list of nix store paths, which are sanitized anyway.
For the others, specifically $target, assuming we have the files foo
and bar baz
, it would like 'foo' 'bar baz'
, whereas previously it was foo bar baz
.
The eval is safe, as it expands to an array literal of string literals.
While fixing the sanitization of $sources, I realised that I'd missed the globbing section of the script. On looking into it, the globbing section is a) inherently unsanitised, and b) impossible to reach, as it requires a nix store path to have a |
Wait, sorry, it still seems to be in use! E.g. In fail2ban. |
This reverts commit 910bab8.
What is the status of this pull request? |
Thank you for your contributions.
|
I think the underlying issue has never been fixed, so somebody who is more familiar with this low level of the nixos module system should revisit it and hopefully merge it |
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.
Looks like some of these changes have already made it into make-etc.sh
but otherwise LGTM.
Can't we just set edit: ah no |
you can use |
I went ahead with the |
I marked this as stale due to inactivity. → More info |
Fixes #41241
Motivation for this change
Variables passed to the
/etc
setup script were not shell escaped. This has been changed.I am not entirely comfortable with the use of
eval
to construct arrays from the sanitised input, but I haven't found a better approach to convert a string of shell-escaped arguments into an array of strings in bash.I have tested it myself, however I am not familiar enough with nixos tests to know which ones need to be run for something as low-level as this, and I'm well aware that any change in the
/etc
setup could break a lot. Any advice on this?Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)