Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-confine: add snap-confine command line parser module #2416
Conversation
zyga
added some commits
Dec 2, 2016
Some checks were not successful
All checks have passed
zyga
added
the
Critical
label
Dec 6, 2016
| + "cannot parse arguments, argc or argv are NULL"); | ||
| + goto out; | ||
| + } | ||
| + // Use local copies of argc and argv. |
niemeyer
Dec 6, 2016
Contributor
This message is misleading. This is a local copy of a pointer to an array. Any changes you make to that array are going straight into the real array, and if you change the pointer you are changing a local variable as usual. There's no "copying".
| + if (args->executable == NULL) { | ||
| + die("cannot allocate memory for executable name"); | ||
| + } | ||
| + // No more positional arguments are required. |
niemeyer
Dec 6, 2016
Contributor
The outcome of this logic is a bit awkward. It accepts snap-confine --version foo bar and snap-confine foo --version bar but not snap-confine foo bar --version, with the latter actually handing off the option into the confined command. It seems more clear to be posix-strict here: flags first, positional arguments later. No flags accepted after we observe the first positional argument.
zyga
Dec 6, 2016
Contributor
Good point, we don't need mixed flags/positionals in any way. I'll simplify the code to handle this.
| + argv[i] = NULL; | ||
| + | ||
| + // Write the updated argc and argv back. | ||
| + *argvp = argv; |
niemeyer
Dec 6, 2016
Contributor
Why? That's exactly the same pointer that the code took out. There were no updates to that pointer.
zyga
Dec 6, 2016
Contributor
Ah, blast from the past, it used to be updated :). I'll get rid of this, thanks!
| + *ptr = NULL; | ||
| +} | ||
| + | ||
| +bool sc_args_is_version_query(struct sc_args *args) |
niemeyer
Dec 6, 2016
Contributor
It's not clear why we need all those helpers. If we cannot allocate arguments, we should return an error right then, instead of going through with an unallocated struct and checking that things are not completely broken on every call site.
zyga
Dec 6, 2016
Contributor
If we cannot allocate memory we do fail instantly with an error message.
Those helpers are there in case someone, later, after various changes and evolution of this code, makes a mistake and manages to call those with NULL pointers. To be extra security paranoid I always check for that.
zyga
added some commits
Dec 7, 2016
All checks have passed
zyga
removed
the
Critical
label
Dec 7, 2016
zyga
referenced this pull request
in snapcore/snap-confine
Dec 7, 2016
Closed
Add module for parsing command line arguments. #200
Some checks were not successful
|
This is meant to replace the hacky parser in #2427 |
zyga
requested a review
from
jdstrand
Dec 9, 2016
zyga
added some commits
Jan 17, 2017
Some checks were not successful
All checks have passed
| + | ||
| + if (argcp == NULL || argvp == NULL) { | ||
| + err = sc_error_init(SC_ARGS_DOMAIN, 0, | ||
| + "cannot parse arguments, argc or argv are NULL"); |
chipaca
Jan 18, 2017
Member
in seriousness, this is a minuscule it, but if you need to submit another commit for any reason, maybe change those to &argc and &argv if you don't want argcp and argvp.
chipaca
Jan 18, 2017
Member
Also: why cannot $FOO, $REASON! instead of cannot $FOO: $REASON! in this file?
zyga
Jan 18, 2017
Contributor
I'll answer the last comment first, no reason, I need to review what the common way is and just apply that everywhere.
zyga
Jan 18, 2017
Contributor
As for argc and argv vs argcp and argvp -- good catch! I'll correct that.
| + // When this happens we want to skip the first positional argument as it is | ||
| + // the security tag repeated (legacy). | ||
| + bool ignore_first_tag = false; | ||
| + char *basename = strrchr(argv[0], '/'); |
chipaca
Jan 18, 2017
Member
So all it takes is to
execl("snap-confine", NULL);to segfault this? Interesting...
zyga
Jan 18, 2017
Contributor
Are you sure? I think I check for argc size above. https://github.com/snapcore/snapd/pull/2416/files/19c093cfbbb28775ef98801038fb89092d1dedb4#diff-5c0eca39cb68ec44020b045836523bbaR51
| + // NOTE: this is safe because we, at most, may move to the NUL byte | ||
| + // that compares to an empty string. | ||
| + basename += 1; | ||
| + if (strcmp(basename, "ubuntu-core-launcher") == 0) { |
zyga
Jan 18, 2017
Contributor
Yeah, I would love to add that to to string-utils.[ch]. Note that this branch pre-dates your pull request. I will gladly refactor it to simplify this later.
| + // | ||
| + // NOTE: optind is not reset, we just continue from where we left off in | ||
| + // the loop above. | ||
| + for (; optind < argc; ++optind) { |
chipaca
Jan 18, 2017
Member
How is this different from
if (ignore_first_tag) optind++;
args->security_tag = strdup(argv[optind++]);
args->executable = strdup(argv[optind++]);(skipping error checks for clarity). You put yourself in a loop of length two, with an if (first) {...} else if (second) {...} as the only body?
| + int i; | ||
| + done: | ||
| + // "shift" the argument vector left, except for argv[0], to "consume" the | ||
| + // arguments that were scanned / parsed correctly. |
zyga
Jan 18, 2017
Contributor
Because of argv[0]. Also because this is what various other parsers do and (maybe some black magic) I wanted to stay in line.
|
to be clear, the "changes requested" is about the use of |
| +}; | ||
| + | ||
| +struct sc_args *sc_nonfatal_parse_args(int *argcp, char ***argvp, | ||
| + struct sc_error **errorp) |
stolowski
Jan 18, 2017
Contributor
Would it make more sense to make the function return sc_error (and return sc_args by value-argument of the function) instead and mark the function with attribute ((warn_unused_result)) to force the caller to do something about the error?
zyga
Jan 18, 2017
Contributor
No, because it is still valid to pass a NULL errorp here and anywhere else (the behavior is predictable and correct) and this might actually be more convenient. Otherwise if you actually want to die you force everyone to collect the error, check it and then die. While in this specific function I don't think this is useful, as a general theme it feels wrong.
zyga
added some commits
Jan 18, 2017
All checks have passed
zyga
added some commits
Jan 20, 2017
All checks have passed
tyhicks
reviewed
Jan 20, 2017
A few minor suggestions/questions except for the question about why getopt(3) is not used. It'd be great if you could answer that ASAP. Thanks!
| + "cannot parse arguments, argc is zero or argv is NULL"); | ||
| + goto out; | ||
| + } | ||
| + // Sanity check, look for NULL argv entries. |
tyhicks
Jan 20, 2017
Contributor
What's the motivation for this sanity check? I'm trying to decide if it is worthwhile to sanity check argv without sanity checking argc.
zyga
Jan 25, 2017
Contributor
Interesting observation. This was in response to a question from @chipaca (that we don't check for NULL arguments). I think you are right. Since we don't (and cannot really) check argc in a reasonable way, checking argv is equally unreasonable.
I was looking if there's a way to attack snap-confine by exec'ing it with a malicious argument vector. Suggestions are very much welcome.
tyhicks
Jan 27, 2017
Contributor
I think ensuring that argc meets a minimum value (whatever the appropriate amount is for progname and minimum number of required args) is as good of a check as you're going to get.
| + ignore_first_tag = true; | ||
| + } | ||
| + } | ||
| + // Parse option switches. |
tyhicks
Jan 20, 2017
Contributor
I'm going to hold off on reviewing this loop until I hear back on this.
zyga
Jan 25, 2017
Contributor
Mostly flexibility. All the various command line parsers have their quirks and we are often asked to have precise semantics. It also lets us be sure that there's nothing we're missing as to how this simple but critical part behaves.
tyhicks
Jan 27, 2017
Contributor
Ok. I think libc's built in parser is likely to be secure and I find it simple to use but I can appreciate the desire to shed some of the unneeded features that it has.
| + return args->is_classic_confinement; | ||
| +} | ||
| + | ||
| +char *sc_args_security_tag(struct sc_args *args) |
tyhicks
Jan 20, 2017
Contributor
Do you want callers of this function to be able to modify the security_tag string? I think returning a const char * would be cleaner.
| + return args->security_tag; | ||
| +} | ||
| + | ||
| +char *sc_args_executable(struct sc_args *args) |
| + * The return value may be NULL if snap-confine was invoked with --version. It | ||
| + * is never NULL otherwise. | ||
| + * | ||
| + * The return value doesn't need to be freed(). It is bound to the lifetime of |
tyhicks
Jan 20, 2017
Contributor
I say "must not" because if some future caller frees the pointer, then sc_args_free() will trigger a double-free. Might as well be explicit in the function documentation.
| + * The return value may be NULL if snap-confine was invoked with --version. It | ||
| + * is never NULL otherwise. | ||
| + * | ||
| + * The return value doesn't need to be freed(). It is bound to the lifetime of |
zyga
added some commits
Jan 25, 2017
All checks have passed
tyhicks
approved these changes
Jan 27, 2017
This looks good to me. I'll restate my preference of using getopt(3) but your implementation is clean.
zyga commentedDec 6, 2016
This branch adds a dedicated, tested module for snap-confine argument parser. It will soon replace the ad-hoc parsing code that was done in
main()