Do not crash on a null in the configuration - #2234
Merged
Merged
Conversation
The array of arguments built for "crun exec" was one element too long, and its length field was set to the whole argc, not to the number of arguments actually copied. The loop ran once past the last argument, reading argv[argc], which is the terminating NULL of argv, so the extra element ended up as a NULL in the middle of an array of strings, and args_len counted the crun arguments (the global options, the "exec" itself and the container id) along with the command. Nothing dereferenced the extra element so far, since the array is passed to execv(), which stops at the NULL, but the length is wrong for anything that walks the array. Copy exactly the arguments that follow the container id. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A JSON null where the configuration expects a string is parsed into a
NULL pointer, which is right: a NULL is how a field that is not present
is represented everywhere else in the parsed document. The code walking
these values, though, took a C string for granted. A config.json with
"mounts": [{"destination": "/dev", "options": ["nosuid", null]}]
made get_idmapped_option() call strlen() on the NULL and die:
$ crun run nullopt
Segmentation fault (core dumped)
Check for a NULL where such a value is used. An option of a mount, which
is free-form, is skipped, as an option that is not there; a value that
selects a behavior -- a seccomp flag or architecture, a scheduler flag, an
environment variable, a sysctl -- is rejected with EINVAL, the way an
unknown value in the same place already is. An annotation whose value is
null is left out of the state, which cannot hold a null.
Five of the loops over the options of a mount call
get_mount_flags_or_option(), so the check for those lives there.
Found by fuzzing, then by putting a null in each string of a
configuration in turn.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Rather than a case per field, build a configuration that touches as many string-valued fields as possible, then put a null in each of its strings in turn -- 87 of them as it stands -- and run a container with it. The runtime has to report what is wrong instead of dying. A container process that dies on a NULL leaves the runtime with a broken channel and no error of its own, which is what the test looks for besides a signal; built with the sanitizers, the same test also reports the undefined behavior of handing a NULL to a function declared not to take one. On the tree before the previous commit the test names 37 offending fields. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
A JSON null where the configuration expects a string is parsed into a NULL
pointer, which is how libocispec represents a field that is not present.
The code walking these values took a C string for granted, so a document
that is merely invalid, rather than malicious, killed the runtime:
The first commit is an unrelated bug found on the way: the array of
arguments built for
crun execwas one element too long, and its lengthcounted the crun arguments along with the command.
The second commit checks for a NULL where such a value is used, and what
it does depends on what the value means. An option of a mount is
free-form, so a null one is skipped, as an option that is not there; a
value that selects a behavior -- a seccomp flag or architecture, a
scheduler flag, an environment variable, a sysctl -- is rejected with
EINVAL, the way an unknown value in the same place already is; an
annotation whose value is null is left out of the state, which cannot hold
a null. Five of the loops over the options of a mount go through
get_mount_flags_or_option(), so the check for those lives there.The third commit is a test that builds a configuration touching as many
string-valued fields as possible and puts a null in each of its strings in
turn -- 87 of them as it stands -- expecting the runtime to report what is
wrong instead of dying. On the tree before the fix it names 37 offending
fields.
A container process that dies on a NULL leaves the runtime with a broken
channel and no error of its own, which is what the test looks for besides
a signal. Built with the sanitizers, the same test also reports the
undefined behavior of handing a NULL to a function declared not to take
one, which is how two of the fields (
process.scheduler.flagsandlinux.sysctl) turned up.The first case came from the fuzzer; a two hour run of all ten targets on
this branch, with ASan and UBSan, found nothing.