diff mbox series

Add Download Endpoint

Message ID 1537248227-21218-1-git-send-email-james.hilliard1@gmail.com
State Changes Requested
Headers show
Series Add Download Endpoint | expand

Commit Message

James Hilliard Sept. 18, 2018, 5:23 a.m. UTC
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(+)

Comments

James Hilliard Sept. 18, 2018, 6:06 a.m. UTC | #1
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
>
Stefano Babic Sept. 18, 2018, 10:19 a.m. UTC | #2
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
>>
>
James Hilliard Sept. 18, 2018, 10:52 a.m. UTC | #3
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
> =====================================================================
Stefano Babic Sept. 18, 2018, 12:50 p.m. UTC | #4
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
>> =====================================================================
>
James Hilliard Sept. 18, 2018, 1:16 p.m. UTC | #5
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
> =====================================================================
Stefano Babic Sept. 18, 2018, 3:35 p.m. UTC | #6
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
James Hilliard Sept. 18, 2018, 8:36 p.m. UTC | #7
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 mbox series

Patch

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