On 03/02/10 18:34, Richard W.M. Jones wrote:

Subject: [PATCH 09/12] hivex: Begin implementation of writing to hives.

This implements hivex_node_set_values which is used to delete the (key, value) pairs at a node and optionally replace them with a new set.

This also implements hivex_commit which is used to commit changes to hives back to disk. --- hivex/hivex.c | 384 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ hivex/hivex.h | 12 ++ hivex/hivex.pod | 125 ++++++++++++++++++ 3 files changed, 521 insertions(+), 0 deletions(-)

This looks generally ok. However, it is *extremely* dense. I've indicated a few places where I feel there are insufficient comments, or magic numbers making the code less clear than it could be.

+/* Allocate an hbin (page), extending the malloc'd space if necessary, + * and updating the hive handle fields (but NOT the hive disk header + * -- the hive disk header is updated when we commit). This function + * also extends the bitmap if necessary. + * + * 'allocation_hint' is the size of the block allocation we would like + * to make. Normally registry blocks are very small (avg 50 bytes) + * and are contained in standard-sized pages (4KB), but the registry + * can support blocks which are larger than a standard page, in which + * case it creates a page of 8KB, 12KB etc. + * + * Returns: + * > 0 : offset of first usable byte of new page (after page header) + * 0 : error (errno set) + */ +static size_t +allocate_page (hive_h *h, size_t allocation_hint) +{

Every usage of nr_4k_pages in this function multiplies it by 4096. It would be more readable to choose a different name which reflects that it's simply a rounding.

+ /* In almost all cases this will be 1. */ + size_t nr_4k_pages = + 1 + (allocation_hint + sizeof (struct ntreg_hbin_page) - 1) / 4096; + assert (nr_4k_pages >= 1);

I think this assert is impossible to trigger.

+ /* 'extend' is the number of bytes to extend the file by. Note that + * hives found in the wild often contain slack between 'endpages' + * and the actual end of the file, so we don't always need to make + * the file larger. + */ + ssize_t extend = h->endpages + nr_4k_pages * 4096 - h->size; + + if (h->msglvl >= 2) { + fprintf (stderr, "allocate_page: current endpages = 0x%zx, current size = 0x%zx\n", + h->endpages, h->size); + fprintf (stderr, "allocate_page: extending file by %zd bytes (<= 0 if no extension)\n", + extend); + } + + if (extend > 0) { + size_t oldsize = h->size; + size_t newsize = h->size + extend;

In general, are we worrying about overflow?

+ char *newaddr = realloc (h->addr, newsize); + if (newaddr == NULL) + return 0; + + size_t oldbitmapsize = 1 + oldsize / 32; + size_t newbitmapsize = 1 + newsize / 32; + char *newbitmap = realloc (h->bitmap, newbitmapsize); + if (newbitmap == NULL) { + free (newaddr); + return 0; + } + + h->addr = newaddr; + h->size = newsize; + h->bitmap = newbitmap; + + memset (h->addr + oldsize, 0, newsize - oldsize); + memset (h->bitmap + oldbitmapsize, 0, newbitmapsize - oldbitmapsize); + } + + size_t offset = h->endpages; + h->endpages += nr_4k_pages * 4096; + + if (h->msglvl >= 2) + fprintf (stderr, "allocate_page: new endpages = 0x%zx, new size = 0x%zx\n", + h->endpages, h->size); + + /* Write the hbin header. */ + struct ntreg_hbin_page *page = + (struct ntreg_hbin_page *) (h->addr + offset); + page->magic[0] = 'h'; + page->magic[1] = 'b'; + page->magic[2] = 'i'; + page->magic[3] = 'n'; + page->offset_first = htole32 (offset - 0x1000);

What's 0x1000? Can you comment this?

+ page->page_size = htole32 (nr_4k_pages * 4096); + memset (page->unknown, 0, sizeof (page->unknown)); + + if (h->msglvl >= 2) + fprintf (stderr, "allocate_page: new page at 0x%zx\n", offset); + + /* Offset of first usable byte after the header. */ + return offset + sizeof (struct ntreg_hbin_page); +} + +/* Allocate a single block, first allocating an hbin (page) at the end + * of the current file if necessary. NB. To keep the implementation + * simple and more likely to be correct, we do not reuse existing free + * blocks. + * + * seg_len is the size of the block (this INCLUDES the block header). + * The header of the block is initialized to -seg_len (negative to + * indicate used). id[2] is the block ID (type), eg. "nk" for nk- + * record. The block bitmap is updated to show this block as valid. + * The rest of the contents of the block will be zero. + * + * Returns: + * > 0 : offset of new block + * 0 : error (errno set) + */ +static size_t +allocate_block (hive_h *h, size_t seg_len, const char id[2]) +{ + if (!h->writable) { + errno = EROFS; + return 0; + } + + if (seg_len < 4) {

Can you replace this '4' with a sizeof(ntreg_hbin_block.seg_len)?

+ /* The caller probably forgot to include the header. Note that + * value lists have no ID field, so seg_len == 4 would be possible + * for them, albeit unusual. + */ + if (h->msglvl >= 2) + fprintf (stderr, "allocate_block: refusing too small allocation (%zu), returning ERANGE\n", + seg_len); + errno = ERANGE; + return 0; + } + + /* Refuse really large allocations. */ + if (seg_len > 1000000) {

Can you replace 1000000 with a #define?

+ if (h->msglvl >= 2) + fprintf (stderr, "allocate_block: refusing large allocation (%zu), returning ERANGE\n", + seg_len); + errno = ERANGE; + return 0; + } + + /* Round up allocation to multiple of 8 bytes. All blocks must be + * on an 8 byte boundary. + */ + seg_len = (seg_len + 7) & ~7; + + /* Allocate a new page if necessary. */ + if (h->endblocks == 0 || h->endblocks + seg_len > h->endpages) { + size_t newendblocks = allocate_page (h, seg_len); + if (newendblocks == 0) + return 0; + h->endblocks = newendblocks; + } + + size_t offset = h->endblocks; + + if (h->msglvl >= 2) + fprintf (stderr, "allocate_block: new block at 0x%zx, size %zu\n", + offset, seg_len); + + struct ntreg_hbin_block *blockhdr = + (struct ntreg_hbin_block *) (h->addr + offset); + + blockhdr->seg_len = htole32 (- (int32_t) seg_len); + if (id[0] && id[1] && seg_len >= 6) {

Can you replace 6 with sizeof(ntreg_hbin_block)?

+ blockhdr->id[0] = id[0]; + blockhdr->id[1] = id[1]; + } + + h->endblocks += seg_len; + + /* If there is space after the last block in the last page, then we + * have to put a dummy free block header here to mark the rest of + * the page as free. + */ + ssize_t rem = h->endpages - h->endblocks; + if (rem > 0) { + if (h->msglvl >= 2) + fprintf (stderr, "allocate_block: marking remainder of page free starting at 0x%zx, size %zd\n", + h->endblocks, rem); + + assert (rem >= 4);

This assertion doesn't look safe. Why must this be the case? In the event that rem < 4, would you allocate a new page for the dummy free block?

+ blockhdr = (struct ntreg_hbin_block *) (h->addr + h->endblocks); + blockhdr->seg_len = htole32 ((int32_t) rem);

Will blockhdr->id ever be read here? Is 0x00 valid?

+ } + + return offset; +} +

