| Message ID | 20201113083108.12567-10-sbabic@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Rework and extend IPC for install | expand |
Hi Stefano, Am 13.11.20 um 09:31 schrieb Stefano Babic: > If selection can be set via IPC, it could be possible to activate a > selection that can damage the board. For example, the IPC selects to > install into the running rootfs instead of the stand-by copy. > > Implement a blacklist for selection to block dangerous modes. The > parameter --exclude <selection>,<mode> is added and it can be issued > multiple times. Isn't that error-prone? I would recommend a whitelist instead of a blacklist. Is the complete change backward compatible? What happens if a user upgrade to this version and use an a/b partition? Could an end-user damage the board? Regards Stefan
Hi Stefan, On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: > Hi Stefano, > > Am 13.11.20 um 09:31 schrieb Stefano Babic: >> If selection can be set via IPC, it could be possible to activate a >> selection that can damage the board. For example, the IPC selects to >> install into the running rootfs instead of the stand-by copy. >> >> Implement a blacklist for selection to block dangerous modes. The >> parameter --exclude <selection>,<mode> is added and it can be issued >> multiple times. > > Isn't that error-prone? This is arguable and I have not the best answer. Still today it is error prone if the integrator passes to SWUpdate a wrong selection as command line parameter, for example related to the copy where the software actually runs instead of stand-by. I guess that implementing a whitelist or a blacklist could be a question of taste. I choose for a blacklist because we cannot know if a future release will add additional selections to SWUpdate. We can know which are the accepted selections at the moment we deliver a release (that is the whitelist), but we cannot know if we need new ones in the future and this limits a whitelist. Then my decision to just block selections that are know to be wrong: for example, the selection pointing to the running copy in a A/B concept. > I would recommend a whitelist instead of a > blacklist. > See above : what do you think ? > Is the complete change backward compatible? It is supposed yes to be full backward compatible. From my tests, I have not found any issue running older SWUs. It is supposed to be a new feature that does not change the previous behavior. > What happens if a user > upgrade to this version and use an a/b partition? Could an end-user > damage the board? At the moment, the "dynamic" selection can be activated only via the library, that means via an application that links to the library. Webserver and backend don't sue this feature, and they use the library in "compatibility" mode without setting the selection, that is the selection is set at the startup as it has been before. And to avoid that a buggy application can destroy the board, this blacklist is introduced. The end used has no possibility to choose a selection, so he cannot damage the board if the integrator has done his job correctly (as he should do now). The remarkable thing in the series is that the API of the library changes, that is applications must be converted to the newer API. Regards, Stefano
Hi Stefano, Am 14.11.20 um 12:24 schrieb Stefano Babic: > On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: >> Am 13.11.20 um 09:31 schrieb Stefano Babic: >>> If selection can be set via IPC, it could be possible to activate a >>> selection that can damage the board. For example, the IPC selects to >>> install into the running rootfs instead of the stand-by copy. >>> >>> Implement a blacklist for selection to block dangerous modes. The >>> parameter --exclude <selection>,<mode> is added and it can be issued >>> multiple times. >> >> Isn't that error-prone? > > This is arguable and I have not the best answer. Still today it is error > prone if the integrator passes to SWUpdate a wrong selection as command > line parameter, for example related to the copy where the software > actually runs instead of stand-by. I guess that implementing a whitelist > or a blacklist could be a question of taste. > > I choose for a blacklist because we cannot know if a future release will > add additional selections to SWUpdate. We can know which are the > accepted selections at the moment we deliver a release (that is the > whitelist), but we cannot know if we need new ones in the future and > this limits a whitelist. Then my decision to just block selections that > are know to be wrong: for example, the selection pointing to the running > copy in a A/B concept. > >> I would recommend a whitelist instead of a blacklist. >> > > See above : what do you think ? After the update every integrator of A/B have to provide a blacklist. How you want to inform the integrator about it? >> Is the complete change backward compatible? > It is supposed yes to be full backward compatible. From my tests, I have > not found any issue running older SWUs. It is supposed to be a new > feature that does not change the previous behavior. Could somebody damage the system if I use a A/B and doesn't provide a blacklist? Until now it isn't possible because even a compromise web server could not change the selection. If I update to the next version I have to provide a blacklist to get the same security. Maybe the feature should be explicitly enabled by the integrator at compile- or run-time. >> What happens if a user >> upgrade to this version and use an a/b partition? Could an end-user >> damage the board? > > At the moment, the "dynamic" selection can be activated only via the > library, that means via an application that links to the library. Or any code that have access to the socket. > Webserver and backend don't sue this feature, and they use the library > in "compatibility" mode without setting the selection, that is the > selection is set at the startup as it has been before. And to avoid that > a buggy application can destroy the board, this blacklist is introduced. > > The end used has no possibility to choose a selection, so he cannot > damage the board if the integrator has done his job correctly (as he > should do now). But the integrator need to know this feature. Do you plan to add a warning or info to the A/B examples? > The remarkable thing in the series is that the API of the library > changes, that is applications must be converted to the newer API. The security change. After this change every ipc user could change the selection. Regards Stefan
Hi Stefan, On 14.11.20 13:09, Stefan Herbrechtsmeier wrote: > Hi Stefano, > > Am 14.11.20 um 12:24 schrieb Stefano Babic: >> On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: >>> Am 13.11.20 um 09:31 schrieb Stefano Babic: >>>> If selection can be set via IPC, it could be possible to activate a >>>> selection that can damage the board. For example, the IPC selects to >>>> install into the running rootfs instead of the stand-by copy. >>>> >>>> Implement a blacklist for selection to block dangerous modes. The >>>> parameter --exclude <selection>,<mode> is added and it can be issued >>>> multiple times. >>> >>> Isn't that error-prone? >> >> This is arguable and I have not the best answer. Still today it is >> error prone if the integrator passes to SWUpdate a wrong selection as >> command line parameter, for example related to the copy where the >> software actually runs instead of stand-by. I guess that implementing >> a whitelist or a blacklist could be a question of taste. >> >> I choose for a blacklist because we cannot know if a future release >> will add additional selections to SWUpdate. We can know which are the >> accepted selections at the moment we deliver a release (that is the >> whitelist), but we cannot know if we need new ones in the future and >> this limits a whitelist. Then my decision to just block selections >> that are know to be wrong: for example, the selection pointing to the >> running copy in a A/B concept. >> >>> I would recommend a whitelist instead of a blacklist. >>> >> >> See above : what do you think ? > > After the update every integrator of A/B have to provide a blacklist. Why ? It shouldn't be required. > How you want to inform the integrator about it? Well, let's make the example with a dunfell out with 2020.04 and we push a gatesgarth with new swupdate and tgis feature. Really, if there is no custom application using the library, nothing should be done. > >>> Is the complete change backward compatible? >> It is supposed yes to be full backward compatible. From my tests, I >> have not found any issue running older SWUs. It is supposed to be a >> new feature that does not change the previous behavior. > > Could somebody damage the system if I use a A/B and doesn't provide a > blacklist? Until now it isn't possible because even a compromise web > server could not change the selection. If I update to the next version I > have to provide a blacklist to get the same security. > > Maybe the feature should be explicitly enabled by the integrator at > compile- or run-time. This can be done, sure. I have just thought about if it is strictly required or if there is a degrade of security. In fact, we have already that the access to the socket must be controlled, but this can be done by setting access rights to it. And still today, if an application has enough rights, it could already mishandle the device. It can simply restart SWUpdate with a different -e <software>,<mode> parameter. Also, I had the same thought as you, but I finish to admit that there is no really changes because if some piece of malware runs with enough right on the device, everything can happen and a "rm -rf /" is then easier as damaging the device via SWUpdate. Anyway, sure this feature could be enabled by a CONFIG_ > >>> What happens if a user >>> upgrade to this version and use an a/b partition? Could an end-user >>> damage the board? >> >> At the moment, the "dynamic" selection can be activated only via the >> library, that means via an application that links to the library. > > Or any code that have access to the socket. Right, but we have already this case. This is a UDS, not a network socket, and if this happens, your device is already compromised. > >> Webserver and backend don't sue this feature, and they use the library >> in "compatibility" mode without setting the selection, that is the >> selection is set at the startup as it has been before. And to avoid >> that a buggy application can destroy the board, this blacklist is >> introduced. >> >> The end used has no possibility to choose a selection, so he cannot >> damage the board if the integrator has done his job correctly (as he >> should do now). > > But the integrator need to know this feature. Well, it is thought to be compatible. Sure, an integrator can use it by setting in the start up script for swupdate something like -e <standby copy> --exclude <running copy> But I do not think we improve compared to 2020.04. > Do you plan to add a > warning or info to the A/B examples? I can add it, of course, > >> The remarkable thing in the series is that the API of the library >> changes, that is applications must be converted to the newer API. > > The security change. After this change every ipc user could change the > selection. This is correct - but this means each IPC user in a custom application making using of the library. Webserver and backend *don't* change the selection. And yes, if they will need in future, strict rules should be added to avoid the scenarios you mention. Regards, Stefano
Hi Stefano, Am 14.11.20 um 15:50 schrieb Stefano Babic: > On 14.11.20 13:09, Stefan Herbrechtsmeier wrote: >> Am 14.11.20 um 12:24 schrieb Stefano Babic: >>> On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: >>>> Am 13.11.20 um 09:31 schrieb Stefano Babic: >>>>> If selection can be set via IPC, it could be possible to activate a >>>>> selection that can damage the board. For example, the IPC selects to >>>>> install into the running rootfs instead of the stand-by copy. >>>>> >>>>> Implement a blacklist for selection to block dangerous modes. The >>>>> parameter --exclude <selection>,<mode> is added and it can be issued >>>>> multiple times. >>>> >>>> Isn't that error-prone? >>> >>> This is arguable and I have not the best answer. Still today it is >>> error prone if the integrator passes to SWUpdate a wrong selection as >>> command line parameter, for example related to the copy where the >>> software actually runs instead of stand-by. I guess that implementing >>> a whitelist or a blacklist could be a question of taste. >>> >>> I choose for a blacklist because we cannot know if a future release >>> will add additional selections to SWUpdate. We can know which are the >>> accepted selections at the moment we deliver a release (that is the >>> whitelist), but we cannot know if we need new ones in the future and >>> this limits a whitelist. Then my decision to just block selections >>> that are know to be wrong: for example, the selection pointing to the >>> running copy in a A/B concept. >>> >>>> I would recommend a whitelist instead of a blacklist. >>>> >>> >>> See above : what do you think ? >> >> After the update every integrator of A/B have to provide a blacklist. > > Why ? It shouldn't be required. > >> How you want to inform the integrator about it? > > Well, let's make the example with a dunfell out with 2020.04 and we push > a gatesgarth with new swupdate and tgis feature. Really, if there is no > custom application using the library, nothing should be done. > >> >>>> Is the complete change backward compatible? >>> It is supposed yes to be full backward compatible. From my tests, I >>> have not found any issue running older SWUs. It is supposed to be a >>> new feature that does not change the previous behavior. >> >> Could somebody damage the system if I use a A/B and doesn't provide a >> blacklist? Until now it isn't possible because even a compromise web >> server could not change the selection. If I update to the next version I >> have to provide a blacklist to get the same security. >> >> Maybe the feature should be explicitly enabled by the integrator at >> compile- or run-time. > > This can be done, sure. I have just thought about if it is strictly > required or if there is a degrade of security. In fact, we have already > that the access to the socket must be controlled, but this can be done > by setting access rights to it. And still today, if an application has > enough rights, it could already mishandle the device. It can simply > restart SWUpdate with a different -e <software>,<mode> parameter. Also, > I had the same thought as you, but I finish to admit that there is no > really changes because if some piece of malware runs with enough right > on the device, everything can happen and a "rm -rf /" is then easier as > damaging the device via SWUpdate. > > Anyway, sure this feature could be enabled by a CONFIG_ > >> >>>> What happens if a user >>>> upgrade to this version and use an a/b partition? Could an end-user >>>> damage the board? >>> >>> At the moment, the "dynamic" selection can be activated only via the >>> library, that means via an application that links to the library. >> >> Or any code that have access to the socket. > > Right, but we have already this case. This is a UDS, not a network > socket, and if this happens, your device is already compromised. > >> >>> Webserver and backend don't sue this feature, and they use the library >>> in "compatibility" mode without setting the selection, that is the >>> selection is set at the startup as it has been before. And to avoid >>> that a buggy application can destroy the board, this blacklist is >>> introduced. >>> >>> The end used has no possibility to choose a selection, so he cannot >>> damage the board if the integrator has done his job correctly (as he >>> should do now). >> >> But the integrator need to know this feature. > > Well, it is thought to be compatible. Sure, an integrator can use it by > setting in the start up script for swupdate something like > -e <standby copy> --exclude <running copy> > > But I do not think we improve compared to 2020.04. > >> Do you plan to add a >> warning or info to the A/B examples? > > I can add it, of course, > >> >>> The remarkable thing in the series is that the API of the library >>> changes, that is applications must be converted to the newer API. >> >> The security change. After this change every ipc user could change the >> selection. > > This is correct - but this means each IPC user in a custom application > making using of the library. Webserver and backend *don't* change the > selection. And yes, if they will need in future, strict rules should be > added to avoid the scenarios you mention. My concern is the web server. What happens if we have an error in the web server and somebody could inject code? Doesn't this feature increase the attack vector for everybody even if they don't need the feature? I would expect that sooner or later somebody add this feature to the web server because of similar use cases. Regards Stefan
Hi Stefan, On 14.11.20 17:04, Stefan Herbrechtsmeier wrote: > Hi Stefano, > > Am 14.11.20 um 15:50 schrieb Stefano Babic: >> On 14.11.20 13:09, Stefan Herbrechtsmeier wrote: >>> Am 14.11.20 um 12:24 schrieb Stefano Babic: >>>> On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: >>>>> Am 13.11.20 um 09:31 schrieb Stefano Babic: >>>>>> If selection can be set via IPC, it could be possible to activate a >>>>>> selection that can damage the board. For example, the IPC selects to >>>>>> install into the running rootfs instead of the stand-by copy. >>>>>> >>>>>> Implement a blacklist for selection to block dangerous modes. The >>>>>> parameter --exclude <selection>,<mode> is added and it can be issued >>>>>> multiple times. >>>>> >>>>> Isn't that error-prone? >>>> >>>> This is arguable and I have not the best answer. Still today it is >>>> error prone if the integrator passes to SWUpdate a wrong selection as >>>> command line parameter, for example related to the copy where the >>>> software actually runs instead of stand-by. I guess that implementing >>>> a whitelist or a blacklist could be a question of taste. >>>> >>>> I choose for a blacklist because we cannot know if a future release >>>> will add additional selections to SWUpdate. We can know which are the >>>> accepted selections at the moment we deliver a release (that is the >>>> whitelist), but we cannot know if we need new ones in the future and >>>> this limits a whitelist. Then my decision to just block selections >>>> that are know to be wrong: for example, the selection pointing to the >>>> running copy in a A/B concept. >>>> >>>>> I would recommend a whitelist instead of a blacklist. >>>>> >>>> >>>> See above : what do you think ? >>> >>> After the update every integrator of A/B have to provide a blacklist. >> >> Why ? It shouldn't be required. >> >>> How you want to inform the integrator about it? >> >> Well, let's make the example with a dunfell out with 2020.04 and we push >> a gatesgarth with new swupdate and tgis feature. Really, if there is no >> custom application using the library, nothing should be done. >> >>> >>>>> Is the complete change backward compatible? >>>> It is supposed yes to be full backward compatible. From my tests, I >>>> have not found any issue running older SWUs. It is supposed to be a >>>> new feature that does not change the previous behavior. >>> >>> Could somebody damage the system if I use a A/B and doesn't provide a >>> blacklist? Until now it isn't possible because even a compromise web >>> server could not change the selection. If I update to the next version I >>> have to provide a blacklist to get the same security. >>> >>> Maybe the feature should be explicitly enabled by the integrator at >>> compile- or run-time. >> >> This can be done, sure. I have just thought about if it is strictly >> required or if there is a degrade of security. In fact, we have already >> that the access to the socket must be controlled, but this can be done >> by setting access rights to it. And still today, if an application has >> enough rights, it could already mishandle the device. It can simply >> restart SWUpdate with a different -e <software>,<mode> parameter. Also, >> I had the same thought as you, but I finish to admit that there is no >> really changes because if some piece of malware runs with enough right >> on the device, everything can happen and a "rm -rf /" is then easier as >> damaging the device via SWUpdate. >> >> Anyway, sure this feature could be enabled by a CONFIG_ >> >>> >>>>> What happens if a user >>>>> upgrade to this version and use an a/b partition? Could an end-user >>>>> damage the board? >>>> >>>> At the moment, the "dynamic" selection can be activated only via the >>>> library, that means via an application that links to the library. >>> >>> Or any code that have access to the socket. >> >> Right, but we have already this case. This is a UDS, not a network >> socket, and if this happens, your device is already compromised. >> >>> >>>> Webserver and backend don't sue this feature, and they use the library >>>> in "compatibility" mode without setting the selection, that is the >>>> selection is set at the startup as it has been before. And to avoid >>>> that a buggy application can destroy the board, this blacklist is >>>> introduced. >>>> >>>> The end used has no possibility to choose a selection, so he cannot >>>> damage the board if the integrator has done his job correctly (as he >>>> should do now). >>> >>> But the integrator need to know this feature. >> >> Well, it is thought to be compatible. Sure, an integrator can use it by >> setting in the start up script for swupdate something like >> -e <standby copy> --exclude <running copy> >> >> But I do not think we improve compared to 2020.04. >> >>> Do you plan to add a >>> warning or info to the A/B examples? >> >> I can add it, of course, >> >>> >>>> The remarkable thing in the series is that the API of the library >>>> changes, that is applications must be converted to the newer API. >>> >>> The security change. After this change every ipc user could change the >>> selection. >> >> This is correct - but this means each IPC user in a custom application >> making using of the library. Webserver and backend *don't* change the >> selection. And yes, if they will need in future, strict rules should be >> added to avoid the scenarios you mention. > > My concern is the web server. Ok > What happens if we have an error in the > web server and somebody could inject code? Well, if someone is able to inject code into the Webserver, he could also start an IPC to attack the device. > Doesn't this feature increase > the attack vector for everybody even if they don't need the feature? As far as one attacker can attack and replace coe in the webserver, yes. I see the point. Let's see what I can do. I can implement according to your suggestion a whitelist. The number of configurations is then fixed, and the worst case is just that additional "selection" can be added in more as one release. It does not seem a big issue. This seems enough (as you have already stated). I could also take the second suggestion (just allow to change <software> and not <mode>), so that an update to the running copy is not possible. But as feeling the solution with whitelist is more flexible. I could also add a switch (I think about a command line parameter), so that this feature is enabled only if activated at start-up. Default remains disable (so no changes at all from current behavior), and an integrator must set it to enable. Even if not set at build time, this is safe enough (same security as now). What do you think about ? > > I would expect that sooner or later somebody add this feature to the web > server because of similar use cases. Yes, agree, this use case could be requested in future from someone else to update from web. Best regards, Stefano
Hi Stefano, Am 14.11.20 um 17:51 schrieb Stefano Babic: > On 14.11.20 17:04, Stefan Herbrechtsmeier wrote: >> Am 14.11.20 um 15:50 schrieb Stefano Babic: >>> On 14.11.20 13:09, Stefan Herbrechtsmeier wrote: >>>> Am 14.11.20 um 12:24 schrieb Stefano Babic: >>>>> On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: >>>>>> Am 13.11.20 um 09:31 schrieb Stefano Babic: >>>>>>> If selection can be set via IPC, it could be possible to activate a >>>>>>> selection that can damage the board. For example, the IPC selects to >>>>>>> install into the running rootfs instead of the stand-by copy. >>>>>>> >>>>>>> Implement a blacklist for selection to block dangerous modes. The >>>>>>> parameter --exclude <selection>,<mode> is added and it can be issued >>>>>>> multiple times. >>>>>> >>>>>> Isn't that error-prone? >>>>> >>>>> This is arguable and I have not the best answer. Still today it is >>>>> error prone if the integrator passes to SWUpdate a wrong selection as >>>>> command line parameter, for example related to the copy where the >>>>> software actually runs instead of stand-by. I guess that implementing >>>>> a whitelist or a blacklist could be a question of taste. >>>>> >>>>> I choose for a blacklist because we cannot know if a future release >>>>> will add additional selections to SWUpdate. We can know which are the >>>>> accepted selections at the moment we deliver a release (that is the >>>>> whitelist), but we cannot know if we need new ones in the future and >>>>> this limits a whitelist. Then my decision to just block selections >>>>> that are know to be wrong: for example, the selection pointing to the >>>>> running copy in a A/B concept. >>>>> >>>>>> I would recommend a whitelist instead of a blacklist. >>>>>> >>>>> >>>>> See above : what do you think ? >>>> >>>> After the update every integrator of A/B have to provide a blacklist. >>> >>> Why ? It shouldn't be required. >>> >>>> How you want to inform the integrator about it? >>> >>> Well, let's make the example with a dunfell out with 2020.04 and we push >>> a gatesgarth with new swupdate and tgis feature. Really, if there is no >>> custom application using the library, nothing should be done. >>> >>>> >>>>>> Is the complete change backward compatible? >>>>> It is supposed yes to be full backward compatible. From my tests, I >>>>> have not found any issue running older SWUs. It is supposed to be a >>>>> new feature that does not change the previous behavior. >>>> >>>> Could somebody damage the system if I use a A/B and doesn't provide a >>>> blacklist? Until now it isn't possible because even a compromise web >>>> server could not change the selection. If I update to the next >>>> version I >>>> have to provide a blacklist to get the same security. >>>> >>>> Maybe the feature should be explicitly enabled by the integrator at >>>> compile- or run-time. >>> >>> This can be done, sure. I have just thought about if it is strictly >>> required or if there is a degrade of security. In fact, we have already >>> that the access to the socket must be controlled, but this can be done >>> by setting access rights to it. And still today, if an application has >>> enough rights, it could already mishandle the device. It can simply >>> restart SWUpdate with a different -e <software>,<mode> parameter. Also, >>> I had the same thought as you, but I finish to admit that there is no >>> really changes because if some piece of malware runs with enough right >>> on the device, everything can happen and a "rm -rf /" is then easier as >>> damaging the device via SWUpdate. >>> >>> Anyway, sure this feature could be enabled by a CONFIG_ >>> >>>> >>>>>> What happens if a user >>>>>> upgrade to this version and use an a/b partition? Could an end-user >>>>>> damage the board? >>>>> >>>>> At the moment, the "dynamic" selection can be activated only via the >>>>> library, that means via an application that links to the library. >>>> >>>> Or any code that have access to the socket. >>> >>> Right, but we have already this case. This is a UDS, not a network >>> socket, and if this happens, your device is already compromised. >>> >>>> >>>>> Webserver and backend don't sue this feature, and they use the library >>>>> in "compatibility" mode without setting the selection, that is the >>>>> selection is set at the startup as it has been before. And to avoid >>>>> that a buggy application can destroy the board, this blacklist is >>>>> introduced. >>>>> >>>>> The end used has no possibility to choose a selection, so he cannot >>>>> damage the board if the integrator has done his job correctly (as he >>>>> should do now). >>>> >>>> But the integrator need to know this feature. >>> >>> Well, it is thought to be compatible. Sure, an integrator can use it by >>> setting in the start up script for swupdate something like >>> -e <standby copy> --exclude <running copy> >>> >>> But I do not think we improve compared to 2020.04. >>> >>>> Do you plan to add a >>>> warning or info to the A/B examples? >>> >>> I can add it, of course, >>> >>>> >>>>> The remarkable thing in the series is that the API of the library >>>>> changes, that is applications must be converted to the newer API. >>>> >>>> The security change. After this change every ipc user could change the >>>> selection. >>> >>> This is correct - but this means each IPC user in a custom application >>> making using of the library. Webserver and backend *don't* change the >>> selection. And yes, if they will need in future, strict rules should be >>> added to avoid the scenarios you mention. >> >> My concern is the web server. > > Ok > >> What happens if we have an error in the web server and somebody could >> inject code? > > Well, if someone is able to inject code into the Webserver, he could > also start an IPC to attack the device. > >> Doesn't this feature increase the attack vector for everybody even if >> they don't need the feature? > > As far as one attacker can attack and replace coe in the webserver, yes. > I see the point. Let's see what I can do. > > I can implement according to your suggestion a whitelist. The number of > configurations is then fixed, and the worst case is just that additional > "selection" can be added in more as one release. It does not seem a big > issue. This seems enough (as you have already stated). > > I could also take the second suggestion (just allow to change <software> > and not <mode>), so that an update to the running copy is not possible. > But as feeling the solution with whitelist is more flexible. > > I could also add a switch (I think about a command line parameter), so > that this feature is enabled only if activated at start-up. Default > remains disable (so no changes at all from current behavior), and an > integrator must set it to enable. Even if not set at build time, this is > safe enough (same security as now). > > What do you think about ? I would use a command line switch. We don't know how the selection is used and maybe our assumption doesn't fit and we have a problem if somebody use the <software> to select between production and development images. If we use a command line switch the blacklist should be easy to maintenance. >> I would expect that sooner or later somebody add this feature to the >> web server because of similar use cases. > > Yes, agree, this use case could be requested in future from someone else > to update from web. Regards Stefan
Hi Stefan, On 14.11.20 18:51, Stefan Herbrechtsmeier wrote: > Hi Stefano, > > Am 14.11.20 um 17:51 schrieb Stefano Babic: >> On 14.11.20 17:04, Stefan Herbrechtsmeier wrote: >>> Am 14.11.20 um 15:50 schrieb Stefano Babic: >>>> On 14.11.20 13:09, Stefan Herbrechtsmeier wrote: >>>>> Am 14.11.20 um 12:24 schrieb Stefano Babic: >>>>>> On 14.11.20 12:00, Stefan Herbrechtsmeier wrote: >>>>>>> Am 13.11.20 um 09:31 schrieb Stefano Babic: >>>>>>>> If selection can be set via IPC, it could be possible to activate a >>>>>>>> selection that can damage the board. For example, the IPC >>>>>>>> selects to >>>>>>>> install into the running rootfs instead of the stand-by copy. >>>>>>>> >>>>>>>> Implement a blacklist for selection to block dangerous modes. The >>>>>>>> parameter --exclude <selection>,<mode> is added and it can be >>>>>>>> issued >>>>>>>> multiple times. >>>>>>> >>>>>>> Isn't that error-prone? >>>>>> >>>>>> This is arguable and I have not the best answer. Still today it is >>>>>> error prone if the integrator passes to SWUpdate a wrong selection as >>>>>> command line parameter, for example related to the copy where the >>>>>> software actually runs instead of stand-by. I guess that implementing >>>>>> a whitelist or a blacklist could be a question of taste. >>>>>> >>>>>> I choose for a blacklist because we cannot know if a future release >>>>>> will add additional selections to SWUpdate. We can know which are the >>>>>> accepted selections at the moment we deliver a release (that is the >>>>>> whitelist), but we cannot know if we need new ones in the future and >>>>>> this limits a whitelist. Then my decision to just block selections >>>>>> that are know to be wrong: for example, the selection pointing to the >>>>>> running copy in a A/B concept. >>>>>> >>>>>>> I would recommend a whitelist instead of a blacklist. >>>>>>> >>>>>> >>>>>> See above : what do you think ? >>>>> >>>>> After the update every integrator of A/B have to provide a blacklist. >>>> >>>> Why ? It shouldn't be required. >>>> >>>>> How you want to inform the integrator about it? >>>> >>>> Well, let's make the example with a dunfell out with 2020.04 and we >>>> push >>>> a gatesgarth with new swupdate and tgis feature. Really, if there is no >>>> custom application using the library, nothing should be done. >>>> >>>>> >>>>>>> Is the complete change backward compatible? >>>>>> It is supposed yes to be full backward compatible. From my tests, I >>>>>> have not found any issue running older SWUs. It is supposed to be a >>>>>> new feature that does not change the previous behavior. >>>>> >>>>> Could somebody damage the system if I use a A/B and doesn't provide a >>>>> blacklist? Until now it isn't possible because even a compromise web >>>>> server could not change the selection. If I update to the next >>>>> version I >>>>> have to provide a blacklist to get the same security. >>>>> >>>>> Maybe the feature should be explicitly enabled by the integrator at >>>>> compile- or run-time. >>>> >>>> This can be done, sure. I have just thought about if it is strictly >>>> required or if there is a degrade of security. In fact, we have already >>>> that the access to the socket must be controlled, but this can be done >>>> by setting access rights to it. And still today, if an application has >>>> enough rights, it could already mishandle the device. It can simply >>>> restart SWUpdate with a different -e <software>,<mode> parameter. Also, >>>> I had the same thought as you, but I finish to admit that there is no >>>> really changes because if some piece of malware runs with enough right >>>> on the device, everything can happen and a "rm -rf /" is then easier as >>>> damaging the device via SWUpdate. >>>> >>>> Anyway, sure this feature could be enabled by a CONFIG_ >>>> >>>>> >>>>>>> What happens if a user >>>>>>> upgrade to this version and use an a/b partition? Could an end-user >>>>>>> damage the board? >>>>>> >>>>>> At the moment, the "dynamic" selection can be activated only via the >>>>>> library, that means via an application that links to the library. >>>>> >>>>> Or any code that have access to the socket. >>>> >>>> Right, but we have already this case. This is a UDS, not a network >>>> socket, and if this happens, your device is already compromised. >>>> >>>>> >>>>>> Webserver and backend don't sue this feature, and they use the >>>>>> library >>>>>> in "compatibility" mode without setting the selection, that is the >>>>>> selection is set at the startup as it has been before. And to avoid >>>>>> that a buggy application can destroy the board, this blacklist is >>>>>> introduced. >>>>>> >>>>>> The end used has no possibility to choose a selection, so he cannot >>>>>> damage the board if the integrator has done his job correctly (as he >>>>>> should do now). >>>>> >>>>> But the integrator need to know this feature. >>>> >>>> Well, it is thought to be compatible. Sure, an integrator can use it by >>>> setting in the start up script for swupdate something like >>>> -e <standby copy> --exclude <running copy> >>>> >>>> But I do not think we improve compared to 2020.04. >>>> >>>>> Do you plan to add a >>>>> warning or info to the A/B examples? >>>> >>>> I can add it, of course, >>>> >>>>> >>>>>> The remarkable thing in the series is that the API of the library >>>>>> changes, that is applications must be converted to the newer API. >>>>> >>>>> The security change. After this change every ipc user could change the >>>>> selection. >>>> >>>> This is correct - but this means each IPC user in a custom application >>>> making using of the library. Webserver and backend *don't* change the >>>> selection. And yes, if they will need in future, strict rules should be >>>> added to avoid the scenarios you mention. >>> >>> My concern is the web server. >> >> Ok >> >>> What happens if we have an error in the web server and somebody could >>> inject code? >> >> Well, if someone is able to inject code into the Webserver, he could >> also start an IPC to attack the device. >> >>> Doesn't this feature increase the attack vector for everybody even if >>> they don't need the feature? >> >> As far as one attacker can attack and replace coe in the webserver, >> yes. I see the point. Let's see what I can do. >> >> I can implement according to your suggestion a whitelist. The number >> of configurations is then fixed, and the worst case is just that >> additional "selection" can be added in more as one release. It does >> not seem a big issue. This seems enough (as you have already stated). >> >> I could also take the second suggestion (just allow to change >> <software> and not <mode>), so that an update to the running copy is >> not possible. But as feeling the solution with whitelist is more >> flexible. >> >> I could also add a switch (I think about a command line parameter), so >> that this feature is enabled only if activated at start-up. Default >> remains disable (so no changes at all from current behavior), and an >> integrator must set it to enable. Even if not set at build time, this >> is safe enough (same security as now). >> >> What do you think about ? > > I would use a command line switch. We don't know how the selection is > used and maybe our assumption doesn't fit and we have a problem if > somebody use the <software> to select between production and development > images. If we use a command line switch the blacklist should be easy to > maintenance. Ok, I'll post a V3. Thanks for your feedback ! Best regards, Stefano
diff --git a/core/network_thread.c b/core/network_thread.c index 6757cc4..f40b99b 100644 --- a/core/network_thread.c +++ b/core/network_thread.c @@ -56,6 +56,36 @@ static unsigned long nrmsgs = 0; static pthread_mutex_t msglock = PTHREAD_MUTEX_INITIALIZER; +static bool is_selection_allowed(const char *software_set, char *running_mode, + struct dict const *excludedlist) +{ + char *swset = NULL; + struct dict_list *sets; + struct dict_list_elem *selection; + bool allowed = true; + + if (!strlen(software_set)) + return true; + if (ENOMEM_ASPRINTF == + asprintf(&swset, "%s,%s", software_set, running_mode)) { + ERROR("OOM generating selection string"); + return false; + } + sets = dict_get_list((struct dict *)excludedlist, "excluded"); + if (sets && swset) { + LIST_FOREACH(selection, sets, next) { + if (!strcmp(swset, selection->value)) { + allowed = false; + ERROR("Selection %s not allowed, rejected !", + swset); + } + } + free(swset); + } + + return allowed; +} + static void clean_msg(char *msg, char drop) { char *lfpos; @@ -349,17 +379,22 @@ void *network_thread (void *data) if (instp->status == IDLE) { instp->fd = ctrlconnfd; instp->req = msg.data.instmsg.req; + if (is_selection_allowed(instp->req.software_set, + instp->req.running_mode, + &instp->software->excluded_set)) { + /* + * Prepare answer + */ + msg.type = ACK; + + /* Drop all old notification from last run */ + cleanum_msg_list(); + + /* Wake-up the installer */ + pthread_cond_signal(&stream_wkup); + } else + msg.type = NACK; - /* - * Prepare answer - */ - msg.type = ACK; - - /* Drop all old notification from last run */ - cleanum_msg_list(); - - /* Wake-up the installer */ - pthread_cond_signal(&stream_wkup); } else { msg.type = NACK; sprintf(msg.data.msg, "Installation in progress"); diff --git a/core/swupdate.c b/core/swupdate.c index 49b0461..e966aca 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -76,6 +76,7 @@ static struct option long_options[] = { {"loglevel", required_argument, NULL, 'l'}, {"syslog", no_argument, NULL, 'L' }, {"select", required_argument, NULL, 'e'}, + {"excluded-select", required_argument, NULL, 'q'}, {"output", required_argument, NULL, 'o'}, {"dry-run", no_argument, NULL, 'n'}, {"no-downgrading", required_argument, NULL, 'N'}, @@ -661,7 +662,7 @@ int main(int argc, char **argv) #endif memset(main_options, 0, sizeof(main_options)); memset(image_url, 0, sizeof(image_url)); - strcpy(main_options, "vhni:e:l:Lcf:p:P:o:N:R:M"); + strcpy(main_options, "vhni:e:q:l:Lcf:p:P:o:N:R:M"); #ifdef CONFIG_MTD strcat(main_options, "b:"); #endif @@ -858,6 +859,9 @@ int main(int argc, char **argv) if (opt_to_hwrev(optarg, &swcfg.hw) < 0) exit(EXIT_FAILURE); break; + case 'q': + dict_insert_value(&swcfg.excluded_set, "excluded", optarg); + break; #ifdef CONFIG_SURICATTA case 'u': if (asprintf(&suricattaoptions,"%s %s", argv[0], optarg) == diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst index 306f2ac..fadc62b 100644 --- a/doc/source/swupdate.rst +++ b/doc/source/swupdate.rst @@ -442,6 +442,13 @@ Command line parameters | | | -e "stable, copy1" ==> install on copy1 | | | | -e "stable, copy2" ==> install on copy2 | +-------------+----------+--------------------------------------------+ +| --excluded | string | ``sel`` is in the format <software>,<mode>.| +| <sel> | | It sets a blacklist of selections that | +| | | cannot be used for an update. | +| | | Selections can be activated not only with | +| | | -e, but also via IPC. | +| | | Multiple --excluded are allowed | ++-------------+----------+--------------------------------------------+ | -h | - | Run usage with help. | +-------------+----------+--------------------------------------------+ | -k <file> | string | Available if CONFIG_SIGNED is set. | diff --git a/include/swupdate.h b/include/swupdate.h index 4ae0b43..506d28d 100644 --- a/include/swupdate.h +++ b/include/swupdate.h @@ -158,6 +158,7 @@ struct swupdate_cfg { struct imglist scripts; struct imglist bootscripts; struct dict bootloader; + struct dict excluded_set; struct proclist extprocs; void *dgst; /* Structure for signed images */ struct swupdate_global_cfg globals;
If selection can be set via IPC, it could be possible to activate a selection that can damage the board. For example, the IPC selects to install into the running rootfs instead of the stand-by copy. Implement a blacklist for selection to block dangerous modes. The parameter --exclude <selection>,<mode> is added and it can be issued multiple times. Signed-off-by: Stefano Babic <sbabic@denx.de> --- core/network_thread.c | 55 +++++++++++++++++++++++++++++++++-------- core/swupdate.c | 6 ++++- doc/source/swupdate.rst | 7 ++++++ include/swupdate.h | 1 + 4 files changed, 58 insertions(+), 11 deletions(-)