On Thu, Jul 4, 2019 at 6:02 PM Richard Haines
<richard_c_haines@xxxxxxxxxxxxxx> wrote:
>
> We used to hash the file_context and skip the restorecon on the top
> level directory if the hash doesn't change. But the file_context
> might change after an OTA update; and some users experienced long
> restorecon time as they have lots of files under directories like
> /data/media.
>
> This CL tries to hash all the partial match entries in the
> file_context for each directory; and skips the restorecon if that
> digest stays the same, regardless of the changes to the other parts
> of file_context.
>
> This is a version ported from Android that was originally written by:
> xunchang <xunchang@xxxxxxxxxx>
>
> Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> ---
> V2 Change:
> Restore using SELABEL_OPT_DIGEST
> V3 Change:
> Replace memcpy in get_digests_all_partial_matches() to overcome error
> flagged by -D_FORTIFY_SOURCE= when building only libselinux.
[...]
> +static void digestcpy(uint8_t *dest, const uint8_t *src, size_t count)
> +{
> + uint8_t *destp = dest;
> + const uint8_t *srcp = src;
> +
> + while (count) {
> + *(destp++) = *(srcp++);
> + --count;
> + }
> +}
> +
> +/*
> + * Returns true if the digest of all partial matched contexts is the same as
> + * the one saved by setxattr, otherwise returns false. The length of the SHA1
> + * digest will always be returned. The caller must free any returned digests.
> + */
> +static bool get_digests_all_partial_matches(struct selabel_handle *rec,
> + const char *pathname,
> + uint8_t **calculated_digest,
> + uint8_t **xattr_digest,
> + size_t *digest_len)
> +{
> + uint8_t read_digest[SHA1_HASH_SIZE];
> + ssize_t read_size = getxattr(pathname, RESTORECON_PARTIAL_MATCH_DIGEST,
> + read_digest, SHA1_HASH_SIZE);
> + uint8_t hash_digest[SHA1_HASH_SIZE];
> + bool status = selabel_hash_all_partial_matches(rec, pathname,
> + hash_digest);
> +
> + *xattr_digest = NULL;
> + *calculated_digest = NULL;
> + *digest_len = SHA1_HASH_SIZE;
> +
> + if (read_size == SHA1_HASH_SIZE) {
> + *xattr_digest = calloc(1, sizeof(SHA1_HASH_SIZE + 1));
> + if (!*xattr_digest)
> + goto oom;
> +
> + digestcpy(*xattr_digest, read_digest, SHA1_HASH_SIZE);
> + }
> +
> + if (status) {
> + *calculated_digest = calloc(1, sizeof(SHA1_HASH_SIZE + 1));
> + if (!*calculated_digest)
> + goto oom;
> +
> + digestcpy(*calculated_digest, hash_digest, SHA1_HASH_SIZE);
> + }
> +
No. This is hiding a real bug, not fixing it. Here is what this code does:
* sizeof(SHA1_HASH_SIZE + 1) gets replaced with sizeof(( 160 / 8 ) +
1) = sizeof(int) = 4
* "*xattr_digest = calloc(1, sizeof(SHA1_HASH_SIZE + 1));" is replaced
by "*xattr_digest = calloc(1, 4);" : it allocates 4 bytes instead of
160/8=20
* digestcpy(*xattr_digest, read_digest, SHA1_HASH_SIZE) copies 20
bytes into a 4-byte buffer, which is a vulnerability kind called "heap
overflow".
In the end, you have only hidden the heap overflow you wanted to
introduce and which was correctly reported by the compiler. Please do
not re-implement memcpy.
A real fix would consists in using calloc(1, SHA1_HASH_SIZE + 1),
without the sizeof.
Thanks,
Nicolas
