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
cmd/snap-confine: use defensive argument parser #3026
Conversation
This switches snap-confine to the far more tested and resilient argument parser. This fixes a possible attack vector where a malicious application would fork/exec snap-confine with especially crafted argument vector. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
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 very nice, readability is much better than the original. One minor nitpick (a general comment rather) - IMHO it would be good to make even simple if-statements such as:
if (cond)
do_something();
into:
if (cond) {
do_something();
}
It just feels good especially for security-sensitive code (but surely in this particular change it's always followed by die(), so no real risks).
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
cmd/snap-confine/snap-confine.c
Outdated
return 1; | ||
} | ||
// We don't want to handle any other errors, just die if we see one. | ||
sc_die_on_error(err); |
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.
This should be something like this instead:
err = sc_parse_args(&argc, &argv, &args);
sc_die_on_error(err);
The usage error may be an actual error inside the respective logic instead of just a code:
error *usage_error = errorf("Usage: snap-confine <security tag> <executable>")
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.
Done.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The error message is now sufficiently useful to be passed to sc_die_on_erorr(). This lets us remove the extra conditional code in main. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
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.
LGTM. Not sure github will let you land it while gustavo's changes-requested is still there though.
This switches snap-confine to the far more tested and resilient argument
parser. This fixes a possible attack vector where a malicious
application would fork/exec snap-confine with especially crafted
argument vector.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com