Most of these changes look good in isolation but the "out:" label in seccomp_load_filters() is asked to do too many things. It is currently trying to handle:
- error returns
- normal code flow
- run as a 'real root'
- run via setuid execution
seccomp_load_filters() should be adjusted to die immediately on any error and avoid use of "out" for that case.
Furthermore the extensive efforts to allow testing in non-production environments is very complicated. The calls to getenv() for UBUNTU_CORE_LAUNCHER_DEBUG and UBUNTU_CORE_LAUNCHER_NO_ROOT should instead use secure_getenv().
The other environment variables should probably also be converted to use secure_getenv() as well but they have far less effect on the execution pathways, only on the correctness of the resulting policy. (secure_getenv() may break the current system but this may simply be a reflection that environment variables are being asked to do tasks they shouldn't.)
Here's the full notes I collected; not everything is direct result of this merge proposal, but this seems as good a place as any to put them:
- run_snappy_app_dev_add() does not check fork() error return
- write_string_to_file() doesn't check fclose() error return
- close(2) manpage describes why this is important
- snappy_udev_setup_required() seems very brittle
- why don't all snaps require this anyway?
- main() argv[NR_ARGS] = (char*)binary -- very odd, argv[NR_ARGS]
should not have been modified at any point during the execution, and the
statement uses a comma rather than ; to separate it from the next
statement, exec().
- main() extra arguments passed through argv[] are passed to the executed
program; is this intentional?
- seccomp_load_filters() "snadbox"
- seccomp_load_filters() snprintf() does not check for >= 128 error return
- seccomp_load_filters() line-too-long error handling is awkward; rather
than jumping to out: it should probably just die immediately.
- seccomp_load_filters() line-too-long test could be written simpler
without line length checks and use strchr('\n') instead. (I think both
will fail if the file doesn't end with a new line character.)
- seccomp_load_filters() out: error-handling is far too complicated.
Errors should probably just die() and allow out: for the "normal" exit
path.
- seccomp_load_filters() "@unrestricted" check is awkward -- strncmp()
isn't needed and only confuses the matter -- and hiding "@unrestricted"
several hundred lines into a file means the rules that were processed to
that point are just dropped entirely. IF we're going to allow an
"@unrestricted" escape hatch we should enforce that it is the entire
contents of the file, not just one line in a file.
Most of these changes look good in isolation but the "out:" label in seccomp_ load_filters( ) is asked to do too many things. It is currently trying to handle:
- error returns
- normal code flow
- run as a 'real root'
- run via setuid execution
seccomp_ load_filters( ) should be adjusted to die immediately on any error and avoid use of "out" for that case.
Furthermore the extensive efforts to allow testing in non-production environments is very complicated. The calls to getenv() for UBUNTU_ CORE_LAUNCHER_ DEBUG and UBUNTU_ CORE_LAUNCHER_ NO_ROOT should instead use secure_getenv().
The other environment variables should probably also be converted to use secure_getenv() as well but they have far less effect on the execution pathways, only on the correctness of the resulting policy. (secure_getenv() may break the current system but this may simply be a reflection that environment variables are being asked to do tasks they shouldn't.)
Here's the full notes I collected; not everything is direct result of this merge proposal, but this seems as good a place as any to put them:
- run_snappy_ app_dev_ add() does not check fork() error return to_file( ) doesn't check fclose() error return udev_setup_ required( ) seems very brittle
- write_string_
- close(2) manpage describes why this is important
- snappy_
- why don't all snaps require this anyway?
- main() argv[NR_ARGS] = (char*)binary -- very odd, argv[NR_ARGS]
should not have been modified at any point during the execution, and the
statement uses a comma rather than ; to separate it from the next
statement, exec().
- main() extra arguments passed through argv[] are passed to the executed
program; is this intentional?
- seccomp_ load_filters( ) "snadbox" load_filters( ) snprintf() does not check for >= 128 error return load_filters( ) line-too-long error handling is awkward; rather load_filters( ) line-too-long test could be written simpler load_filters( ) out: error-handling is far too complicated. load_filters( ) "@unrestricted" check is awkward -- strncmp()
- seccomp_
- seccomp_
than jumping to out: it should probably just die immediately.
- seccomp_
without line length checks and use strchr('\n') instead. (I think both
will fail if the file doesn't end with a new line character.)
- seccomp_
Errors should probably just die() and allow out: for the "normal" exit
path.
- seccomp_
isn't needed and only confuses the matter -- and hiding "@unrestricted"
several hundred lines into a file means the rules that were processed to
that point are just dropped entirely. IF we're going to allow an
"@unrestricted" escape hatch we should enforce that it is the entire
contents of the file, not just one line in a file.
Thanks