From c155e206bf806dd639b57922c3eef0b428c985df Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 27 Aug 2012 23:10:26 -0500 Subject: [PATCH] verify_dentry() Do some more verifications on each dentry after we read them all into memory. --- src/dentry.c | 76 +++++++++++++++++++++++++++++++++++++++++-- src/dentry.h | 2 ++ src/lookup_table.h | 10 ++++++ src/resource.c | 16 ++++++--- src/security.c | 12 +++++-- src/wim.c | 4 +-- src/wimlib_internal.h | 9 +++-- tests/test-imagex | 2 +- 8 files changed, 114 insertions(+), 17 deletions(-) diff --git a/src/dentry.c b/src/dentry.c index 6d104ee5..ac158e84 100644 --- a/src/dentry.c +++ b/src/dentry.c @@ -1049,7 +1049,7 @@ int read_dentry(const u8 metadata_resource[], u64 metadata_resource_len, dentry_common_init(dentry); /*Make sure the dentry really fits into the metadata resource.*/ - if (offset + 8 > metadata_resource_len) { + if (offset + 8 > metadata_resource_len || offset + 8 < offset) { ERROR("Directory entry starting at %"PRIu64" ends past the " "end of the metadata resource (size %"PRIu64")", offset, metadata_resource_len); @@ -1073,7 +1073,9 @@ int read_dentry(const u8 metadata_resource[], u64 metadata_resource_len, * not too short, read the rest of it (excluding the alternate data * streams, but including the file name and short name variable-length * fields) into memory. */ - if (offset + dentry->length >= metadata_resource_len) { + if (offset + dentry->length >= metadata_resource_len + || offset + dentry->length < offset) + { ERROR("Directory entry at offset %"PRIu64" and with size " "%"PRIu64" ends past the end of the metadata resource " "(size %"PRIu64")", @@ -1262,6 +1264,76 @@ out_free_file_name: return ret; } +/* Run some miscellaneous verifications on a WIM dentry */ +int verify_dentry(struct dentry *dentry, void *wim) +{ + const WIMStruct *w = wim; + const struct lookup_table *table = w->lookup_table; + const struct wim_security_data *sd = wim_const_security_data(w); + int ret = WIMLIB_ERR_INVALID_DENTRY; + + /* Check the security ID */ + if (dentry->security_id < -1) { + ERROR("Dentry `%s' has an invalid security ID (%d)", + dentry->full_path_utf8, dentry->security_id); + goto out; + } + if (dentry->security_id >= sd->num_entries) { + ERROR("Dentry `%s' has an invalid security ID (%d) " + "(there are only %u entries in the security table)", + dentry->full_path_utf8, dentry->security_id, + sd->num_entries); + goto out; + } + + /* Check that lookup table entries for all the resources exist, except + * if the SHA1 message digest is all 0's, which indicates there is + * intentionally no resource there. */ + if (w->hdr.total_parts == 1) { + for (unsigned i = 0; i <= dentry->num_ads; i++) { + struct lookup_table_entry *lte; + const u8 *hash; + hash = dentry_stream_hash_unresolved(dentry, i); + lte = __lookup_resource(table, hash); + if (!lte && !is_zero_hash(hash)) { + ERROR("Could not find lookup table entry for stream " + "%u of dentry `%s'", i, dentry->full_path_utf8); + goto out; + } + } + } + + /* Make sure there is only one un-named stream. */ + unsigned num_unnamed_streams = 0; + unsigned unnamed_stream_idx; + for (unsigned i = 0; i <= dentry->num_ads; i++) { + const u8 *hash; + hash = dentry_stream_hash_unresolved(dentry, i); + if (dentry_stream_name_len(dentry, i) && !is_zero_hash(hash)) { + num_unnamed_streams++; + unnamed_stream_idx = i; + } + } + if (num_unnamed_streams > 1) { + ERROR("Dentry `%s' has multiple (%u) un-named streams", + dentry->full_path_utf8, num_unnamed_streams); + goto out; + } + +#if 0 + /* Check timestamps */ + if (dentry->last_access_time < dentry->creation_time || + dentry->last_write_time < dentry->creation_time) { + WARNING("Dentry `%s' was created after it was last accessed or " + "written to", dentry->full_path_utf8); + } +#endif + + ret = 0; +out: + return ret; +} + /* * Writes a WIM dentry to an output buffer. * diff --git a/src/dentry.h b/src/dentry.h index 74776420..5b2aeab9 100644 --- a/src/dentry.h +++ b/src/dentry.h @@ -355,6 +355,8 @@ extern void calculate_dir_tree_statistics(struct dentry *root, extern int read_dentry(const u8 metadata_resource[], u64 metadata_resource_len, u64 offset, struct dentry *dentry); +extern int verify_dentry(struct dentry *dentry, void *wim); + extern int read_dentry_tree(const u8 metadata_resource[], u64 metadata_resource_len, struct dentry *dentry); diff --git a/src/lookup_table.h b/src/lookup_table.h index c211fe14..8634c7f9 100644 --- a/src/lookup_table.h +++ b/src/lookup_table.h @@ -294,6 +294,16 @@ static inline const u8 *dentry_stream_hash_unresolved(const struct dentry *dentr return dentry->ads_entries[stream_idx - 1].hash; } +static inline unsigned dentry_stream_name_len(const struct dentry *dentry, + unsigned stream_idx) +{ + wimlib_assert(stream_idx <= dentry->num_ads); + if (stream_idx == 0) + return dentry->file_name_len; + else + return dentry->ads_entries[stream_idx - 1].stream_name_len; +} + static inline const u8 *dentry_stream_hash_resolved(const struct dentry *dentry, unsigned stream_idx) { diff --git a/src/resource.c b/src/resource.c index dbdda6bc..3c573f1c 100644 --- a/src/resource.c +++ b/src/resource.c @@ -466,6 +466,7 @@ int read_wim_resource(const struct lookup_table_entry *lte, u8 buf[], if (!fp) { ERROR_WITH_ERRNO("Failed to open the file " "`%s'", lte->file_on_disk); + return WIMLIB_ERR_OPEN; } } ret = read_uncompressed_resource(fp, offset, size, buf); @@ -1093,14 +1094,13 @@ int write_dentry_resources(struct dentry *dentry, void *wim_p) * * @return: Zero on success, nonzero on failure. */ -int read_metadata_resource(FILE *fp, int wim_ctype, struct image_metadata *imd) +int read_metadata_resource(WIMStruct *w, struct image_metadata *imd) { u8 *buf; int ctype; u32 dentry_offset; int ret; struct dentry *dentry; - struct wim_security_data *sd; struct link_group_table *lgt; const struct lookup_table_entry *metadata_lte; u64 metadata_len; @@ -1150,7 +1150,7 @@ int read_metadata_resource(FILE *fp, int wim_ctype, struct image_metadata *imd) * and if successful, go ahead and calculate the offset in the metadata * resource of the root dentry. */ - ret = read_security_data(buf, metadata_len, &sd); + ret = read_security_data(buf, metadata_len, &imd->security_data); if (ret != 0) goto out_free_buf; @@ -1202,10 +1202,15 @@ int read_metadata_resource(FILE *fp, int wim_ctype, struct image_metadata *imd) ret = link_groups_free_duplicate_data(lgt); if (ret != 0) goto out_free_lgt; + + DEBUG("Running miscellaneous verifications on the dentry tree"); + ret = for_dentry_in_tree(dentry, verify_dentry, w); + if (ret != 0) + goto out_free_lgt; + DEBUG("Done reading image metadata"); imd->lgt = lgt; - imd->security_data = sd; imd->root_dentry = dentry; goto out_free_buf; out_free_lgt: @@ -1213,7 +1218,8 @@ out_free_lgt: out_free_dentry_tree: free_dentry_tree(dentry, NULL); out_free_security_data: - free_security_data(sd); + free_security_data(imd->security_data); + imd->security_data = NULL; out_free_buf: FREE(buf); return ret; diff --git a/src/security.c b/src/security.c index 99f09134..b30ffc65 100644 --- a/src/security.c +++ b/src/security.c @@ -63,6 +63,12 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len, p = get_u32(p, &sd->total_length); p = get_u32(p, &sd->num_entries); + if (sd->num_entries > 0x7fffffff) { + ERROR("Security data has too many entries!"); + ret = WIMLIB_ERR_INVALID_SECURITY_DATA; + goto out_free_sd; + } + /* Verify the listed total length of the security data is big enough to * include the sizes array, verify that the file data is big enough to * include it as well, then allocate the array of sizes. @@ -75,7 +81,7 @@ 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_RESOURCE_SIZE; + ret = WIMLIB_ERR_INVALID_SECURITY_DATA; goto out_free_sd; } @@ -94,7 +100,7 @@ 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_RESOURCE_SIZE; + ret = WIMLIB_ERR_INVALID_SECURITY_DATA; goto out_free_sd; } sd->sizes = MALLOC(sizes_size); @@ -133,7 +139,7 @@ int read_security_data(const u8 metadata_resource[], u64 metadata_resource_len, 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_RESOURCE_SIZE; + ret = WIMLIB_ERR_INVALID_SECURITY_DATA; goto out_free_sd; } sd->descriptors[i] = MALLOC(sd->sizes[i]); diff --git a/src/wim.c b/src/wim.c index 0b37899d..0d292e6c 100644 --- a/src/wim.c +++ b/src/wim.c @@ -222,9 +222,7 @@ int wimlib_select_image(WIMStruct *w, int image) "lookup table entry:"); print_lookup_table_entry(imd->metadata_lte); #endif - return read_metadata_resource(w->fp, - wimlib_get_compression_type(w), - imd); + return read_metadata_resource(w, imd); } } diff --git a/src/wimlib_internal.h b/src/wimlib_internal.h index be4c6091..c89d733e 100644 --- a/src/wimlib_internal.h +++ b/src/wimlib_internal.h @@ -191,8 +191,11 @@ struct wim_security_data { * that wimlib writes, currently), it will be 8 bytes. */ u32 total_length; - /* The number of security descriptors in the array @descriptors, below. */ - u32 num_entries; + /* The number of security descriptors in the array @descriptors, below. + * It is really an unsigned int, but it must fit into an int because the + * security ID's are signed. (Not like you would ever have more than a + * few hundred security descriptors anyway). */ + int32_t num_entries; /* Array of sizes of the descriptors in the array @descriptors. */ u64 *sizes; @@ -385,7 +388,7 @@ extern int extract_wim_resource_to_fd(const struct lookup_table_entry *lte, extern int extract_full_wim_resource_to_fd(const struct lookup_table_entry *lte, int fd); -extern int read_metadata_resource(FILE *fp, int wim_ctype, +extern int read_metadata_resource(WIMStruct *w, struct image_metadata *image_metadata); diff --git a/tests/test-imagex b/tests/test-imagex index 9a7f4f09..f1f2154a 100755 --- a/tests/test-imagex +++ b/tests/test-imagex @@ -260,7 +260,7 @@ if imagex append SOME_NONEXISTENT_FILE dir.wim; then fi echo "Testing appending directory containing unreadable file (should generate errors)" mkdir -p dir3 -touch dir3/file +echo 1 > dir3/file chmod -r dir3/file if imagex append dir3 dir.wim; then error "Incorrectly succeeded in capturing directory with unreadable file" -- 2.43.0