Update dentry, security data reading

diff --git a/src/dentry.c b/src/dentry.c
index 7d17fb9c9db0408d3486c0ef0d1f2e705607d42c..beb963d2c839f96517aac79b8ad3f9259db74270 100644 (file)
--- a/src/dentry.c
+++ b/src/dentry.c
@@ -1455,7 +1455,7 @@ int read_dentry(const u8 metadata_resource[], u64 metadata_resource_len,
 
                p = get_bytes(p, short_name_len, short_name);
                if (*(u16*)p)
-                       WARNING("Expected two zero bytes following the file name "
+                       WARNING("Expected two zero bytes following the short name of "
                                "`%s', but found non-zero bytes", file_name_utf8);
                p += 2;
        }
@@ -1470,18 +1470,35 @@ int read_dentry(const u8 metadata_resource[], u64 metadata_resource_len,
         * included in the dentry->length field for some reason.
         */
        if (inode->num_ads != 0) {
-               if (calculated_size > metadata_resource_len - offset) {
-                       ERROR("Not enough space in metadata resource for "
-                             "alternate stream entries");
-                       ret = WIMLIB_ERR_INVALID_DENTRY;
-                       goto out_free_short_name;
+
+               /* Trying different lengths is just a hack to make sure we have
+                * a chance of reading the ADS entries correctly despite the
+                * poor documentation. */
+
+               if (calculated_size != dentry->length) {
+                       WARNING("Trying calculated dentry length (%"PRIu64") "
+                               "instead of dentry->length field (%"PRIu64") "
+                               "to read ADS entries",
+                               calculated_size, dentry->length);
                }
-               ret = read_ads_entries(&metadata_resource[offset + calculated_size],
-                                      inode,
-                                      metadata_resource_len - offset - calculated_size);
-               if (ret != 0)
-                       goto out_free_short_name;
+               u64 lengths_to_try[3] = {calculated_size,
+                                        dentry->length + 7 & ~7,
+                                        dentry->length};
+               ret = WIMLIB_ERR_INVALID_DENTRY;
+               for (size_t i = 0; i < ARRAY_LEN(lengths_to_try); i++) {
+                       if (lengths_to_try[i] > metadata_resource_len - offset)
+                               continue;
+                       ret = read_ads_entries(&metadata_resource[offset + lengths_to_try[i]],
+                                              inode,
+                                              metadata_resource_len - offset - lengths_to_try[i]);
+                       if (ret == 0)
+                               goto out;
+               }
+               ERROR("Failed to read alternate data stream "
+                     "entries of `%s'", dentry->file_name_utf8);
+               goto out_free_short_name;
        }
+out:
 
        /* We've read all the data for this dentry.  Set the names and their
         * lengths, and we've done. */

diff --git a/src/resource.c b/src/resource.c
index 69ccf3844111c70536420bd3ff8c862ee6f432bc..013f9ec5145e44e54efa26ccb06923675e323506 100644 (file)
--- a/src/resource.c
+++ b/src/resource.c
@@ -1165,6 +1165,12 @@ int read_metadata_resource(WIMStruct *w, struct image_metadata *imd)
                return WIMLIB_ERR_INVALID_RESOURCE_SIZE;
        }
 
+       if (sizeof(size_t) < 8 && metadata_len > 0xffffffff) {
+               ERROR("Metadata resource is too large (%"PRIu64" bytes",
+                     metadata_len);
+               return WIMLIB_ERR_INVALID_RESOURCE_SIZE;
+       }
+
        /* Allocate memory for the uncompressed metadata resource. */
        buf = MALLOC(metadata_len);
 
@@ -1192,14 +1198,18 @@ int read_metadata_resource(WIMStruct *w, struct image_metadata *imd)
         * and if successful, go ahead and calculate the offset in the metadata
         * resource of the root dentry. */
 
+       wimlib_assert(imd->security_data == NULL);
        ret = read_security_data(buf, metadata_len, &imd->security_data);
        if (ret != 0)
                goto out_free_buf;
 
-       get_u32(buf, &dentry_offset);
-       if (dentry_offset == 0)
-               dentry_offset = 8;
-       dentry_offset = (dentry_offset + 7) & ~7;
+       dentry_offset = imd->security_data->total_length + 7 & ~7;
+
+       if (dentry_offset == 0) {
+               ERROR("Integer overflow while reading metadata resource");
+               ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
+               goto out_free_security_data;
+       }
 
        /* Allocate memory for the root dentry and read it into memory */
        dentry = MALLOC(sizeof(struct dentry));

diff --git a/src/security.c b/src/security.c
index 55a603d6eddfe0221276d508d5cb2dddf52c647b..ec1590f6f09a7636ff9a29a929eaf0712ff71a1a 100644 (file)
--- a/src/security.c
+++ b/src/security.c
@@ -37,7 +37,7 @@
  * invalid.  However, a security descriptor like this exists in the Windows 7
  * install.wim.  Here, security descriptors matching this pattern are modified
  * to have no SACL.  This should make no difference since the SACL had no
- * entries anyway; however  his ensures that that the security descriptors pass
+ * entries anyway; however this ensures that that the security descriptors pass
  * the validation in libntfs-3g.
  */
 static void empty_sacl_fixup(char *descr, u64 *size_p)
@@ -74,6 +74,11 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len,
        int ret;
        u64 total_len;
 
+       /*
+        * Sorry this function is excessively complicated--- I'm just being
+        * extremely careful about integer overflows.
+        */
+
        sd = MALLOC(sizeof(struct wim_security_data));
        if (!sd) {
                ERROR("Out of memory");
@@ -87,10 +92,14 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len,
        p = get_u32(p, &sd->total_length);
        p = get_u32(p, (u32*)&sd->num_entries);
 
-       if (sd->num_entries > 0x7fffffff) {
+       /* The security_id field of each dentry is a signed 32-bit integer, so
+        * the possible indices into the security descriptors table are 0
+        * through 0x7fffffff.  Which means 0x80000000 security descriptors
+        * maximum.  Not like you should ever have anywhere close to that many
+        * security descriptors! */
+       if (sd->num_entries > 0x80000000) {
                ERROR("Security data has too many entries!");
-               ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
-               goto out_free_sd;
+               goto out_invalid_sd;
        }
 
        /* Verify the listed total length of the security data is big enough to
@@ -105,17 +114,21 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len,
                ERROR("Security data total length (%u) is bigger than the "
                      "metadata resource length (%"PRIu64")",
                      sd->total_length, metadata_resource_len);
-               ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
-               goto out_free_sd;
+               goto out_invalid_sd;
        }
 
        DEBUG("Reading security data: %u entries, length = %u",
              sd->num_entries, sd->total_length);
 
        if (sd->num_entries == 0) {
-               /* No security data. */
-               total_len = 8;
-               goto out;
+               /* No security descriptors. */
+               if (sd->total_length != 0 && sd->total_length != 8) {
+                       ERROR("Invalid security data length (%u): expected 0 or 8",
+                             sd->total_length);
+                       goto out_invalid_sd;
+               }
+               sd->total_length = 8;
+               goto out_return_sd;
        }
 
        u64 sizes_size = (u64)sd->num_entries * sizeof(u64);
@@ -124,8 +137,11 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len,
                ERROR("Security data total length of %u is too short because "
                      "there must be at least %"PRIu64" bytes of security data",
                      sd->total_length, 8 + sizes_size);
-               ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
-               goto out_free_sd;
+               goto out_invalid_sd;
+       }
+       if (sizeof(size_t) < 8 && sizes_size > 0xffffffff) {
+               ERROR("Too many security descriptors!");
+               goto out_invalid_sd;
        }
        sd->sizes = MALLOC(sizes_size);
        if (!sd->sizes) {
@@ -155,16 +171,18 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len,
                              "(current total length = %"PRIu64", security "
                              "descriptor size = %"PRIu64")",
                              total_len, sd->sizes[i]);
-                       ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
-                       goto out_free_sd;
+                       goto out_invalid_sd;
                }
                total_len += sd->sizes[i];
+               /* This check assures that the descriptor size fits in a 32 bit
+                * integer.  Because if it didn't, the total length would come
+                * out bigger than sd->total_length, which is a 32 bit integer.
+                * */
                if (total_len > (u64)sd->total_length) {
                        ERROR("Security data total length of %u is too short "
                              "because there are at least %"PRIu64" bytes of "
                              "security data", sd->total_length, total_len);
-                       ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
-                       goto out_free_sd;
+                       goto out_invalid_sd;
                }
                sd->descriptors[i] = MALLOC(sd->sizes[i]);
                if (!sd->descriptors[i]) {
@@ -176,10 +194,18 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len,
                p = get_bytes(p, sd->sizes[i], sd->descriptors[i]);
                empty_sacl_fixup(sd->descriptors[i], &sd->sizes[i]);
        }
-out:
-       sd->total_length = (u32)total_len;
+       wimlib_assert(total_len <= 0xffffffff);
+       if ((total_len + 7 & ~7) != ((sd->total_length + 7) & ~7)) {
+               ERROR("Expected security data total length = %u, but "
+                     "calculated %u", sd->total_length, total_len);
+               goto out_invalid_sd;
+       }
+       sd->total_length = total_len;
+out_return_sd:
        *sd_p = sd;
        return 0;
+out_invalid_sd:
+       ret = WIMLIB_ERR_INVALID_SECURITY_DATA;
 out_free_sd:
        free_security_data(sd);
        return ret;