[Spice-devel,vdagent-win,3/6] vdagent: file_xfer: make g_key_get_string safer

Submitted by Uri Lublin on Nov. 7, 2013, 10:02 p.m.

Details

Message ID 1383861776-25959-4-git-send-email-uril@redhat.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin Nov. 7, 2013, 10:02 p.m.
By providing the size of the destination string buffer.

Patch hide | download patch | download mbox

diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
index 2a6480a..0c44c45 100644
--- a/vdagent/file_xfer.cpp
+++ b/vdagent/file_xfer.cpp
@@ -49,7 +49,7 @@  void FileXfer::handle_start(VDAgentFileXferStartMessage* start,

     status->id = start->id;
     status->result = VD_AGENT_FILE_XFER_STATUS_ERROR;
-    if (!g_key_get_string(file_meta, "vdagent-file-xfer", "name", file_name) ||
+    if (!g_key_get_string(file_meta, "vdagent-file-xfer", "name", file_name, sizeof(file_name)) ||
             !g_key_get_uint64(file_meta, "vdagent-file-xfer", "size", &file_size)) {
         vd_printf("file id %u meta parsing failed", start->id);
         return;
@@ -181,10 +181,12 @@  bool FileXfer::dispatch(VDAgentMessage* msg, VDAgentFileXferStatusMessage* statu
 //minimal parsers for GKeyFile, supporting only key=value with no spaces.
 #define G_KEY_MAX_LEN 256

-bool FileXfer::g_key_get_string(char* data, const char* group, const char* key, char* value)
+bool FileXfer::g_key_get_string(char* data, const char* group, const char* key, char* value,
+                                                  unsigned vsize)
 {
     char group_pfx[G_KEY_MAX_LEN], key_pfx[G_KEY_MAX_LEN];
-    char *group_pos, *key_pos, *next_group_pos;
+    char *group_pos, *key_pos, *next_group_pos, *start, *end;
+    unsigned len;

     snprintf(group_pfx, sizeof(group_pfx), "[%s]", group);
     if (!(group_pos = strstr((char*)data, group_pfx))) return false;
@@ -193,15 +195,26 @@  bool FileXfer::g_key_get_string(char* data, const char* group, const char* key,
     if (!(key_pos = strstr(group_pos, key_pfx))) return false;

     next_group_pos = strstr(group_pos + strlen(group_pfx), "[");
-    if (next_group_pos && key_pos > next_group_pos) return false; 
+    if (next_group_pos && key_pos > next_group_pos) return false;

-    return !!sscanf(key_pos + strlen(key_pfx), "%s\n", value);
+    start = key_pos + strlen(key_pfx);
+    end = strchr(start, '\n');
+    if (!end) return false;
+
+    len = end - start;
+    if (len >= vsize) return false;
+
+    memcpy(value, start, len);
+    value[len] = '\0';
+
+    return true;
 }

 bool FileXfer::g_key_get_uint64(char* data, const char* group, const char* key, uint64_t* value)
 {
     char str[G_KEY_MAX_LEN];

-    if (!g_key_get_string(data, group, key, str)) return false;
+    if (!g_key_get_string(data, group, key, str, sizeof(str)))
+        return false;
     return !!sscanf(str, "%" PRIu64, value);
 }
diff --git a/vdagent/file_xfer.h b/vdagent/file_xfer.h
index 649b296..b506f59 100644
--- a/vdagent/file_xfer.h
+++ b/vdagent/file_xfer.h
@@ -44,7 +44,8 @@  private:
     void handle_start(VDAgentFileXferStartMessage* start, VDAgentFileXferStatusMessage* status);
     bool handle_data(VDAgentFileXferDataMessage* data, VDAgentFileXferStatusMessage* status);
     void handle_status(VDAgentFileXferStatusMessage* status);
-    bool g_key_get_string(char* data, const char* group, const char* key, char* value);
+    bool g_key_get_string(char* data, const char* group, const char* key, char* value,
+                                        unsigned vsize);
     bool g_key_get_uint64(char* data, const char* group, const char* key, uint64_t* value);

 private:

Comments

On Fri, Nov 08, 2013 at 12:02:53AM +0200, Uri Lublin wrote:
> By providing the size of the destination string buffer.
> 
> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index 2a6480a..0c44c45 100644
> --- a/vdagent/file_xfer.cpp
> +++ b/vdagent/file_xfer.cpp
> @@ -49,7 +49,7 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage* start,
> 
>      status->id = start->id;
>      status->result = VD_AGENT_FILE_XFER_STATUS_ERROR;
> -    if (!g_key_get_string(file_meta, "vdagent-file-xfer", "name", file_name) ||
> +    if (!g_key_get_string(file_meta, "vdagent-file-xfer", "name", file_name, sizeof(file_name)) ||
>              !g_key_get_uint64(file_meta, "vdagent-file-xfer", "size", &file_size)) {
>          vd_printf("file id %u meta parsing failed", start->id);
>          return;
> @@ -181,10 +181,12 @@ bool FileXfer::dispatch(VDAgentMessage* msg, VDAgentFileXferStatusMessage* statu
>  //minimal parsers for GKeyFile, supporting only key=value with no spaces.
>  #define G_KEY_MAX_LEN 256
> 
> -bool FileXfer::g_key_get_string(char* data, const char* group, const char* key, char* value)
> +bool FileXfer::g_key_get_string(char* data, const char* group, const char* key, char* value,
> +                                                  unsigned vsize)
>  {
>      char group_pfx[G_KEY_MAX_LEN], key_pfx[G_KEY_MAX_LEN];
> -    char *group_pos, *key_pos, *next_group_pos;
> +    char *group_pos, *key_pos, *next_group_pos, *start, *end;
> +    unsigned len;
> 
>      snprintf(group_pfx, sizeof(group_pfx), "[%s]", group);
>      if (!(group_pos = strstr((char*)data, group_pfx))) return false;
> @@ -193,15 +195,26 @@ bool FileXfer::g_key_get_string(char* data, const char* group, const char* key,
>      if (!(key_pos = strstr(group_pos, key_pfx))) return false;
> 
>      next_group_pos = strstr(group_pos + strlen(group_pfx), "[");
> -    if (next_group_pos && key_pos > next_group_pos) return false; 
> +    if (next_group_pos && key_pos > next_group_pos) return false;
> 
> -    return !!sscanf(key_pos + strlen(key_pfx), "%s\n", value);
> +    start = key_pos + strlen(key_pfx);
> +    end = strchr(start, '\n');
> +    if (!end) return false;
> +
> +    len = end - start;
> +    if (len >= vsize) return false;
> +
> +    memcpy(value, start, len);
> +    value[len] = '\0';
> +
> +    return true;
>  }
> 
>  bool FileXfer::g_key_get_uint64(char* data, const char* group, const char* key, uint64_t* value)
>  {
>      char str[G_KEY_MAX_LEN];
> 
> -    if (!g_key_get_string(data, group, key, str)) return false;
> +    if (!g_key_get_string(data, group, key, str, sizeof(str)))
> +        return false;
>      return !!sscanf(str, "%" PRIu64, value);
>  }
> diff --git a/vdagent/file_xfer.h b/vdagent/file_xfer.h
> index 649b296..b506f59 100644
> --- a/vdagent/file_xfer.h
> +++ b/vdagent/file_xfer.h
> @@ -44,7 +44,8 @@ private:
>      void handle_start(VDAgentFileXferStartMessage* start, VDAgentFileXferStatusMessage* status);
>      bool handle_data(VDAgentFileXferDataMessage* data, VDAgentFileXferStatusMessage* status);
>      void handle_status(VDAgentFileXferStatusMessage* status);
> -    bool g_key_get_string(char* data, const char* group, const char* key, char* value);
> +    bool g_key_get_string(char* data, const char* group, const char* key, char* value,
> +                                        unsigned vsize);
>      bool g_key_get_uint64(char* data, const char* group, const char* key, uint64_t* value);

Looks good ,ACK.

> 
>  private:
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel