system: sysupgrade: rework firmware validation

Fixes following deficiencies:

 * unhandled read() errors
 * everything bundled in one long function, which is hard to follow and
   reason about
 * JSON parser errors are being ignored, anything else then
   json_tokener_continue is fatal error
 * JSON parser errors are being output to stderr, thus invisible via SSH
 * validate_firmware_image_call can fail at a lot of places, but we just
   get one generic "Firmware image couldn't be validated" so it's hard
   to debug

Cc: Rafał Miłecki <rafal@milecki.pl>
Tested-by: Kuan-Yi Li <kyli@abysm.org>
Signed-off-by: Petr Štetiar <ynezz@true.cz>

diff --git a/system.c b/system.c
index 5cd88e0d82272d809226d957246180ebd8a09fad..ffd8f7175594338c5274f11d8c789446fe770bae 100644 (file)
--- a/system.c
+++ b/system.c
@@ -37,6 +37,12 @@ static struct blob_buf b;
 static int notify;
 static struct ubus_context *_ctx;
 
+enum vjson_state {
+       VJSON_ERROR,
+       VJSON_CONTINUE,
+       VJSON_SUCCESS,
+};
+
 static int system_board(struct ubus_context *ctx, struct ubus_object *obj,
                  struct ubus_request_data *req, const char *method,
                  struct blob_attr *msg)
@@ -413,30 +419,128 @@ static int proc_signal(struct ubus_context *ctx, struct ubus_object *obj,
        return 0;
 }
 
+__attribute__((format (printf, 2, 3)))
+static enum vjson_state vjson_error(char **b, const char *fmt, ...)
+{
+       static char buf[256] = { 0 };
+       const char *pfx = "Firmware image couldn't be validated: ";
+       va_list va;
+       int r;
+
+       r = snprintf(buf, sizeof(buf), "%s", pfx);
+       if (r < 0) {
+               *b = "vjson_error() snprintf failed";
+               return VJSON_ERROR;
+       }
+
+       va_start(va, fmt);
+       r = vsnprintf(buf+r, sizeof(buf)-r, fmt, va);
+       if (r < 0) {
+               *b = "vjson_error() vsnprintf failed";
+               return VJSON_ERROR;
+       }
+       va_end(va);
+
+       *b = buf;
+       return VJSON_ERROR;
+}
+
+static enum vjson_state vjson_parse_token(json_tokener *tok, char *buf, ssize_t len, char **err)
+{
+       json_object *jsobj = NULL;
+
+       jsobj = json_tokener_parse_ex(tok, buf, len);
+       if (json_tokener_get_error(tok) == json_tokener_continue)
+               return VJSON_CONTINUE;
+
+       if (json_tokener_get_error(tok) == json_tokener_success) {
+               if (json_object_get_type(jsobj) != json_type_object) {
+                       json_object_put(jsobj);
+                       return vjson_error(err, "result is not an JSON object");
+               }
+
+               blobmsg_add_object(&b, jsobj);
+               json_object_put(jsobj);
+               return VJSON_SUCCESS;
+       }
+
+       return vjson_error(err, "failed to parse JSON: %s (%d)",
+                          json_tokener_error_desc(json_tokener_get_error(tok)),
+                          json_tokener_get_error(tok));
+}
+
+static enum vjson_state vjson_parse(int fd, char **err)
+{
+       enum vjson_state r = VJSON_ERROR;
+       size_t read_count = 0;
+       char buf[64] = { 0 };
+       json_tokener *tok;
+       ssize_t len;
+       int _errno;
+
+       tok = json_tokener_new();
+       if (!tok)
+               return vjson_error(err, "json_tokener_new() failed");
+
+       vjson_error(err, "incomplete JSON input");
+
+       while ((len = read(fd, buf, sizeof(buf)))) {
+               if (len < 0 && errno == EINTR)
+                       continue;
+
+               if (len < 0) {
+                       _errno = errno;
+                       json_tokener_free(tok);
+                       return vjson_error(err, "read() failed: %s (%d)",
+                                          strerror(_errno), _errno);
+               }
+
+               read_count += len;
+               r = vjson_parse_token(tok, buf, len, err);
+               if (r != VJSON_CONTINUE)
+                       break;
+
+               memset(buf, 0, sizeof(buf));
+       }
+
+       if (read_count == 0)
+               vjson_error(err, "no JSON input");
+
+       json_tokener_free(tok);
+       return r;
+}
+
 /**
  * validate_firmware_image_call - perform validation & store result in global b
  *
  * @file: firmware image path
  */
