Comment 8 for bug 1668892

Revision history for this message
Tyler Hicks (tyhicks) wrote : Re: [Bug 1668892] Re: apparmor package upgrades unload all LXD profiles

On 03/12/2017 09:22 AM, Christian Boltz wrote:
> aa-remove-unknown:
> ...
> + else
> + echo "Removing '${profile}'"
> + echo -n "$profile" > "${REMOVE}"
> + fi
> + done
> +
> +# will not catch all errors, but still better than nothing
> +exit $?
>
> I slightly disagree with the "better than nothing" part ;-)
>
> If I didn't miss something, the only thing that could go wrong is
> + echo -n "$profile" > "${REMOVE}"

There's much more that could go wrong. To name a few:

1) Things could go wrong in the profiles_names_list() function
2) Things could go wrong while executing the awk and/or sort binaries
3) Things could go wrong in the awk script itself

Since all of that goop is piped together, none of those errors would be
caught. IMO, It is some pretty bad code that needs to be improved
outside of this security bug.

The reason I don't feel like it needs to be fixed at the same time as
this security bug is because it should fail safely. In other words,
failing would mean that processes are left confined rather than unconfined.

> It would be much better to use something like
> + echo -n "$profile" > "${REMOVE}" || failure=1
> ...
> exit $failure
>
> (obviously, also initialize failure=0 before the loop)

I agree that it would be better. I'll include an updated patch. Thanks
for the review!