interfaces: recover panics when sanitizing plugs and slots #3208
Conversation
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This patch switches the interface repository to the new pair of
Sanitize{Plug,Slot} functions that recover panics if the interface
sanitization code panics.
The interface code is processing user input and like it was found in the
following forum thread, it is not immune to errors. This code will
prevent denial-of-service attacks where snapd would otherwise crash when
presented with a potentially malicious snap.
Forum: https://forum.snapcraft.io/t/snapd-service-doesnt-start/319/5
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
There was a problem hiding this comment.
I think this is a good idea, it saves us in cases where a bug in sanitize could paralyze snapd. However I think that it will delay a problem to a later stage (when we do connect), and we will inevitably panic anyway, becacuse addConnectedPlug/Slot method will most likely have similar issue to that of Sanitize (or it will panic explicitely because of missing sanitization).
The code in this PR looks fine, but with the above in mind I think we should in addition either: mark the interface as "broken" and do not attempt to connect it later, or handle panics on connect too. What do you think?
|
@stolowski but if this code fails it will return an error so sanitize won't work so we won't even add the corresponding plug/slot. No way to connect to something that doesn't exist. |
|
This needs a more detailed conversation, as it's turning the system into an exception mechanism, which panics are not meant to be. Why this and not every single function we call? Closing for the time being. Let's discuss elsewhere. |
This branch makes snapd more robust against denial of service attacks in case the interface code that is responsible for sanitization of user-controlled plug/slot definition itself panics.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com