-static int validate_firmware_image_call(const char *file)
+static enum vjson_state validate_firmware_image_call(const char *file, char **err)
 {
        const char *path = "/usr/libexec/validate_firmware_image";
-       json_object *jsobj = NULL;
-       json_tokener *tok;
-       char buf[64];
-       ssize_t len;
+       enum vjson_state ret = VJSON_ERROR;
+       int _errno;
        int fds[2];
-       int err;
        int fd;
 
-       if (pipe(fds))
-               return -errno;
+       blob_buf_init(&b, 0);
+       vjson_error(err, "unhandled error");
+
+       if (pipe(fds)) {
+               _errno = errno;
+               return vjson_error(err, "pipe() failed: %s (%d)",
+                                  strerror(_errno), _errno);
+       }
 
        switch (fork()) {
        case -1:
+               _errno = errno;
+
                close(fds[0]);
                close(fds[1]);
-               return -errno;
+
+               return vjson_error(err, "fork() failed: %s (%d)",
+                                  strerror(_errno), _errno);
        case 0:
                /* Set stdin & stderr to /dev/null */
                fd = open("/dev/null", O_RDWR);
@@ -458,43 +562,10 @@ static int validate_firmware_image_call(const char *file)
        /* Parent process */
        close(fds[1]);
 
-       tok = json_tokener_new();
-       if (!tok) {
-               close(fds[0]);
-               return -ENOMEM;
-       }
-
-       blob_buf_init(&b, 0);
-       while ((len = read(fds[0], buf, sizeof(buf)))) {
-               if (len < 0 && errno == EINTR)
-                       continue;
-
-               jsobj = json_tokener_parse_ex(tok, buf, len);
-
-               if (json_tokener_get_error(tok) == json_tokener_success)
-                       break;
-               else if (json_tokener_get_error(tok) == json_tokener_continue)
-                       continue;
-               else
-                       fprintf(stderr, "Failed to parse JSON: %d\n",
-                               json_tokener_get_error(tok));
-       }
-
+       ret = vjson_parse(fds[0], err);
        close(fds[0]);
 
-       err = -ENOENT;
-       if (jsobj) {
-               if (json_object_get_type(jsobj) == json_type_object) {
-                       blobmsg_add_object(&b, jsobj);
-                       err = 0;
-               }
-
-               json_object_put(jsobj);
-       }
-
-       json_tokener_free(tok);
-
-       return err;
+       return ret;
 }
 
 enum {
@@ -512,6 +583,8 @@ static int validate_firmware_image(struct ubus_context *ctx,
                                   const char *method, struct blob_attr *msg)
 {
        struct blob_attr *tb[__VALIDATE_FIRMWARE_IMAGE_MAX];
+       enum vjson_state ret = VJSON_ERROR;
+       char *err;
 
        if (!msg)
                return UBUS_STATUS_INVALID_ARGUMENT;
@@ -520,7 +593,8 @@ static int validate_firmware_image(struct ubus_context *ctx,
        if (!tb[VALIDATE_FIRMWARE_IMAGE_PATH])
                return UBUS_STATUS_INVALID_ARGUMENT;
 
-       if (validate_firmware_image_call(blobmsg_get_string(tb[VALIDATE_FIRMWARE_IMAGE_PATH])))
+       ret = validate_firmware_image_call(blobmsg_get_string(tb[VALIDATE_FIRMWARE_IMAGE_PATH]), &err);
+       if (ret != VJSON_SUCCESS)
                return UBUS_STATUS_UNKNOWN_ERROR;
 
        ubus_send_reply(ctx, req, b.head);
@@ -580,6 +654,8 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
        struct blob_attr *validation[__VALIDATION_MAX];
        struct blob_attr *tb[__SYSUPGRADE_MAX];
        bool valid, forceable, allow_backup;
+       enum vjson_state ret = VJSON_ERROR;
+       char *err;
 
        if (!msg)
                return UBUS_STATUS_INVALID_ARGUMENT;
@@ -588,8 +664,9 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
        if (!tb[SYSUPGRADE_PATH] || !tb[SYSUPGRADE_PREFIX])
                return UBUS_STATUS_INVALID_ARGUMENT;
 
-       if (validate_firmware_image_call(blobmsg_get_string(tb[SYSUPGRADE_PATH]))) {
-               sysupgrade_error(ctx, req, "Firmware image couldn't be validated");
+       ret = validate_firmware_image_call(blobmsg_get_string(tb[SYSUPGRADE_PATH]), &err);
+       if (ret != VJSON_SUCCESS) {
+               sysupgrade_error(ctx, req, err);
                return UBUS_STATUS_UNKNOWN_ERROR;
        }