| Message ID | 1537248227-21218-1-git-send-email-james.hilliard1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Download Endpoint | expand |
We use this feature to implement a manual 2 stage "check for updates" widget in our devices web interface. On our devices the swupdate http server is not directly exposed to the user, it listens only on localhost and requests are relayed through a nginx reverse proxy that validates authentication tokens against our main webapp backend before allowing access to the swupdate endpoints. The user first clicks the "check for updates" button which calls an endpoint on the device which downloads a manifest from our remote update server, this manifest is then returned in the device endpoint response to the user and includes the download url for the swu update file in addition to a changelog. This allows for the user to manually download the swu file to the computer, in addition there is an "update" button which we display in the widget when the device firmware is older than the firmware on our update server, when clicked this button makes an ajax call to this swupdate download endpoint(also routed via the nginx reverse proxy) so that users don't have to download any files to their computer. This endpoint also allows the user to tell the device to update directly from any other source as well such as a local server on their network. Our project requirements call for all firmware updates to be manually initiated from the device web interface for security reasons. On Mon, Sep 17, 2018 at 11:25 PM <james.hilliard1@gmail.com> wrote: > > From: James Hilliard <james.hilliard1@gmail.com> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > mongoose/mongoose_interface.c | 109 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 109 insertions(+) > > diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c > index 2521b30..534a027 100644 > --- a/mongoose/mongoose_interface.c > +++ b/mongoose/mongoose_interface.c > @@ -58,6 +58,61 @@ struct file_upload_state { > static struct mg_serve_http_opts s_http_server_opts; > static void upload_handler(struct mg_connection *nc, int ev, void *p); > > +#ifdef CONFIG_DOWNLOAD > +/* > + * Downloader > + */ > +#include <pthread.h> > +#include "download_interface.h" > +#include "channel.h" > +#include "channel_curl.h" > +#include "globals.h" > + > +struct downloader_thread_argvs{ > + char *url; > +}; > + > +#define DL_LOWSPEED_TIME 300 > + > +#define DL_DEFAULT_RETRIES 3 > + > +#define SETSTRING(p, v) do { \ > + if (p) \ > + free(p); \ > + p = strdup(v); \ > +} while (0) > + > +static channel_data_t channel_options = { > + .source = SOURCE_DOWNLOADER, > + .debug = false, > + .retries = DL_DEFAULT_RETRIES, > + .low_speed_timeout = DL_LOWSPEED_TIME > +}; > + > +static void download_handler(struct mg_connection *nc, int ev, void *p); > +static void *start_download_thread(void *argv); > + > + > +static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) > +{ > + channel_t *channel = channel_new(); > + if (channel->open(channel, channel_data) != CHANNEL_OK) { > + free(channel); > + return FAILURE; > + } > + > + RECOVERY_STATUS result = SUCCESS; > + channel_op_res_t chanresult = channel->get_file(channel, channel_data); > + if (chanresult != CHANNEL_OK) { > + result = FAILURE; > + } > + ipc_wait_for_complete(NULL); > + channel->close(channel); > + free(channel); > + return result; > +} > +#endif > + > /* > * These functions are for V2 of the protocol > */ > @@ -430,6 +485,29 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p) > } > } > > +#ifdef CONFIG_DOWNLOAD > +static void download_handler(struct mg_connection *nc, int ev, void *p) > +{ > + struct http_message *hm = (struct http_message *) p; > + > + char url[1024]; > + int ret = mg_get_http_var(&hm->body, "url", url, sizeof(url)); > + fprintf(stderr,"Received URL %d\n",ret); > + if (ret==-1) { > + mg_http_send_error(nc, 500, NULL); > + } else { > + pthread_t thread_download; > + pthread_create(&thread_download, NULL, start_download_thread, (void *)&url); > + > + mg_send_response_line(nc, 200, > + "Content-Type: text/plain\r\n" > + "Connection: close"); > + mg_send(nc, "\r\n", 2); > + nc->flags |= MG_F_SEND_AND_CLOSE; > + } > +} > +#endif > + > static void ev_handler_v1(struct mg_connection __attribute__ ((__unused__)) *nc, > int __attribute__ ((__unused__)) ev, > void __attribute__ ((__unused__)) *ev_data) > @@ -637,6 +715,9 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) > case MONGOOSE_API_V2: > mg_register_http_endpoint(nc, "/restart", restart_handler); > mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL)); > +#ifdef CONFIG_DOWNLOAD > + mg_register_http_endpoint(nc, "/download", download_handler); > +#endif > mg_start_thread(broadcast_message_thread, &mgr); > mg_start_thread(broadcast_progress_thread, &mgr); > break; > @@ -654,3 +735,31 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) > > return 0; > } > + > +#ifdef CONFIG_DOWNLOAD > +static void *start_download_thread(void *argv) > +{ > + char *url; > + if (argv) { > + url=(char*) argv; > + > + SETSTRING(channel_options.url, url); > + > + optind = 1; > + > + RECOVERY_STATUS result = download_from_url(&channel_options); > + if (result != FAILURE) { > + ipc_message msg; > + if (ipc_postupdate(&msg) != 0) { > + result = FAILURE; > + } else { > + result = msg.type == ACK ? result : FAILURE; > + } > + } > + > + if (channel_options.url != NULL) { > + free(channel_options.url); > + } > + } > +} > +#endif > -- > 2.7.4 >
Hi James, On 18/09/2018 08:06, James Hilliard wrote: > We use this feature to implement a manual 2 stage "check for updates" > widget in our devices web interface. On our devices the swupdate http > server is not directly exposed to the user, it listens only on > localhost and requests are relayed through a nginx reverse proxy that > validates authentication tokens against our main webapp backend before > allowing access to the swupdate endpoints. > > The user first clicks the "check for updates" button which calls an > endpoint on the device which downloads a manifest from our remote > update server, this manifest is then returned in the device endpoint > response to the user and includes the download url for the swu update > file in addition to a changelog. This allows for the user to manually > download the swu file to the computer, in addition there is an > "update" button which we display in the widget when the device > firmware is older than the firmware on our update server, when clicked > this button makes an ajax call to this swupdate download endpoint(also > routed via the nginx reverse proxy) so that users don't have to > download any files to their computer. This endpoint also allows the > user to tell the device to update directly from any other source as > well such as a local server on their network. > > Our project requirements call for all firmware updates to be manually > initiated from the device web interface for security reasons. Ok, thanks for explanation, I got your requirements. However, I disagree with the implementation. You copied a big part of the downloader handler into the webserver interface, and this is quite bad. I am trying to factorize all what I can. Nevertheless, the concept in the patch is quite weird. We have a Webserver interface, that is the download of a SWU is in "push mode" (from outside to the device), but it is forced in someway to go into "pull" mode, mixing upload and download. It depends from CONFIG_DOWNLOAD, too, and this adds libcurl to all builds, even if just webserver is required. And as far as I could see, adding libcurl (and its dependencies) has a big impact on the footprint, causing that most rescue systems cannot fit anymore. A such check of external source with a manifest or whatever is something that should be integrated in the "suricatta" mode instead of the Webserver. I want also to ask if you are aware of some changes in code that should make easier 2 stage update. They are the "dry-run" and the possibility to store the SWU (but yes, on the device, so this could be not interesting for you). The dry-run approach does much more as just parsing a manifest file, because it try to run the whole SWU without installing anything. How do you verify your manifest file ? I guess you had to provide your own stuff to verify a signed manifest. I am also bother about security. You add a "/download" callback, where you pass the URL. A malicious could use it to push an own URL - I guess you masked this in your internal Webserver, but if the patch is merged, the "/download" is open for everybody. Best regards, Stefano Babic > On Mon, Sep 17, 2018 at 11:25 PM <james.hilliard1@gmail.com> wrote: >> >> From: James Hilliard <james.hilliard1@gmail.com> >> >> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >> --- >> mongoose/mongoose_interface.c | 109 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 109 insertions(+) >> >> diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c >> index 2521b30..534a027 100644 >> --- a/mongoose/mongoose_interface.c >> +++ b/mongoose/mongoose_interface.c >> @@ -58,6 +58,61 @@ struct file_upload_state { >> static struct mg_serve_http_opts s_http_server_opts; >> static void upload_handler(struct mg_connection *nc, int ev, void *p); >> >> +#ifdef CONFIG_DOWNLOAD >> +/* >> + * Downloader >> + */ >> +#include <pthread.h> >> +#include "download_interface.h" >> +#include "channel.h" >> +#include "channel_curl.h" >> +#include "globals.h" >> + >> +struct downloader_thread_argvs{ >> + char *url; >> +}; >> + >> +#define DL_LOWSPEED_TIME 300 >> + >> +#define DL_DEFAULT_RETRIES 3 >> + >> +#define SETSTRING(p, v) do { \ >> + if (p) \ >> + free(p); \ >> + p = strdup(v); \ >> +} while (0) >> + >> +static channel_data_t channel_options = { >> + .source = SOURCE_DOWNLOADER, >> + .debug = false, >> + .retries = DL_DEFAULT_RETRIES, >> + .low_speed_timeout = DL_LOWSPEED_TIME >> +}; >> + >> +static void download_handler(struct mg_connection *nc, int ev, void *p); >> +static void *start_download_thread(void *argv); >> + >> + >> +static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) >> +{ >> + channel_t *channel = channel_new(); >> + if (channel->open(channel, channel_data) != CHANNEL_OK) { >> + free(channel); >> + return FAILURE; >> + } >> + >> + RECOVERY_STATUS result = SUCCESS; >> + channel_op_res_t chanresult = channel->get_file(channel, channel_data); >> + if (chanresult != CHANNEL_OK) { >> + result = FAILURE; >> + } >> + ipc_wait_for_complete(NULL); >> + channel->close(channel); >> + free(channel); >> + return result; >> +} >> +#endif >> + >> /* >> * These functions are for V2 of the protocol >> */ >> @@ -430,6 +485,29 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p) >> } >> } >> >> +#ifdef CONFIG_DOWNLOAD >> +static void download_handler(struct mg_connection *nc, int ev, void *p) >> +{ >> + struct http_message *hm = (struct http_message *) p; >> + >> + char url[1024]; >> + int ret = mg_get_http_var(&hm->body, "url", url, sizeof(url)); >> + fprintf(stderr,"Received URL %d\n",ret); >> + if (ret==-1) { >> + mg_http_send_error(nc, 500, NULL); >> + } else { >> + pthread_t thread_download; >> + pthread_create(&thread_download, NULL, start_download_thread, (void *)&url); >> + >> + mg_send_response_line(nc, 200, >> + "Content-Type: text/plain\r\n" >> + "Connection: close"); >> + mg_send(nc, "\r\n", 2); >> + nc->flags |= MG_F_SEND_AND_CLOSE; >> + } >> +} >> +#endif >> + >> static void ev_handler_v1(struct mg_connection __attribute__ ((__unused__)) *nc, >> int __attribute__ ((__unused__)) ev, >> void __attribute__ ((__unused__)) *ev_data) >> @@ -637,6 +715,9 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) >> case MONGOOSE_API_V2: >> mg_register_http_endpoint(nc, "/restart", restart_handler); >> mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL)); >> +#ifdef CONFIG_DOWNLOAD >> + mg_register_http_endpoint(nc, "/download", download_handler); >> +#endif >> mg_start_thread(broadcast_message_thread, &mgr); >> mg_start_thread(broadcast_progress_thread, &mgr); >> break; >> @@ -654,3 +735,31 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) >> >> return 0; >> } >> + >> +#ifdef CONFIG_DOWNLOAD >> +static void *start_download_thread(void *argv) >> +{ >> + char *url; >> + if (argv) { >> + url=(char*) argv; >> + >> + SETSTRING(channel_options.url, url); >> + >> + optind = 1; >> + >> + RECOVERY_STATUS result = download_from_url(&channel_options); >> + if (result != FAILURE) { >> + ipc_message msg; >> + if (ipc_postupdate(&msg) != 0) { >> + result = FAILURE; >> + } else { >> + result = msg.type == ACK ? result : FAILURE; >> + } >> + } >> + >> + if (channel_options.url != NULL) { >> + free(channel_options.url); >> + } >> + } >> +} >> +#endif >> -- >> 2.7.4 >> >
On Tue, Sep 18, 2018 at 4:19 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, > > On 18/09/2018 08:06, James Hilliard wrote: > > We use this feature to implement a manual 2 stage "check for updates" > > widget in our devices web interface. On our devices the swupdate http > > server is not directly exposed to the user, it listens only on > > localhost and requests are relayed through a nginx reverse proxy that > > validates authentication tokens against our main webapp backend before > > allowing access to the swupdate endpoints. > > > > The user first clicks the "check for updates" button which calls an > > endpoint on the device which downloads a manifest from our remote > > update server, this manifest is then returned in the device endpoint > > response to the user and includes the download url for the swu update > > file in addition to a changelog. This allows for the user to manually > > download the swu file to the computer, in addition there is an > > "update" button which we display in the widget when the device > > firmware is older than the firmware on our update server, when clicked > > this button makes an ajax call to this swupdate download endpoint(also > > routed via the nginx reverse proxy) so that users don't have to > > download any files to their computer. This endpoint also allows the > > user to tell the device to update directly from any other source as > > well such as a local server on their network. > > > > Our project requirements call for all firmware updates to be manually > > initiated from the device web interface for security reasons. > > Ok, thanks for explanation, I got your requirements. > > However, I disagree with the implementation. You copied a big part of > the downloader handler into the webserver interface, and this is quite > bad. I am trying to factorize all what I can. Yeah, it was a bit of a quick hack to get something out the door and my c is pretty rusty. What would a better approach be that would fit our project requirements? > > Nevertheless, the concept in the patch is quite weird. We have a > Webserver interface, that is the download of a SWU is in "push mode" > (from outside to the device), but it is forced in someway to go into > "pull" mode, mixing upload and download. It depends from > CONFIG_DOWNLOAD, too, and this adds libcurl to all builds, even if just > webserver is required. And as far as I could see, adding libcurl (and > its dependencies) has a big impact on the footprint, causing that most > rescue systems cannot fit anymore. Well the main issue is that we want to trigger an remote update from the local webif without having to restart the swupdate service, our device supports both "push" and "pull" modes. Should this be separated out into its own config so that this doesn't get built automatically with CONFIG_DOWNLOAD? > > A such check of external source with a manifest or whatever is something > that should be integrated in the "suricatta" mode instead of the > Webserver. I want also to ask if you are aware of some changes in code > that should make easier 2 stage update. They are the "dry-run" and the > possibility to store the SWU (but yes, on the device, so this could be > not interesting for you). The dry-run approach does much more as just > parsing a manifest file, because it try to run the whole SWU without > installing anything. Since we need a way to manually trigger this without a swupdate restart we need some sort of IPC mechanism and figured the webserver was the most straight forward. Our device uses primary/secondary redundant partitions already so I don't think we would have any use for a dry-run or temporary image storage. The manifest part is mostly for displaying stuff like change logs/messages to the end user so I'm not sure that sort of functionality would be something that would make sense to put into swupdate itself. > > How do you verify your manifest file ? I guess you had to provide your > own stuff to verify a signed manifest. I am also bother about security. > You add a "/download" callback, where you pass the URL. A malicious > could use it to push an own URL - I guess you masked this in your > internal Webserver, but if the patch is merged, the "/download" is open > for everybody. The manifest server API is protected via https, it's a super simple custom json API that our device side php webapp backend makes requests to using libcurl, we do enable RSA signatures for the swu files as well. We use nginx to protect all of the swupdate webserver's API endpoints(nginx passes the auth credentials in the request header to php for validation before it proxies the request to swupdate), although the authentication issue isn't specific to just the "/download" URL as the "/upload" URL has effectively the same security risk. > > Best regards, > Stefano Babic > > > On Mon, Sep 17, 2018 at 11:25 PM <james.hilliard1@gmail.com> wrote: > >> > >> From: James Hilliard <james.hilliard1@gmail.com> > >> > >> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >> --- > >> mongoose/mongoose_interface.c | 109 ++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 109 insertions(+) > >> > >> diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c > >> index 2521b30..534a027 100644 > >> --- a/mongoose/mongoose_interface.c > >> +++ b/mongoose/mongoose_interface.c > >> @@ -58,6 +58,61 @@ struct file_upload_state { > >> static struct mg_serve_http_opts s_http_server_opts; > >> static void upload_handler(struct mg_connection *nc, int ev, void *p); > >> > >> +#ifdef CONFIG_DOWNLOAD > >> +/* > >> + * Downloader > >> + */ > >> +#include <pthread.h> > >> +#include "download_interface.h" > >> +#include "channel.h" > >> +#include "channel_curl.h" > >> +#include "globals.h" > >> + > >> +struct downloader_thread_argvs{ > >> + char *url; > >> +}; > >> + > >> +#define DL_LOWSPEED_TIME 300 > >> + > >> +#define DL_DEFAULT_RETRIES 3 > >> + > >> +#define SETSTRING(p, v) do { \ > >> + if (p) \ > >> + free(p); \ > >> + p = strdup(v); \ > >> +} while (0) > >> + > >> +static channel_data_t channel_options = { > >> + .source = SOURCE_DOWNLOADER, > >> + .debug = false, > >> + .retries = DL_DEFAULT_RETRIES, > >> + .low_speed_timeout = DL_LOWSPEED_TIME > >> +}; > >> + > >> +static void download_handler(struct mg_connection *nc, int ev, void *p); > >> +static void *start_download_thread(void *argv); > >> + > >> + > >> +static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) > >> +{ > >> + channel_t *channel = channel_new(); > >> + if (channel->open(channel, channel_data) != CHANNEL_OK) { > >> + free(channel); > >> + return FAILURE; > >> + } > >> + > >> + RECOVERY_STATUS result = SUCCESS; > >> + channel_op_res_t chanresult = channel->get_file(channel, channel_data); > >> + if (chanresult != CHANNEL_OK) { > >> + result = FAILURE; > >> + } > >> + ipc_wait_for_complete(NULL); > >> + channel->close(channel); > >> + free(channel); > >> + return result; > >> +} > >> +#endif > >> + > >> /* > >> * These functions are for V2 of the protocol > >> */ > >> @@ -430,6 +485,29 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p) > >> } > >> } > >> > >> +#ifdef CONFIG_DOWNLOAD > >> +static void download_handler(struct mg_connection *nc, int ev, void *p) > >> +{ > >> + struct http_message *hm = (struct http_message *) p; > >> + > >> + char url[1024]; > >> + int ret = mg_get_http_var(&hm->body, "url", url, sizeof(url)); > >> + fprintf(stderr,"Received URL %d\n",ret); > >> + if (ret==-1) { > >> + mg_http_send_error(nc, 500, NULL); > >> + } else { > >> + pthread_t thread_download; > >> + pthread_create(&thread_download, NULL, start_download_thread, (void *)&url); > >> + > >> + mg_send_response_line(nc, 200, > >> + "Content-Type: text/plain\r\n" > >> + "Connection: close"); > >> + mg_send(nc, "\r\n", 2); > >> + nc->flags |= MG_F_SEND_AND_CLOSE; > >> + } > >> +} > >> +#endif > >> + > >> static void ev_handler_v1(struct mg_connection __attribute__ ((__unused__)) *nc, > >> int __attribute__ ((__unused__)) ev, > >> void __attribute__ ((__unused__)) *ev_data) > >> @@ -637,6 +715,9 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) > >> case MONGOOSE_API_V2: > >> mg_register_http_endpoint(nc, "/restart", restart_handler); > >> mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL)); > >> +#ifdef CONFIG_DOWNLOAD > >> + mg_register_http_endpoint(nc, "/download", download_handler); > >> +#endif > >> mg_start_thread(broadcast_message_thread, &mgr); > >> mg_start_thread(broadcast_progress_thread, &mgr); > >> break; > >> @@ -654,3 +735,31 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) > >> > >> return 0; > >> } > >> + > >> +#ifdef CONFIG_DOWNLOAD > >> +static void *start_download_thread(void *argv) > >> +{ > >> + char *url; > >> + if (argv) { > >> + url=(char*) argv; > >> + > >> + SETSTRING(channel_options.url, url); > >> + > >> + optind = 1; > >> + > >> + RECOVERY_STATUS result = download_from_url(&channel_options); > >> + if (result != FAILURE) { > >> + ipc_message msg; > >> + if (ipc_postupdate(&msg) != 0) { > >> + result = FAILURE; > >> + } else { > >> + result = msg.type == ACK ? result : FAILURE; > >> + } > >> + } > >> + > >> + if (channel_options.url != NULL) { > >> + free(channel_options.url); > >> + } > >> + } > >> +} > >> +#endif > >> -- > >> 2.7.4 > >> > > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
Hi James, On 18/09/2018 12:52, James Hilliard wrote: > On Tue, Sep 18, 2018 at 4:19 AM Stefano Babic <sbabic@denx.de> wrote: >> >> Hi James, >> >> On 18/09/2018 08:06, James Hilliard wrote: >>> We use this feature to implement a manual 2 stage "check for updates" >>> widget in our devices web interface. On our devices the swupdate http >>> server is not directly exposed to the user, it listens only on >>> localhost and requests are relayed through a nginx reverse proxy that >>> validates authentication tokens against our main webapp backend before >>> allowing access to the swupdate endpoints. >>> >>> The user first clicks the "check for updates" button which calls an >>> endpoint on the device which downloads a manifest from our remote >>> update server, this manifest is then returned in the device endpoint >>> response to the user and includes the download url for the swu update >>> file in addition to a changelog. This allows for the user to manually >>> download the swu file to the computer, in addition there is an >>> "update" button which we display in the widget when the device >>> firmware is older than the firmware on our update server, when clicked >>> this button makes an ajax call to this swupdate download endpoint(also >>> routed via the nginx reverse proxy) so that users don't have to >>> download any files to their computer. This endpoint also allows the >>> user to tell the device to update directly from any other source as >>> well such as a local server on their network. >>> >>> Our project requirements call for all firmware updates to be manually >>> initiated from the device web interface for security reasons. >> >> Ok, thanks for explanation, I got your requirements. >> >> However, I disagree with the implementation. You copied a big part of >> the downloader handler into the webserver interface, and this is quite >> bad. I am trying to factorize all what I can. > Yeah, it was a bit of a quick hack to get something out the door and > my c is pretty rusty. What would a better approach be that would fit > our project requirements? Well, I do not know your project, just some thoughts. You could: 1. chekc the download manifest outside SWUpdate and then use the IPC (see tools/client.c) to forward the SWU after having downloaded it. If you do not use the Webserver at all, you can short-cut it and use the ipc directly. 2. check the manifest outside SWUpdate and then connect to the Webserver in SWUpdate to start the install (I think, this is closer to your description). However, this has some drawbacks because nobody can ensure that the file downloaded on the PC is the same as the one pointed to the manifest file. This allows for example to install a previous version with security leak. IMHO this manual approach does not increase the security. 3. Add a backend to suricatta that periodically checks for update. This is a semi-automatic way because the check is automatic. A manual update could be triggered by adding a feature to enable/disable the suricatta daemon - suricatta just tests when is enable and doen not run when deactivated (or does nothing). >> >> Nevertheless, the concept in the patch is quite weird. We have a >> Webserver interface, that is the download of a SWU is in "push mode" >> (from outside to the device), but it is forced in someway to go into >> "pull" mode, mixing upload and download. It depends from >> CONFIG_DOWNLOAD, too, and this adds libcurl to all builds, even if just >> webserver is required. And as far as I could see, adding libcurl (and >> its dependencies) has a big impact on the footprint, causing that most >> rescue systems cannot fit anymore. > Well the main issue is that we want to trigger an remote update from > the local webif without having to restart the swupdate service, our > device supports both "push" and "pull" modes. Should this be separated > out into its own config so that this doesn't get built automatically > with CONFIG_DOWNLOAD? >> >> A such check of external source with a manifest or whatever is something >> that should be integrated in the "suricatta" mode instead of the >> Webserver. I want also to ask if you are aware of some changes in code >> that should make easier 2 stage update. They are the "dry-run" and the >> possibility to store the SWU (but yes, on the device, so this could be >> not interesting for you). The dry-run approach does much more as just >> parsing a manifest file, because it try to run the whole SWU without >> installing anything. > Since we need a way to manually trigger this without a swupdate > restart we need some sort of IPC mechanism and figured the webserver > was the most straight forward. SWUpdate has already an IPC mechanism and you can trigger an update without the Webserver and without restarting the daemon - check in the doc and in tools/ for examples and details. > Our device uses primary/secondary > redundant partitions already so I don't think we would have any use > for a dry-run or temporary image storage. The manifest part is mostly > for displaying stuff like change logs/messages to the end user so I'm > not sure that sort of functionality would be something that would make > sense to put into swupdate itself. ok >> >> How do you verify your manifest file ? I guess you had to provide your >> own stuff to verify a signed manifest. I am also bother about security. >> You add a "/download" callback, where you pass the URL. A malicious >> could use it to push an own URL - I guess you masked this in your >> internal Webserver, but if the patch is merged, the "/download" is open >> for everybody. > The manifest server API is protected via https, it's a super simple > custom json API that our device side php webapp backend makes requests Well, this is not enough...SWUpdate loads the packages via https, then if this is enought why do we need to sign ? Your mechanism is not man-in-the-middle safe. An attacker can replace the file son the server or change DNS to point to an own server with an own manifest file. The concept is not atomic, because you rely on the signed image into SWUpdate but the main decision if an updatemust be done relies on a not signed meta data outside the SWU package. > to using libcurl, we do enable RSA signatures for the swu files as > well. We use nginx to protect all of the swupdate webserver's API > endpoints(nginx passes the auth credentials in the request header to > php for validation before it proxies the request to swupdate), > although the authentication issue isn't specific to just the > "/download" URL as the "/upload" URL has effectively the same security > risk. Best regards, Stefano >> >> Best regards, >> Stefano Babic >> >>> On Mon, Sep 17, 2018 at 11:25 PM <james.hilliard1@gmail.com> wrote: >>>> >>>> From: James Hilliard <james.hilliard1@gmail.com> >>>> >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>>> --- >>>> mongoose/mongoose_interface.c | 109 ++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 109 insertions(+) >>>> >>>> diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c >>>> index 2521b30..534a027 100644 >>>> --- a/mongoose/mongoose_interface.c >>>> +++ b/mongoose/mongoose_interface.c >>>> @@ -58,6 +58,61 @@ struct file_upload_state { >>>> static struct mg_serve_http_opts s_http_server_opts; >>>> static void upload_handler(struct mg_connection *nc, int ev, void *p); >>>> >>>> +#ifdef CONFIG_DOWNLOAD >>>> +/* >>>> + * Downloader >>>> + */ >>>> +#include <pthread.h> >>>> +#include "download_interface.h" >>>> +#include "channel.h" >>>> +#include "channel_curl.h" >>>> +#include "globals.h" >>>> + >>>> +struct downloader_thread_argvs{ >>>> + char *url; >>>> +}; >>>> + >>>> +#define DL_LOWSPEED_TIME 300 >>>> + >>>> +#define DL_DEFAULT_RETRIES 3 >>>> + >>>> +#define SETSTRING(p, v) do { \ >>>> + if (p) \ >>>> + free(p); \ >>>> + p = strdup(v); \ >>>> +} while (0) >>>> + >>>> +static channel_data_t channel_options = { >>>> + .source = SOURCE_DOWNLOADER, >>>> + .debug = false, >>>> + .retries = DL_DEFAULT_RETRIES, >>>> + .low_speed_timeout = DL_LOWSPEED_TIME >>>> +}; >>>> + >>>> +static void download_handler(struct mg_connection *nc, int ev, void *p); >>>> +static void *start_download_thread(void *argv); >>>> + >>>> + >>>> +static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) >>>> +{ >>>> + channel_t *channel = channel_new(); >>>> + if (channel->open(channel, channel_data) != CHANNEL_OK) { >>>> + free(channel); >>>> + return FAILURE; >>>> + } >>>> + >>>> + RECOVERY_STATUS result = SUCCESS; >>>> + channel_op_res_t chanresult = channel->get_file(channel, channel_data); >>>> + if (chanresult != CHANNEL_OK) { >>>> + result = FAILURE; >>>> + } >>>> + ipc_wait_for_complete(NULL); >>>> + channel->close(channel); >>>> + free(channel); >>>> + return result; >>>> +} >>>> +#endif >>>> + >>>> /* >>>> * These functions are for V2 of the protocol >>>> */ >>>> @@ -430,6 +485,29 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p) >>>> } >>>> } >>>> >>>> +#ifdef CONFIG_DOWNLOAD >>>> +static void download_handler(struct mg_connection *nc, int ev, void *p) >>>> +{ >>>> + struct http_message *hm = (struct http_message *) p; >>>> + >>>> + char url[1024]; >>>> + int ret = mg_get_http_var(&hm->body, "url", url, sizeof(url)); >>>> + fprintf(stderr,"Received URL %d\n",ret); >>>> + if (ret==-1) { >>>> + mg_http_send_error(nc, 500, NULL); >>>> + } else { >>>> + pthread_t thread_download; >>>> + pthread_create(&thread_download, NULL, start_download_thread, (void *)&url); >>>> + >>>> + mg_send_response_line(nc, 200, >>>> + "Content-Type: text/plain\r\n" >>>> + "Connection: close"); >>>> + mg_send(nc, "\r\n", 2); >>>> + nc->flags |= MG_F_SEND_AND_CLOSE; >>>> + } >>>> +} >>>> +#endif >>>> + >>>> static void ev_handler_v1(struct mg_connection __attribute__ ((__unused__)) *nc, >>>> int __attribute__ ((__unused__)) ev, >>>> void __attribute__ ((__unused__)) *ev_data) >>>> @@ -637,6 +715,9 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) >>>> case MONGOOSE_API_V2: >>>> mg_register_http_endpoint(nc, "/restart", restart_handler); >>>> mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL)); >>>> +#ifdef CONFIG_DOWNLOAD >>>> + mg_register_http_endpoint(nc, "/download", download_handler); >>>> +#endif >>>> mg_start_thread(broadcast_message_thread, &mgr); >>>> mg_start_thread(broadcast_progress_thread, &mgr); >>>> break; >>>> @@ -654,3 +735,31 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) >>>> >>>> return 0; >>>> } >>>> + >>>> +#ifdef CONFIG_DOWNLOAD >>>> +static void *start_download_thread(void *argv) >>>> +{ >>>> + char *url; >>>> + if (argv) { >>>> + url=(char*) argv; >>>> + >>>> + SETSTRING(channel_options.url, url); >>>> + >>>> + optind = 1; >>>> + >>>> + RECOVERY_STATUS result = download_from_url(&channel_options); >>>> + if (result != FAILURE) { >>>> + ipc_message msg; >>>> + if (ipc_postupdate(&msg) != 0) { >>>> + result = FAILURE; >>>> + } else { >>>> + result = msg.type == ACK ? result : FAILURE; >>>> + } >>>> + } >>>> + >>>> + if (channel_options.url != NULL) { >>>> + free(channel_options.url); >>>> + } >>>> + } >>>> +} >>>> +#endif >>>> -- >>>> 2.7.4 >>>> >>> >> >> >> -- >> ===================================================================== >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de >> ===================================================================== >
On Tue, Sep 18, 2018 at 6:51 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, > > On 18/09/2018 12:52, James Hilliard wrote: > > On Tue, Sep 18, 2018 at 4:19 AM Stefano Babic <sbabic@denx.de> wrote: > >> > >> Hi James, > >> > >> On 18/09/2018 08:06, James Hilliard wrote: > >>> We use this feature to implement a manual 2 stage "check for updates" > >>> widget in our devices web interface. On our devices the swupdate http > >>> server is not directly exposed to the user, it listens only on > >>> localhost and requests are relayed through a nginx reverse proxy that > >>> validates authentication tokens against our main webapp backend before > >>> allowing access to the swupdate endpoints. > >>> > >>> The user first clicks the "check for updates" button which calls an > >>> endpoint on the device which downloads a manifest from our remote > >>> update server, this manifest is then returned in the device endpoint > >>> response to the user and includes the download url for the swu update > >>> file in addition to a changelog. This allows for the user to manually > >>> download the swu file to the computer, in addition there is an > >>> "update" button which we display in the widget when the device > >>> firmware is older than the firmware on our update server, when clicked > >>> this button makes an ajax call to this swupdate download endpoint(also > >>> routed via the nginx reverse proxy) so that users don't have to > >>> download any files to their computer. This endpoint also allows the > >>> user to tell the device to update directly from any other source as > >>> well such as a local server on their network. > >>> > >>> Our project requirements call for all firmware updates to be manually > >>> initiated from the device web interface for security reasons. > >> > >> Ok, thanks for explanation, I got your requirements. > >> > >> However, I disagree with the implementation. You copied a big part of > >> the downloader handler into the webserver interface, and this is quite > >> bad. I am trying to factorize all what I can. > > Yeah, it was a bit of a quick hack to get something out the door and > > my c is pretty rusty. What would a better approach be that would fit > > our project requirements? > > Well, I do not know your project, just some thoughts. You could: > > 1. chekc the download manifest outside SWUpdate and then use the IPC > (see tools/client.c) to forward the SWU after having downloaded it. > If you do not use the Webserver at all, you can short-cut it and use the > ipc directly. I'm trying to avoid having to handle the file downloading part outside of swupdate. > > 2. check the manifest outside SWUpdate and then connect to the Webserver > in SWUpdate to start the install (I think, this is closer to your > description). However, this has some drawbacks because nobody can ensure > that the file downloaded on the PC is the same as the one pointed to the > manifest file. This allows for example to install a previous version > with security leak. IMHO this manual approach does not increase the > security. downgrade protection would be done with the signed sw-description, the manifest outside of swupdate is mostly a convenience feature, not really intended to improve security > > 3. Add a backend to suricatta that periodically checks for update. This > is a semi-automatic way because the check is automatic. > A manual update could be triggered by adding a feature to enable/disable > the suricatta daemon - suricatta just tests when is enable and doen not > run when deactivated (or does nothing). hmm, we need to have the option to pass the download url over http to the device so that probably wouldn't work > > > >> > >> Nevertheless, the concept in the patch is quite weird. We have a > >> Webserver interface, that is the download of a SWU is in "push mode" > >> (from outside to the device), but it is forced in someway to go into > >> "pull" mode, mixing upload and download. It depends from > >> CONFIG_DOWNLOAD, too, and this adds libcurl to all builds, even if just > >> webserver is required. And as far as I could see, adding libcurl (and > >> its dependencies) has a big impact on the footprint, causing that most > >> rescue systems cannot fit anymore. > > Well the main issue is that we want to trigger an remote update from > > the local webif without having to restart the swupdate service, our > > device supports both "push" and "pull" modes. Should this be separated > > out into its own config so that this doesn't get built automatically > > with CONFIG_DOWNLOAD? > >> > >> A such check of external source with a manifest or whatever is something > >> that should be integrated in the "suricatta" mode instead of the > >> Webserver. I want also to ask if you are aware of some changes in code > >> that should make easier 2 stage update. They are the "dry-run" and the > >> possibility to store the SWU (but yes, on the device, so this could be > >> not interesting for you). The dry-run approach does much more as just > >> parsing a manifest file, because it try to run the whole SWU without > >> installing anything. > > Since we need a way to manually trigger this without a swupdate > > restart we need some sort of IPC mechanism and figured the webserver > > was the most straight forward. > > SWUpdate has already an IPC mechanism and you can trigger an update > without the Webserver and without restarting the daemon - check in the > doc and in tools/ for examples and details. can it trigger by just passing a url to swupdate or do you have to download the swu file beforehand? > > > Our device uses primary/secondary > > redundant partitions already so I don't think we would have any use > > for a dry-run or temporary image storage. The manifest part is mostly > > for displaying stuff like change logs/messages to the end user so I'm > > not sure that sort of functionality would be something that would make > > sense to put into swupdate itself. > > ok > > > >> > >> How do you verify your manifest file ? I guess you had to provide your > >> own stuff to verify a signed manifest. I am also bother about security. > >> You add a "/download" callback, where you pass the URL. A malicious > >> could use it to push an own URL - I guess you masked this in your > >> internal Webserver, but if the patch is merged, the "/download" is open > >> for everybody. > > The manifest server API is protected via https, it's a super simple > > custom json API that our device side php webapp backend makes requests > > Well, this is not enough...SWUpdate loads the packages via https, then > if this is enought why do we need to sign ? because updates can be uploaded as well locally without involving the update server in any way, so we need signature enforcement there > > Your mechanism is not man-in-the-middle safe. An attacker can replace > the file son the server or change DNS to point to an own server with an > own manifest file. The concept is not atomic, because you rely on the > signed image into SWUpdate but the main decision if an updatemust be > done relies on a not signed meta data outside the SWU package. everything remote is https so it would be safe as long as the remote server isn't hacked or something like that, local isn't but we do have swu signature validation at least > > > to using libcurl, we do enable RSA signatures for the swu files as > > well. We use nginx to protect all of the swupdate webserver's API > > endpoints(nginx passes the auth credentials in the request header to > > php for validation before it proxies the request to swupdate), > > although the authentication issue isn't specific to just the > > "/download" URL as the "/upload" URL has effectively the same security > > risk. > > > Best regards, > Stefano > > > >> > >> Best regards, > >> Stefano Babic > >> > >>> On Mon, Sep 17, 2018 at 11:25 PM <james.hilliard1@gmail.com> wrote: > >>>> > >>>> From: James Hilliard <james.hilliard1@gmail.com> > >>>> > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >>>> --- > >>>> mongoose/mongoose_interface.c | 109 ++++++++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 109 insertions(+) > >>>> > >>>> diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c > >>>> index 2521b30..534a027 100644 > >>>> --- a/mongoose/mongoose_interface.c > >>>> +++ b/mongoose/mongoose_interface.c > >>>> @@ -58,6 +58,61 @@ struct file_upload_state { > >>>> static struct mg_serve_http_opts s_http_server_opts; > >>>> static void upload_handler(struct mg_connection *nc, int ev, void *p); > >>>> > >>>> +#ifdef CONFIG_DOWNLOAD > >>>> +/* > >>>> + * Downloader > >>>> + */ > >>>> +#include <pthread.h> > >>>> +#include "download_interface.h" > >>>> +#include "channel.h" > >>>> +#include "channel_curl.h" > >>>> +#include "globals.h" > >>>> + > >>>> +struct downloader_thread_argvs{ > >>>> + char *url; > >>>> +}; > >>>> + > >>>> +#define DL_LOWSPEED_TIME 300 > >>>> + > >>>> +#define DL_DEFAULT_RETRIES 3 > >>>> + > >>>> +#define SETSTRING(p, v) do { \ > >>>> + if (p) \ > >>>> + free(p); \ > >>>> + p = strdup(v); \ > >>>> +} while (0) > >>>> + > >>>> +static channel_data_t channel_options = { > >>>> + .source = SOURCE_DOWNLOADER, > >>>> + .debug = false, > >>>> + .retries = DL_DEFAULT_RETRIES, > >>>> + .low_speed_timeout = DL_LOWSPEED_TIME > >>>> +}; > >>>> + > >>>> +static void download_handler(struct mg_connection *nc, int ev, void *p); > >>>> +static void *start_download_thread(void *argv); > >>>> + > >>>> + > >>>> +static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) > >>>> +{ > >>>> + channel_t *channel = channel_new(); > >>>> + if (channel->open(channel, channel_data) != CHANNEL_OK) { > >>>> + free(channel); > >>>> + return FAILURE; > >>>> + } > >>>> + > >>>> + RECOVERY_STATUS result = SUCCESS; > >>>> + channel_op_res_t chanresult = channel->get_file(channel, channel_data); > >>>> + if (chanresult != CHANNEL_OK) { > >>>> + result = FAILURE; > >>>> + } > >>>> + ipc_wait_for_complete(NULL); > >>>> + channel->close(channel); > >>>> + free(channel); > >>>> + return result; > >>>> +} > >>>> +#endif > >>>> + > >>>> /* > >>>> * These functions are for V2 of the protocol > >>>> */ > >>>> @@ -430,6 +485,29 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p) > >>>> } > >>>> } > >>>> > >>>> +#ifdef CONFIG_DOWNLOAD > >>>> +static void download_handler(struct mg_connection *nc, int ev, void *p) > >>>> +{ > >>>> + struct http_message *hm = (struct http_message *) p; > >>>> + > >>>> + char url[1024]; > >>>> + int ret = mg_get_http_var(&hm->body, "url", url, sizeof(url)); > >>>> + fprintf(stderr,"Received URL %d\n",ret); > >>>> + if (ret==-1) { > >>>> + mg_http_send_error(nc, 500, NULL); > >>>> + } else { > >>>> + pthread_t thread_download; > >>>> + pthread_create(&thread_download, NULL, start_download_thread, (void *)&url); > >>>> + > >>>> + mg_send_response_line(nc, 200, > >>>> + "Content-Type: text/plain\r\n" > >>>> + "Connection: close"); > >>>> + mg_send(nc, "\r\n", 2); > >>>> + nc->flags |= MG_F_SEND_AND_CLOSE; > >>>> + } > >>>> +} > >>>> +#endif > >>>> + > >>>> static void ev_handler_v1(struct mg_connection __attribute__ ((__unused__)) *nc, > >>>> int __attribute__ ((__unused__)) ev, > >>>> void __attribute__ ((__unused__)) *ev_data) > >>>> @@ -637,6 +715,9 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) > >>>> case MONGOOSE_API_V2: > >>>> mg_register_http_endpoint(nc, "/restart", restart_handler); > >>>> mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL)); > >>>> +#ifdef CONFIG_DOWNLOAD > >>>> + mg_register_http_endpoint(nc, "/download", download_handler); > >>>> +#endif > >>>> mg_start_thread(broadcast_message_thread, &mgr); > >>>> mg_start_thread(broadcast_progress_thread, &mgr); > >>>> break; > >>>> @@ -654,3 +735,31 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) > >>>> > >>>> return 0; > >>>> } > >>>> + > >>>> +#ifdef CONFIG_DOWNLOAD > >>>> +static void *start_download_thread(void *argv) > >>>> +{ > >>>> + char *url; > >>>> + if (argv) { > >>>> + url=(char*) argv; > >>>> + > >>>> + SETSTRING(channel_options.url, url); > >>>> + > >>>> + optind = 1; > >>>> + > >>>> + RECOVERY_STATUS result = download_from_url(&channel_options); > >>>> + if (result != FAILURE) { > >>>> + ipc_message msg; > >>>> + if (ipc_postupdate(&msg) != 0) { > >>>> + result = FAILURE; > >>>> + } else { > >>>> + result = msg.type == ACK ? result : FAILURE; > >>>> + } > >>>> + } > >>>> + > >>>> + if (channel_options.url != NULL) { > >>>> + free(channel_options.url); > >>>> + } > >>>> + } > >>>> +} > >>>> +#endif > >>>> -- > >>>> 2.7.4 > >>>> > >>> > >> > >> > >> -- > >> ===================================================================== > >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > >> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > >> ===================================================================== > > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
Hi James, On 18/09/2018 15:16, James Hilliard wrote: > On Tue, Sep 18, 2018 at 6:51 AM Stefano Babic <sbabic@denx.de> wrote: >> >> Hi James, >> >> On 18/09/2018 12:52, James Hilliard wrote: >>> On Tue, Sep 18, 2018 at 4:19 AM Stefano Babic <sbabic@denx.de> wrote: >>>> >>>> Hi James, >>>> >>>> On 18/09/2018 08:06, James Hilliard wrote: >>>>> We use this feature to implement a manual 2 stage "check for updates" >>>>> widget in our devices web interface. On our devices the swupdate http >>>>> server is not directly exposed to the user, it listens only on >>>>> localhost and requests are relayed through a nginx reverse proxy that >>>>> validates authentication tokens against our main webapp backend before >>>>> allowing access to the swupdate endpoints. >>>>> >>>>> The user first clicks the "check for updates" button which calls an >>>>> endpoint on the device which downloads a manifest from our remote >>>>> update server, this manifest is then returned in the device endpoint >>>>> response to the user and includes the download url for the swu update >>>>> file in addition to a changelog. This allows for the user to manually >>>>> download the swu file to the computer, in addition there is an >>>>> "update" button which we display in the widget when the device >>>>> firmware is older than the firmware on our update server, when clicked >>>>> this button makes an ajax call to this swupdate download endpoint(also >>>>> routed via the nginx reverse proxy) so that users don't have to >>>>> download any files to their computer. This endpoint also allows the >>>>> user to tell the device to update directly from any other source as >>>>> well such as a local server on their network. >>>>> >>>>> Our project requirements call for all firmware updates to be manually >>>>> initiated from the device web interface for security reasons. >>>> >>>> Ok, thanks for explanation, I got your requirements. >>>> >>>> However, I disagree with the implementation. You copied a big part of >>>> the downloader handler into the webserver interface, and this is quite >>>> bad. I am trying to factorize all what I can. >>> Yeah, it was a bit of a quick hack to get something out the door and >>> my c is pretty rusty. What would a better approach be that would fit >>> our project requirements? >> >> Well, I do not know your project, just some thoughts. You could: >> >> 1. chekc the download manifest outside SWUpdate and then use the IPC >> (see tools/client.c) to forward the SWU after having downloaded it. >> If you do not use the Webserver at all, you can short-cut it and use the >> ipc directly. > I'm trying to avoid having to handle the file downloading part outside > of swupdate. >> >> 2. check the manifest outside SWUpdate and then connect to the Webserver >> in SWUpdate to start the install (I think, this is closer to your >> description). However, this has some drawbacks because nobody can ensure >> that the file downloaded on the PC is the same as the one pointed to the >> manifest file. This allows for example to install a previous version >> with security leak. IMHO this manual approach does not increase the >> security. > downgrade protection would be done with the signed sw-description, the > manifest outside of swupdate is mostly a convenience feature, not > really intended to improve security ok - so I guess you set the version in sw-description and you activate the no-downgrading flag. >> >> 3. Add a backend to suricatta that periodically checks for update. This >> is a semi-automatic way because the check is automatic. >> A manual update could be triggered by adding a feature to enable/disable >> the suricatta daemon - suricatta just tests when is enable and doen not >> run when deactivated (or does nothing). > hmm, we need to have the option to pass the download url over http to > the device so that probably wouldn't work Why do you need to pass the URL over http to the device ? Does the URL change any time ? If the URL is just used to load your manifest, it is not used by the internal Webserver at all. You just need a way to pass it to SWUpdate (via IPC or via configuration file if it is fixed), but I do not see any reason why it should be done via http. >> >> >>>> >>>> Nevertheless, the concept in the patch is quite weird. We have a >>>> Webserver interface, that is the download of a SWU is in "push mode" >>>> (from outside to the device), but it is forced in someway to go into >>>> "pull" mode, mixing upload and download. It depends from >>>> CONFIG_DOWNLOAD, too, and this adds libcurl to all builds, even if just >>>> webserver is required. And as far as I could see, adding libcurl (and >>>> its dependencies) has a big impact on the footprint, causing that most >>>> rescue systems cannot fit anymore. >>> Well the main issue is that we want to trigger an remote update from >>> the local webif without having to restart the swupdate service, our >>> device supports both "push" and "pull" modes. Should this be separated >>> out into its own config so that this doesn't get built automatically >>> with CONFIG_DOWNLOAD? >>>> >>>> A such check of external source with a manifest or whatever is something >>>> that should be integrated in the "suricatta" mode instead of the >>>> Webserver. I want also to ask if you are aware of some changes in code >>>> that should make easier 2 stage update. They are the "dry-run" and the >>>> possibility to store the SWU (but yes, on the device, so this could be >>>> not interesting for you). The dry-run approach does much more as just >>>> parsing a manifest file, because it try to run the whole SWU without >>>> installing anything. >>> Since we need a way to manually trigger this without a swupdate >>> restart we need some sort of IPC mechanism and figured the webserver >>> was the most straight forward. >> >> SWUpdate has already an IPC mechanism and you can trigger an update >> without the Webserver and without restarting the daemon - check in the >> doc and in tools/ for examples and details. > can it trigger by just passing a url to swupdate or do you have to > download the swu file beforehand? >> >>> Our device uses primary/secondary >>> redundant partitions already so I don't think we would have any use >>> for a dry-run or temporary image storage. The manifest part is mostly >>> for displaying stuff like change logs/messages to the end user so I'm >>> not sure that sort of functionality would be something that would make >>> sense to put into swupdate itself. >> >> ok >> >> >>>> >>>> How do you verify your manifest file ? I guess you had to provide your >>>> own stuff to verify a signed manifest. I am also bother about security. >>>> You add a "/download" callback, where you pass the URL. A malicious >>>> could use it to push an own URL - I guess you masked this in your >>>> internal Webserver, but if the patch is merged, the "/download" is open >>>> for everybody. >>> The manifest server API is protected via https, it's a super simple >>> custom json API that our device side php webapp backend makes requests >> >> Well, this is not enough...SWUpdate loads the packages via https, then >> if this is enought why do we need to sign ? > because updates can be uploaded as well locally without involving the > update server in any way, so we need signature enforcement there Right, what I meant is that https is not enough... >> >> Your mechanism is not man-in-the-middle safe. An attacker can replace >> the file son the server or change DNS to point to an own server with an >> own manifest file. The concept is not atomic, because you rely on the >> signed image into SWUpdate but the main decision if an updatemust be >> done relies on a not signed meta data outside the SWU package. > everything remote is https so it would be safe as long as the remote > server isn't hacked or something like that, This is what I meant. The server can be hacked or the network can be hacked and the device loads the manifest from another server and not yours. > local isn't but we do have > swu signature validation at least >> >>> to using libcurl, we do enable RSA signatures for the swu files as >>> well. We use nginx to protect all of the swupdate webserver's API >>> endpoints(nginx passes the auth credentials in the request header to >>> php for validation before it proxies the request to swupdate), >>> although the authentication issue isn't specific to just the >>> "/download" URL as the "/upload" URL has effectively the same security >>> risk. >> Best regards, Stefano Babic
On Tue, Sep 18, 2018 at 9:35 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, > > On 18/09/2018 15:16, James Hilliard wrote: > > On Tue, Sep 18, 2018 at 6:51 AM Stefano Babic <sbabic@denx.de> wrote: > >> > >> Hi James, > >> > >> On 18/09/2018 12:52, James Hilliard wrote: > >>> On Tue, Sep 18, 2018 at 4:19 AM Stefano Babic <sbabic@denx.de> wrote: > >>>> > >>>> Hi James, > >>>> > >>>> On 18/09/2018 08:06, James Hilliard wrote: > >>>>> We use this feature to implement a manual 2 stage "check for updates" > >>>>> widget in our devices web interface. On our devices the swupdate http > >>>>> server is not directly exposed to the user, it listens only on > >>>>> localhost and requests are relayed through a nginx reverse proxy that > >>>>> validates authentication tokens against our main webapp backend before > >>>>> allowing access to the swupdate endpoints. > >>>>> > >>>>> The user first clicks the "check for updates" button which calls an > >>>>> endpoint on the device which downloads a manifest from our remote > >>>>> update server, this manifest is then returned in the device endpoint > >>>>> response to the user and includes the download url for the swu update > >>>>> file in addition to a changelog. This allows for the user to manually > >>>>> download the swu file to the computer, in addition there is an > >>>>> "update" button which we display in the widget when the device > >>>>> firmware is older than the firmware on our update server, when clicked > >>>>> this button makes an ajax call to this swupdate download endpoint(also > >>>>> routed via the nginx reverse proxy) so that users don't have to > >>>>> download any files to their computer. This endpoint also allows the > >>>>> user to tell the device to update directly from any other source as > >>>>> well such as a local server on their network. > >>>>> > >>>>> Our project requirements call for all firmware updates to be manually > >>>>> initiated from the device web interface for security reasons. > >>>> > >>>> Ok, thanks for explanation, I got your requirements. > >>>> > >>>> However, I disagree with the implementation. You copied a big part of > >>>> the downloader handler into the webserver interface, and this is quite > >>>> bad. I am trying to factorize all what I can. > >>> Yeah, it was a bit of a quick hack to get something out the door and > >>> my c is pretty rusty. What would a better approach be that would fit > >>> our project requirements? > >> > >> Well, I do not know your project, just some thoughts. You could: > >> > >> 1. chekc the download manifest outside SWUpdate and then use the IPC > >> (see tools/client.c) to forward the SWU after having downloaded it. > >> If you do not use the Webserver at all, you can short-cut it and use the > >> ipc directly. > > I'm trying to avoid having to handle the file downloading part outside > > of swupdate. > >> > >> 2. check the manifest outside SWUpdate and then connect to the Webserver > >> in SWUpdate to start the install (I think, this is closer to your > >> description). However, this has some drawbacks because nobody can ensure > >> that the file downloaded on the PC is the same as the one pointed to the > >> manifest file. This allows for example to install a previous version > >> with security leak. IMHO this manual approach does not increase the > >> security. > > downgrade protection would be done with the signed sw-description, the > > manifest outside of swupdate is mostly a convenience feature, not > > really intended to improve security > > ok - so I guess you set the version in sw-description and you activate > the no-downgrading flag. Yes, that's what I was planning on doing for when we need to prevent downgrades. We do normally allow downgrades so that users can rollback if they find any bugs in new builds. > > >> > >> 3. Add a backend to suricatta that periodically checks for update. This > >> is a semi-automatic way because the check is automatic. > >> A manual update could be triggered by adding a feature to enable/disable > >> the suricatta daemon - suricatta just tests when is enable and doen not > >> run when deactivated (or does nothing). > > hmm, we need to have the option to pass the download url over http to > > the device so that probably wouldn't work > > Why do you need to pass the URL over http to the device ? Does the URL > change any time ? We pass the url so that devices can be told to download specific swu update files such as testing builds or from servers on the local network(it's not unusual for thousands of our devices to be on the same local network). > > If the URL is just used to load your manifest, it is not used by the > internal Webserver at all. You just need a way to pass it to SWUpdate > (via IPC or via configuration file if it is fixed), but I do not see any > reason why it should be done via http. We have it like that so the device can be pointed at arbitrary locations to download from. > > >> > >> > >>>> > >>>> Nevertheless, the concept in the patch is quite weird. We have a > >>>> Webserver interface, that is the download of a SWU is in "push mode" > >>>> (from outside to the device), but it is forced in someway to go into > >>>> "pull" mode, mixing upload and download. It depends from > >>>> CONFIG_DOWNLOAD, too, and this adds libcurl to all builds, even if just > >>>> webserver is required. And as far as I could see, adding libcurl (and > >>>> its dependencies) has a big impact on the footprint, causing that most > >>>> rescue systems cannot fit anymore. > >>> Well the main issue is that we want to trigger an remote update from > >>> the local webif without having to restart the swupdate service, our > >>> device supports both "push" and "pull" modes. Should this be separated > >>> out into its own config so that this doesn't get built automatically > >>> with CONFIG_DOWNLOAD? > >>>> > >>>> A such check of external source with a manifest or whatever is something > >>>> that should be integrated in the "suricatta" mode instead of the > >>>> Webserver. I want also to ask if you are aware of some changes in code > >>>> that should make easier 2 stage update. They are the "dry-run" and the > >>>> possibility to store the SWU (but yes, on the device, so this could be > >>>> not interesting for you). The dry-run approach does much more as just > >>>> parsing a manifest file, because it try to run the whole SWU without > >>>> installing anything. > >>> Since we need a way to manually trigger this without a swupdate > >>> restart we need some sort of IPC mechanism and figured the webserver > >>> was the most straight forward. > >> > >> SWUpdate has already an IPC mechanism and you can trigger an update > >> without the Webserver and without restarting the daemon - check in the > >> doc and in tools/ for examples and details. > > can it trigger by just passing a url to swupdate or do you have to > > download the swu file beforehand? > >> > >>> Our device uses primary/secondary > >>> redundant partitions already so I don't think we would have any use > >>> for a dry-run or temporary image storage. The manifest part is mostly > >>> for displaying stuff like change logs/messages to the end user so I'm > >>> not sure that sort of functionality would be something that would make > >>> sense to put into swupdate itself. > >> > >> ok > >> > >> > >>>> > >>>> How do you verify your manifest file ? I guess you had to provide your > >>>> own stuff to verify a signed manifest. I am also bother about security. > >>>> You add a "/download" callback, where you pass the URL. A malicious > >>>> could use it to push an own URL - I guess you masked this in your > >>>> internal Webserver, but if the patch is merged, the "/download" is open > >>>> for everybody. > >>> The manifest server API is protected via https, it's a super simple > >>> custom json API that our device side php webapp backend makes requests > >> > >> Well, this is not enough...SWUpdate loads the packages via https, then > >> if this is enought why do we need to sign ? > > because updates can be uploaded as well locally without involving the > > update server in any way, so we need signature enforcement there > > Right, what I meant is that https is not enough... If the manifest API server is compromised an attacker would still not be able to do much without the swu signing key since the device would just reject unsigned builds. > > >> > >> Your mechanism is not man-in-the-middle safe. An attacker can replace > >> the file son the server or change DNS to point to an own server with an > >> own manifest file. The concept is not atomic, because you rely on the > >> signed image into SWUpdate but the main decision if an updatemust be > >> done relies on a not signed meta data outside the SWU package. > > everything remote is https so it would be safe as long as the remote > > server isn't hacked or something like that, > > This is what I meant. The server can be hacked or the network can be > hacked and the device loads the manifest from another server and not yours. Sure, but I don't really see that being much of an issue due to swu signature enforcement. > > > local isn't but we do have > > swu signature validation at least > >> > >>> to using libcurl, we do enable RSA signatures for the swu files as > >>> well. We use nginx to protect all of the swupdate webserver's API > >>> endpoints(nginx passes the auth credentials in the request header to > >>> php for validation before it proxies the request to swupdate), > >>> although the authentication issue isn't specific to just the > >>> "/download" URL as the "/upload" URL has effectively the same security > >>> risk. > >> > > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c index 2521b30..534a027 100644 --- a/mongoose/mongoose_interface.c +++ b/mongoose/mongoose_interface.c @@ -58,6 +58,61 @@ struct file_upload_state { static struct mg_serve_http_opts s_http_server_opts; static void upload_handler(struct mg_connection *nc, int ev, void *p); +#ifdef CONFIG_DOWNLOAD +/* + * Downloader + */ +#include <pthread.h> +#include "download_interface.h" +#include "channel.h" +#include "channel_curl.h" +#include "globals.h" + +struct downloader_thread_argvs{ + char *url; +}; + +#define DL_LOWSPEED_TIME 300 + +#define DL_DEFAULT_RETRIES 3 + +#define SETSTRING(p, v) do { \ + if (p) \ + free(p); \ + p = strdup(v); \ +} while (0) + +static channel_data_t channel_options = { + .source = SOURCE_DOWNLOADER, + .debug = false, + .retries = DL_DEFAULT_RETRIES, + .low_speed_timeout = DL_LOWSPEED_TIME +}; + +static void download_handler(struct mg_connection *nc, int ev, void *p); +static void *start_download_thread(void *argv); + + +static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) +{ + channel_t *channel = channel_new(); + if (channel->open(channel, channel_data) != CHANNEL_OK) { + free(channel); + return FAILURE; + } + + RECOVERY_STATUS result = SUCCESS; + channel_op_res_t chanresult = channel->get_file(channel, channel_data); + if (chanresult != CHANNEL_OK) { + result = FAILURE; + } + ipc_wait_for_complete(NULL); + channel->close(channel); + free(channel); + return result; +} +#endif + /* * These functions are for V2 of the protocol */ @@ -430,6 +485,29 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p) } } +#ifdef CONFIG_DOWNLOAD +static void download_handler(struct mg_connection *nc, int ev, void *p) +{ + struct http_message *hm = (struct http_message *) p; + + char url[1024]; + int ret = mg_get_http_var(&hm->body, "url", url, sizeof(url)); + fprintf(stderr,"Received URL %d\n",ret); + if (ret==-1) { + mg_http_send_error(nc, 500, NULL); + } else { + pthread_t thread_download; + pthread_create(&thread_download, NULL, start_download_thread, (void *)&url); + + mg_send_response_line(nc, 200, + "Content-Type: text/plain\r\n" + "Connection: close"); + mg_send(nc, "\r\n", 2); + nc->flags |= MG_F_SEND_AND_CLOSE; + } +} +#endif + static void ev_handler_v1(struct mg_connection __attribute__ ((__unused__)) *nc, int __attribute__ ((__unused__)) ev, void __attribute__ ((__unused__)) *ev_data) @@ -637,6 +715,9 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) case MONGOOSE_API_V2: mg_register_http_endpoint(nc, "/restart", restart_handler); mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL)); +#ifdef CONFIG_DOWNLOAD + mg_register_http_endpoint(nc, "/download", download_handler); +#endif mg_start_thread(broadcast_message_thread, &mgr); mg_start_thread(broadcast_progress_thread, &mgr); break; @@ -654,3 +735,31 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[]) return 0; } + +#ifdef CONFIG_DOWNLOAD +static void *start_download_thread(void *argv) +{ + char *url; + if (argv) { + url=(char*) argv; + + SETSTRING(channel_options.url, url); + + optind = 1; + + RECOVERY_STATUS result = download_from_url(&channel_options); + if (result != FAILURE) { + ipc_message msg; + if (ipc_postupdate(&msg) != 0) { + result = FAILURE; + } else { + result = msg.type == ACK ? result : FAILURE; + } + } + + if (channel_options.url != NULL) { + free(channel_options.url); + } + } +} +#endif