+ +/* Delete all existing values at this node. */ +static int +delete_values (hive_h *h, hive_node_h node) +{ + assert (h->writable); + + hive_value_h *values; + size_t *blocks; + if (get_values (h, node, &values, &blocks) == -1) + return -1; + + size_t i; + for (i = 0; blocks[i] != 0; ++i) + mark_block_unused (h, blocks[i]); + + free (blocks); + + for (i = 0; values[i] != 0; ++i) { + struct ntreg_vk_record *vk = + (struct ntreg_vk_record *) (h->addr + values[i]); + + size_t len; + len = le32toh (vk->data_len); + if (len == 0x80000000) /* special case */ + len = 4; + len &= 0x7fffffff;

Comment, please. What's going on with this record length?

+ + if (len > 4) { /* non-inline, so remove data block */ + size_t data_offset = le32toh (vk->data_offset); + data_offset += 0x1000; + mark_block_unused (h, data_offset);

More magic numbers :) Why is this necessary?

+ } + + /* remove vk record */ + mark_block_unused (h, values[i]); + } + + free (values); + + struct ntreg_nk_record *nk = (struct ntreg_nk_record *) (h->addr + node); + nk->nr_values = htole32 (0); + nk->vallist = htole32 (0xffffffff); + + return 0; +} + +int +hivex_commit (hive_h *h, const char *filename, int flags) +{ + if (flags != 0) { + errno = EINVAL; + return -1; + }

What are you intending to use the flags for?

+ if (!h->writable) { + errno = EROFS; + return -1; + } + + filename = filename ? : h->filename; + int fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0666); + if (fd == -1) + return -1; + + /* Update the header fields. */ + uint32_t sequence = le32toh (h->hdr->sequence1); + sequence++; + h->hdr->sequence1 = htole32 (sequence); + h->hdr->sequence2 = htole32 (sequence); + /* XXX Ought to update h->hdr->last_modified. */ + h->hdr->blocks = htole32 (h->endpages - 0x1000); + + /* Recompute header checksum. */ + uint32_t sum = header_checksum (h); + h->hdr->csum = htole32 (sum); + + if (h->msglvl >= 2) + fprintf (stderr, "hivex_commit: new header checksum: 0x%x\n", sum); + + if (full_write (fd, h->addr, h->size) != h->size) { + int err = errno; + close (fd); + errno = err; + return -1; + } + + if (close (fd) == -1) + return -1; + + return 0; +} + +int +hivex_node_set_values (hive_h *h, hive_node_h node, + size_t nr_values, const hive_set_value *values, + int flags) +{ + if (!h->writable) { + errno = EROFS; + return -1; + } + + if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) { + errno = EINVAL; + return -1;

This would be an internal inconsistency error, surely. If so, you probably want an assert here instead.

+ } + + /* Delete all existing values. */ + if (delete_values (h, node) == -1) + return -1; + + if (nr_values == 0) + return 0; + + /* Allocate value list node. Value lists have no id field. */ + static const char nul_id[2] = { 0, 0 }; + size_t seg_len = + sizeof (struct ntreg_value_list) + (nr_values - 1) * sizeof (uint32_t);

Why are you subtracting 1 from nr_values above?

+ size_t vallist_offs = allocate_block (h, seg_len, nul_id); + if (vallist_offs == 0) + return -1; + + struct ntreg_nk_record *nk = (struct ntreg_nk_record *) (h->addr + node); + nk->nr_values = htole32 (nr_values); + nk->vallist = htole32 (vallist_offs - 0x1000);

Whatever the purpose of adding or subtracting 0x1000 to offsets is, it could probably do with a descriptively-named macro, which also does htole32 or le32toh as appropriate.

+ struct ntreg_value_list *vallist = + (struct ntreg_value_list *) (h->addr + vallist_offs); + + size_t i; + for (i = 0; i < nr_values; ++i) { + /* Allocate vk record to store this (key, value) pair. */ + static const char vk_id[2] = { 'v', 'k' }; + seg_len = sizeof (struct ntreg_vk_record) + strlen (values[i].key); + size_t vk_offs = allocate_block (h, seg_len, vk_id); + if (vk_offs == 0) + return -1; + + vallist->offset[i] = htole32 (vk_offs - 0x1000); + + struct ntreg_vk_record *vk = (struct ntreg_vk_record *) (h->addr + vk_offs); + size_t name_len = strlen (values[i].key);

If you did this further up you could re-use the result of strlen.

+ vk->name_len = htole16 (name_len); + strcpy (vk->name, values[i].key); + vk->data_type = htole32 (values[i].t); + vk->data_len = htole16 (values[i].len); + vk->flags = name_len == 0 ? 0 : 1; + + if (values[i].len <= 4) /* Store data inline. */

sizeof.

+ memcpy (&vk->data_offset, values[i].value, values[i].len); + else { + size_t offs = allocate_block (h, values[i].len + 4, nul_id);

sizeof.

+ if (offs == 0) + return -1; + memcpy (h->addr + offs + 4, values[i].value, values[i].len);

sizeof, or the address of a struct member.

+ vk->data_offset = htole32 (offs - 0x1000); + } + + if (name_len * 2 > le32toh (nk->max_vk_name_len)) + nk->max_vk_name_len = htole32 (name_len * 2);

Why is name_len multiplied by 2? Comment, please.

+ if (values[i].len > le32toh (nk->max_vk_data_len)) + nk->max_vk_data_len = htole32 (values[i].len); + } + + return 0; +} diff --git a/hivex/hivex.h b/hivex/hivex.h index 56718b4..6a3cb3a 100644 --- a/hivex/hivex.h +++ b/hivex/hivex.h @@ -110,6 +110,18 @@ struct hivex_visitor { extern int hivex_visit (hive_h *h, const struct hivex_visitor *visitor, size_t len, void *opaque, int flags); extern int hivex_visit_node (hive_h *h, hive_node_h node, const struct hivex_visitor *visitor, size_t len, void *opaque, int flags);

+extern int hivex_commit (hive_h *h, const char *filename, int flags); + +struct hive_set_value { + char *key; + hive_type t; + size_t len; + char *value; +}; +typedef struct hive_set_value hive_set_value; + +extern int hivex_node_set_values (hive_h *h, hive_node_h node, size_t nr_values, const hive_set_value *values, int flags); + #ifdef __cplusplus } #endif diff --git a/hivex/hivex.pod b/hivex/hivex.pod index 5a58144..5df75aa 100644 --- a/hivex/hivex.pod +++ b/hivex/hivex.pod @@ -326,6 +326,127 @@ starts at C<node>.

=back

+=head2 WRITING TO HIVE FILES + +The hivex library supports making limited modifications to hive files. +We have tried to implement this very conservatively in order to reduce +the chance of corrupting your registry. However you should be careful +and take back-ups, since Microsoft has never documented the hive +format, and so it is possible there are nuances in the +reverse-engineered format that we do not understand. + +To be able to modify a hive, you must pass the C<HIVEX_OPEN_WRITE> +flag to C<hivex_open>, otherwise any write operation will return with +errno C<EROFS>. + +The write operations shown below do not modify the on-disk file +immediately. You must call C<hivex_commit> in order to write the +changes to disk. If you call C<hivex_close> without committing then +any writes are discarded. + +Hive files internally consist of a "memory dump" of binary blocks +(like the C heap), and some of these blocks can be unused. The hivex +library never reuses these unused blocks. Instead, to ensure +robustness in the face of the partially understood on-disk format, +hivex only allocates new blocks after the end of the file, and makes +minimal modifications to existing structures in the file to point to +these new blocks. This makes hivex slightly less disk-efficient than +it could be, but disk is cheap, and registry modifications tend to be +very small. + +When deleting nodes, it is possible that this library may leave +unreachable live blocks in the hive. This is because certain parts of +the hive disk format such as security (sk) records and big data (db) +records and classname fields are not well understood (and not +documented at all) and we play it safe by not attempting to modify +them. Apart from wasting a little bit of disk space, it is not +thought that unreachable blocks are a problem. + +=over 4 + +=item int hivex_commit (hive_h *h, const char *filename, int flags); + +Commit (write) any changes which have been made. + +C<filename> is the new file to write. If C<filename == NULL> then we +overwrite the original file (ie. the file name that was passed to +C<hivex_open>). C<flags> is not used, always pass 0. + +Returns 0 on success. On error this returns -1 and sets errno. + +Note this does not close the hive handle. You can perform further +operations on the hive after committing, including making more +modifications. If you no longer wish to use the hive, call +C<hivex_close> after this. + +=item hive_set_value + +The typedef C<hive_set_value> is used in conjunction with the +C<hivex_node_set_values> call described below. + + struct hive_set_value { + char *key; /* key - a UTF-8 encoded ASCIIZ string */ + hive_type t; /* type of value field */ + size_t len; /* length of value field in bytes */ + char *value; /* value field */ + }; + typedef struct hive_set_value hive_set_value; + +To set the default value for a node, you have to pass C<key = "">. + +Note that the C<value> field is just treated as a list of bytes, and +is stored directly in the hive. The caller has to ensure correct +encoding and endianness, for example converting dwords to little +endian. + +The correct type and encoding for values depends on the node and key +in the registry, the version of Windows, and sometimes even changes +between versions of Windows for the same key. We don't document it +here. Often it's not documented at all. + +=item int hivex_node_set_values (hive_h *h, hive_node_h node, size_t nr_values, const hive_set_value *values, int flags); + +This call can be used to set all the (key, value) pairs stored in C<node>. + +C<node> is the node to modify. C<values> is an array of (key, value) +pairs. There should be C<nr_values> elements in this array. C<flags> +is not used, always pass 0. + +Any existing values stored at the node are discarded, and their +C<hive_value_h> handles become invalid. Thus you can remove all +values stored at C<node> by passing C<nr_values = 0>. + +Returns 0 on success. On error this returns -1 and sets errno. + +Note that this library does not offer a way to modify just a single +key at a node. We don't implement a way to do this efficiently. + +=back + +=head3 WRITE OPERATIONS WHICH ARE NOT SUPPORTED + +=over 4 + +=item * + +Changing the root node. + +=item * + +Creating a new hive file from scratch. This is impossible at present +because not all fields in the header are understood. + +=item * + +Modifying or deleting single values at a node. + +=item * + +Modifying security key (sk) records or classnames. These are not +well understood. + +=back + =head1 THE STRUCTURE OF THE WINDOWS REGISTRY

Note: To understand the relationship between hives and the common @@ -452,6 +573,10 @@ Registry contains cycles.

Field in the registry out of range.

+=item EROFS + +Tried to write to a registry which is not opened for writing. + =back

=head1 ENVIRONMENT VARIABLES -- 1.6.5.2

-- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490